Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add protocol detection for get/request calls without explicit protocol #3950

Merged
merged 1 commit into from Sep 2, 2021

Conversation

thomaspurchas
Copy link
Contributor

You can call get/request on http/https without providing an
explicit protocol in the request options. In this case the protocol is
automatically set based on which module you made the call to.

Previously calls like this would result in breadcrumbs and traces
without a protocol in the URL creating downstream issues for consumers
of Breadcrumbs as the missing protocol results in URL throwing parsing
errors.

We now attempt to get the protocol by either extracting it from the
agent passed as part of the request options, or by extracting it from
the globalAgent on which ever http module is pass in this.

Fixes #3948

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

You can fix the lint CI errors by running yarn lint:fix. As for the Node 6,8 test failures, seems like there is an issue with the awaiting the promise. Maybe we can convert to just relying on callbacks?

@thomaspurchas
Copy link
Contributor Author

Fixed the lint issues. Tracking down the Node v8/6 bug was a little tricker, turned out that was related to how nock works, and how nocks patching interacts with Sentry's patching. Left a big comment explaining the very strange work around I had to use.

You can call `get`/`request` on `http`/`https` without providing an
explicit protocol in the request options. In this case the protocol is
automatically set based on which module you made the call to.

Previously calls like this would result in breadcrumbs and traces
without a protocol in the URL creating downstream issues for consumers
of Breadcrumbs as the missing protocol results in URL throwing parsing
errors.

We now attempt to get the protocol by either extracting it from the
`agent` passed as part of the request options, or by extracting it from
the `globalAgent` on which ever http module is pass in `this`.
@kamilogorek
Copy link
Contributor

That's exactly what we had to work around here

// NOTE: Prior to Node 9, `https` used internals of `http` module, thus we don't patch it.
// If we do, we'd get double breadcrumbs and double spans for `https` calls.
// It has been changed in Node 9, so for all versions equal and above, we patch `https` separately.
if (NODE_VERSION.major && NODE_VERSION.major > 8) {
const httpsModule = require('https');
fill(httpsModule, 'get', wrappedHandlerMaker);
fill(httpsModule, 'request', wrappedHandlerMaker);
}
}
but I'm sure you've seen it :)

@kamilogorek kamilogorek enabled auto-merge (squash) September 2, 2021 10:29
@kamilogorek kamilogorek merged commit 88b96f6 into getsentry:master Sep 2, 2021
@thomaspurchas
Copy link
Contributor Author

Thanks for getting that merged @kamilogorek, do you have any idea when a new release is gonna get published on npm? Keen to get this patch into production.

@kamilogorek
Copy link
Contributor

We have a beta release that we test now, so I'd assume it should be out early next week. Most likely Monday. I'll talk with the team today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node HTTP integration missing protocol in breadcrumbs
3 participants