ref(node): Refactor http instrumentation away from OTEL#21974
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
mydea
requested review from
JPeer264,
andreiborza,
isaacs and
timfish
and removed request for
a team
| * exactly once per request, including for reused keep-alive sockets and | ||
| * `agent: false` requests. | ||
| * | ||
| * `https` requests reuse `http`'s `ClientRequest`, so patching `http` covers |
There was a problem hiding this comment.
Oh this is a nice bonus!
There was a problem hiding this comment.
This is a huge improvement! Very good direction found by the clanker ;)
However, onSocket doesn't play nice with http.Agent if sockets are being managed. I suggested two other approaches we could use (either patch all 3 of the header-sending public methods, or the single internal _storeHeader method).
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
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 60a3f07. Configure here.
| /** Creates a test server that can be used to check headers */ | ||
| // eslint-disable-next-line @typescript-eslint/explicit-function-return-type | ||
| export function createTestServer() { | ||
| export function createTestServer(): TestServer { |
There was a problem hiding this comment.
q: Is this part of the PR? Pretty sure it is, just wondering why we didn't need it before. And if we need it we can remove the disable-next-line above, unfortunately oxlint, doesn't complain if the rule doesn't apply 😢
There was a problem hiding this comment.
only tangentially, noticed that this lead to squiggly lines in tests that I touched here - imho something seems not properly set up for node integration tests, need to look at this in a follow up :) and will remove the ignore line!
| */ | ||
| export const httpIntegration = defineIntegration((options: HttpOptions = {}) => { | ||
| // In node-core, for now we disable incoming requests spans by default | ||
| // we may revisit this in a future release |
There was a problem hiding this comment.
q: Because this got moved, should we add a v11 todo? As node-core will be merged in v11 this might be a good time to revisit already
There was a problem hiding this comment.
I think no need for a todo comment here specifically, because we'll need to revisit this anyhow because node-core will go away so all of this will necessarily receive attention :D
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Some nits on the JSDoc comments, just combing through the deprecated stuff, but all the functional problems seem to be addressed 🥇
Co-authored-by: isaacs <i@izs.me>
Co-authored-by: isaacs <i@izs.me> Co-authored-by: Francesco Gringl-Novy <francesconovy@gmail.com>
4 participants
Refactors the outgoing HTTP instrumentation so it no longer relies on OpenTelemetry's
InstrumentationBase/InstrumentationNodeModuleDefinitionmachinery. Outgoing requests (breadcrumbs, spans, and trace-header propagation) are now handled directly by Sentry code, using Node'snode:httpclient diagnostics channel where available and falling back to monkey-patching on older runtimes.What changed
node-core:SentryHttpInstrumentation→instrumentHttpOutgoingRequests(). The core logic is now a plain function instead of an OTEL instrumentation class. On Node 22.12+ (and 23.2+/24+) it subscribes to thenode:httpclient diagnostics channel — and is safely re-subscribable, so calling it again swaps in the latest options. On older runtimes it falls back to monkey-patching. TheSentryHttpInstrumentationclass is kept as a thin, deprecated wrapper for backwards compatibility.core: new monkey-patch strategy — patchClientRequest.prototype.onSocketinstead of the module'srequest/getexports. Every outgoing request ultimately constructs aClientRequestand callsonSocketon the shared prototype, so patching it reaches consumers regardless of how the module was imported — including immutable ESM namespace bindings (import * as http/import { request }) that can't be monkey-patched otherwise.onSocketruns synchronously during request setup (before headers flush, in the caller's async context), so trace headers are still injectable and spans are parented correctly.httpsreuseshttp'sClientRequest, so a single patch covers both, and inherited-prototype double-patching is guarded against. Instrumentation errors are swallowed so they can never break the underlying request.core:HttpExporttype reshaped to expose theClientRequestconstructor (HttpClientRequestConstructor) rather thanrequest/get.node&node-corehttpIntegrationwiring now pointsinstrumentSentryHttpatinstrumentHttpOutgoingRequestsdirectly (viaObject.assign) instead ofgenerateInstrumentOnce+new SentryHttpInstrumentation.instrumentHttpOutgoingRequestsis now exported fromnode-core.Docs: clarified the
OutgoingHttpRequestInstrumentationOptionsoption comments to match actual behavior — notably thatignoreOutgoingRequestsskips the request entirely (no breadcrumb, no span, no trace propagation), and that the outgoing hooks only fire when spans are actually created.Tests / test-utils: updated
client-patchtests for the new strategy and gavecreateTestServeran explicitTestServerreturn type.Closes #20902