feat(http): refactor node:http client instrumentation for portability#20393
feat(http): refactor node:http client instrumentation for portability#20393
Conversation
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
1f9cef2 to
5ab332a
Compare
size-limit report 📦
|
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
3 times, most recently
from
18b93f6 to
fd04ed5
Compare
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
0877c6e to
1963717
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
1963717 to
3e7001c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
0307e3a to
640f8f0
Compare
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
1b8377c to
1a8d51e
Compare
…ttp client instrumentation (#20393)
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
953b012 to
1ef480a
Compare
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
1ef480a to
13607f2
Compare
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
13607f2 to
d8d225c
Compare
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
d8d225c to
8ad3dd0
Compare
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Overall it looks great. I just want to clarify the OTel double instrumentation before approving (it might not be an issue but I want to be sure)
| // if we do not have earlier access to the request object at creation | ||
| // time. The http.client.request.error channel is only available on | ||
| // the same node versions as client.request.created, so no help. | ||
| if (_options.breadcrumbs && !FULLY_SUPPORTS_HTTP_DIAGNOSTICS_CHANNEL) { |
There was a problem hiding this comment.
q: Do I understand it correctly, that http.client.request.error has been removed, because http.client.response.finish was here first, so http.client.request.error is kinda useless in our case, as finish did the trick already?!
If so, I wonder what would happen on newer Node versions where http.client.response.finish is not getting triggered - I think I'm missing something
There was a problem hiding this comment.
Ok, I think you're actually noticing a valid point here. (It's very confusing!)
So, the flow is:
- create ClientRequest object
- fire
http.client.request.created(node >=22.12 only) - pass object to user
- user does things, calls
.end()at some point. - Headers sent!
- fire
http.client.request.start(all node versions) - response arrives from server
- user consumes response
- fire
http.client.response.finish(all node versions)
("All node versions" means >=18.6, so, all that we support anyway.)
If at some point, the request has an error, it'll fire http.client.request.error, but only on node >=22.6.0 || 20 >=20.17.0.
On node 22 >=22.12.0 || 23 >=23.2.0 || >=24 we have the http.client.request.created, and we attach an errorMonitor to the request object, so there's no need for http.client.request.error on those versions.
On nodes that don't support http.client.request.created, if breadcrumbs are enabled, we generate breadcrumbs on the http.client.response.finish. (This doesn't matter if we attached to request.created, because we do the errorMonitor and prependListener('response') listeners early enough.)
However, the valid point is that the response is not guaranteed, if the request has a socket hangup error or something before the response arrives.
I think the solution is that for old nodes, we attach a listener on the http.client.request.start diagnostics channel. This gets access to the request object, albeit too late to set headers, so no trace propagation. But it can attach the error listener to do breadcrumbs without a response. This covers more node versions than the http.client.request.error channel, and we don't need to have another version range to be checking.
There was a problem hiding this comment.
On newer node versions, we're relying on the fact that the request.created channel listener attaches a response event handler, and then we deal with the response at that point.
| * Inject Sentry trace-propagation headers into an outgoing request if the | ||
| * target URL matches the configured `tracePropagationTargets`. | ||
| */ | ||
| export function injectTracePropagationHeaders( |
There was a problem hiding this comment.
l: There is already the _INTERNAL_getTracingHeadersForFetchRequest which is quite similar, with the difference that it doesn't apply the headers, but returns them. Theoretically we can try to reuse it, but not really mandatory for this PR.
| @@ -0,0 +1,28 @@ | |||
| import { objectToBaggageHeader, parseBaggageHeader } from '../../utils/baggage'; | |||
|
|
|||
| // TODO: should this be in utils/baggage? | |||
There was a problem hiding this comment.
l: Yeah good idea, it might make sense there, as these utilities there are quite similar. I would then suggest to move mergeBaggage there and maybe call it mergeBaggageHeader?!
| export function getOpenTelemetryInstrumentationToPreload(): (((options?: any) => void) & { id: string })[] { | ||
| return [ | ||
| instrumentSentryHttp, | ||
| instrumentOtelHttp, |
There was a problem hiding this comment.
q/l: Would that mean we can remove the @opentelemetry/instrumentation-http dependency as well?
There was a problem hiding this comment.
Ah, yes, I had thought we needed that for the Server bits for now, but in fact we do not. Good catch!
| export function getOpenTelemetryInstrumentationToPreload(): (((options?: any) => void) & { id: string })[] { | ||
| return [ | ||
| instrumentSentryHttp, | ||
| instrumentOtelHttp, |
There was a problem hiding this comment.
q/m: Do you know what will happen for users that already have an OTel setup? Usually the HTTP instrumentation is on by default when doing auto instrumentation. Before it wouldn't do double instrumentation, as we already registered it, but now we removed this and OTel can now instrument it.
There was a problem hiding this comment.
This is a good point! Also, it's going to affect any instrumentations that we migrate from OTel to Sentry for use in other platforms, right?
This does add to the argument in favor of changing the span origin from auto.http.otel.http to auto.http.client (similarly for the express instrumentation). So, if users do start getting duplicate instrumentations, they'll know why.
I'll try to set up a demo app with this scenario, and see if there's a way we can detect the situation or somehow register with OTel in that case.
There was a problem hiding this comment.
this is a good point! IMHO it is not a blocker if we can't get this to work. but maybe we can find a way to at least warn if we detect this somehow… but this may be hard due to timing, as we may be the first or last to wrap. ideal world, if possible:
- warn if we wrap and detect that otel has already wrapped it
- mark our wrapped stuff in a way that otel will also detect it as wrapped (??? not sure if this is feasible and/or has other problems attached to it…)
There was a problem hiding this comment.
Also, it's going to affect any instrumentations that we migrate from OTel to Sentry for use in other platforms, right?
I think so yes.
This does add to the argument in favor of changing the span origin from auto.http.otel.http to auto.http.client (similarly for the express instrumentation). So, if users do start getting duplicate instrumentations, they'll know why.
Fair point. Maybe we can even set up a document somewhere where we basically have potential double instrumentation and would recommend to not do autoinstrumentation but allow list these instrumentations. Some "best practice" guide or something.
mark our wrapped stuff in a way that otel will also detect it as wrapped
Could work potentially - timing is an issue as you mentioned.
| }); | ||
|
|
||
| request.prependListener('response', response => { | ||
| // no longer need this, listen on response now |
There was a problem hiding this comment.
to clarify, if this is prepended then it will def. be called and close can no longer be called?
There was a problem hiding this comment.
Well, it's more that once we have a response, the response will either emit end or error at some point, and the breadcrumbs will be emitted at that point. The value of removing the listener is somewhat debatable, though, now that I look at it. It might actually be preferable from a perf point of view to just leave it there, since it's not hurting anyone, and removing it is slightly more work than leaving it in place. 🤔
| import type { HttpClientRequest } from './types'; | ||
|
|
||
| /** | ||
| * Inject Sentry trace-propagation headers into an outgoing request if the |
There was a problem hiding this comment.
let's mention here somewhere that this has to be called before the request is started, for future reference?
mydea
left a comment
mydea
left a comment
There was a problem hiding this comment.
scanned it a bit and it generally looks sweet to me, great work! I'd ask @JPeer264 , @andreiborza and/or @nicohrubec to take some time to review this as it would be ideal to have broader understanding in the team about how these things work together, possibly worth it to do a session to sit together and explain the changes etc!
This was implemented for the portable Express integration, but others will need the same functionality, so make it a reusable util.
Refactor the `node:http` outgoing request instrumentation so that it can be applied to non-Node.js environments by patching the http module. Also, refactor so that the diagnostics_channel and monkeypatching paths can share code, and so that light and normal node-core instrumentations can share more of the functionality as well. To facilitate this, some portable minimal types are vendored in from the `node:http` module.
isaacs
force-pushed
the
isaacschlueter/portable-http-integration-client
branch
from
8ad3dd0 to
c5a2b94
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c5a2b94. Configure here.
| propagateTrace: options.propagateTraceInOutgoingRequests, | ||
| applyCustomAttributesOnSpan, | ||
| ...options, | ||
| spans: options.createSpansForOutgoingRequests ?? options.spans, |
There was a problem hiding this comment.
Node-core unintentionally creates outgoing HTTP request spans
High Severity
The node-core httpIntegration never sets createSpansForOutgoingRequests or spans in its SentryHttpInstrumentationOptions. After the refactor, patchOptions.spans resolves to undefined ?? undefined, which causes getHttpClientSubscriptions to fall back to hasSpansEnabled(clientOptions). When tracing is enabled, this now creates outgoing request spans — a behavior that never existed before in node-core. The old code used createSpansForOutgoingRequests && (spans ?? true), which was always falsy when createSpansForOutgoingRequests was unset.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c5a2b94. Configure here.
3 participants
Refactor the
node:httpoutgoing request instrumentation so that it canbe applied to non-Node.js environments by patching the http module.
Also, refactor so that the diagnostics_channel and monkeypatching paths
can share code, and so that light and normal node-core instrumentations
can share more of the functionality as well.
To facilitate this, some portable minimal types are vendored in from the
node:httpmodule.yarn lint) & (yarn test).Notes on test changes:
conditionalTeston a node version.originspan data changes from the vague (and now incorrect)auto.http.otel.httptoauto.http.clientfor client spans. This also affects a lot of the tests.Otherwise, all prior behavior should be unchanged, which is reflected in the integration and unit tests.