Uh oh!
There was an error while loading. Please reload this page.
ref(node): Refactor http instrumentation away from OTEL - #21974
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
| * exactly once per request, including for reused keep-alive sockets and | ||
| * `agent: false` requests. | ||
| * | ||
| * `https` requests reuse `http`'s `ClientRequest`, so patching `http` covers |
Uh oh!
There was an error while loading. Please reload this page.
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).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /** 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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 🥇
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: isaacs <i@izs.me>
Co-authored-by: isaacs <i@izs.me> Co-authored-by: Francesco Gringl-Novy <francesconovy@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.

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