feat(node): Avoid OTEL instrumentation for outgoing requests on Node 22+#17355
feat(node): Avoid OTEL instrumentation for outgoing requests on Node 22+#17355
Conversation
size-limit report 📦
|
mydea
force-pushed
the
fn/avoid-otel-http-instrumentation
branch
from
67942ee to
b8b33f4
Compare
andreiborza
force-pushed
the
fn/avoid-otel-http-instrumentation
branch
2 times, most recently
from
a66e597 to
002fb1e
Compare
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
andreiborza
force-pushed
the
fn/avoid-otel-http-instrumentation
branch
2 times, most recently
from
dcb74c7 to
4023787
Compare
Registers diagnostics channels for outgoing requests on Node >= 22 that takes care of creating spans, rather than relying on OTEL instrumentation.
andreiborza
force-pushed
the
fn/avoid-otel-http-instrumentation
branch
from
4023787 to
1dad574
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| * This is a feature flag that should be enabled by SDKs when the runtime supports it (Node 22+). | ||
| * Individual users should not need to configure this directly. |
There was a problem hiding this comment.
This comment sound very directed to us as SDK maintainers. As this comment is public-facing I would write that a bit differently as it can be confusing how to act on this as a user.
There was a problem hiding this comment.
Do you have a suggestion? The comment already calls out individual users should not set this.
| * | ||
| * @default `true` | ||
| */ | ||
| spans?: boolean; |
There was a problem hiding this comment.
Q: I'm wondering why we need this second option...would someone ever want to set this to false?
There was a problem hiding this comment.
Users with custom OTel setups that add @opentelemetry/instrumentation-http will want to set this, see: https://docs.sentry.io/platforms/javascript/guides/node/opentelemetry/custom-setup/#custom-http-instrumentation
| // In this case, `http.client.response.finish` is not triggered | ||
| subscribe('http.client.request.error', onHttpClientRequestError); | ||
|
|
||
| if (this.getConfig().createSpansForOutgoingRequests) { |
There was a problem hiding this comment.
With the span option, we should also check for this.getConfig().spans (if I understood that correctly).
There was a problem hiding this comment.
This is checked in the handler itself but I agree, we can check this earlier. Currently it can be misleading to set spans: false and then still get the Handling started outgoing request log.
Codecov Results 📊Generated by Codecov Action |
Codecov Results 📊✅ 23 passed | ⏭️ 7 skipped | Total: 30 | Pass Rate: 76.67% | Execution Time: 10.18s All tests are passing successfully. Generated by Codecov Action |
packages/node-core/src/integrations/http/SentryHttpInstrumentation.ts
Outdated
Show resolved
Hide resolved
packages/node-core/src/integrations/http/SentryHttpInstrumentation.ts
Outdated
Show resolved
Hide resolved
| propagateTraceInOutgoingRequests: !useOtelHttpInstrumentation, | ||
| propagateTraceInOutgoingRequests: FULLY_SUPPORTS_HTTP_DIAGNOSTICS_CHANNEL || !useOtelHttpInstrumentation, | ||
| createSpansForOutgoingRequests: FULLY_SUPPORTS_HTTP_DIAGNOSTICS_CHANNEL, | ||
| spans: options.spans, |
There was a problem hiding this comment.
Span creation overhead when tracing is disabled on Node 22.12+
Medium Severity
On Node 22.12+ when tracing is globally disabled (no tracesSampleRate/tracesSampler), createSpansForOutgoingRequests is unconditionally true and spans is passed as undefined (defaulting to true). This causes _startSpanForOutgoingRequest to run for every outgoing HTTP request, creating non-recording spans and setting up a Proxy on request.once, registering multiple event listeners, and computing span data — all of which is wasted work. Before this change, OTEL instrumentation was explicitly not set up in this scenario, resulting in zero span-creation overhead. The spans option doesn't account for hasSpansEnabled(clientOptions), so error-only SDK users on Node 22.12+ get unnecessary per-request overhead.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
There was a problem hiding this comment.
This is checked within the instrumentation and handled there.
| breadcrumbs: options.breadcrumbs, | ||
| propagateTraceInOutgoingRequests: !useOtelHttpInstrumentation, | ||
| propagateTraceInOutgoingRequests: FULLY_SUPPORTS_HTTP_DIAGNOSTICS_CHANNEL || !useOtelHttpInstrumentation, | ||
| createSpansForOutgoingRequests: FULLY_SUPPORTS_HTTP_DIAGNOSTICS_CHANNEL, |
There was a problem hiding this comment.
Missing integration test for new diagnostics channel spans
Low Severity
This is a feat PR that introduces diagnostics-channel-based outgoing request span creation, but the diff does not include a new integration or E2E test that specifically exercises this code path. The existing http-basic integration test may cover it implicitly on Node 22.12+ CI runners, but there's no test that explicitly verifies the createSpansForOutgoingRequests / diagnostics channel flow or differentiates it from the OTEL-based flow. Adding a targeted integration test would guard against regressions in this specific feature.
Triggered by project rule: PR Review Guidelines for Cursor Bot
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ignoreOutgoingRequests: options.ignoreOutgoingRequests, | ||
| outgoingRequestHook: (span, request) => { | ||
| // Sanitize data URLs to prevent long base64 strings in span attributes | ||
| const url = getRequestUrl(request); |
There was a problem hiding this comment.
Bug: The outgoingRequestHook incorrectly calls getRequestUrl with a ClientRequest object, but the function expects RequestOptions, likely causing a runtime error.
Severity: HIGH
Suggested Fix
The getClientRequestUrl function, which correctly handles ClientRequest objects, should be used instead of getRequestUrl. This requires exporting getClientRequestUrl from @sentry/node-core so it can be imported and used in packages/node/src/integrations/http.ts.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/node/src/integrations/http.ts#L262
Potential issue: In `packages/node/src/integrations/http.ts`, the `outgoingRequestHook`
receives a `http.ClientRequest` object but passes it to `getRequestUrl`. This function
is designed to accept `RequestOptions`, not `ClientRequest`. This type mismatch will
likely lead to a runtime error when `getRequestUrl` attempts to access properties that
do not exist on the `ClientRequest` object, such as `requestOptions.hostname`. A similar
incorrect call is also present in the regular OTEL HTTP instrumentation's `requestHook`.
Did we get this right? 👍 / 👎 to inform future reviews.
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.
| */ | ||
| private _onOutgoingRequestCreated(request: http.ClientRequest): void { | ||
| DEBUG_BUILD && debug.log(INSTRUMENTATION_NAME, 'Handling outgoing request created'); | ||
|
|
There was a problem hiding this comment.
Ignored outgoing requests lose trace propagation on Node 22.12+
Medium Severity
On Node 22.12+, _onOutgoingRequestCreated returns early when _shouldIgnoreOutgoingRequest is true, which skips both span creation AND trace propagation. Previously, OTEL's HttpInstrumentation handled outgoing propagation independently—its ignoreOutgoingRequestHook only suppressed span creation while still propagating trace context. Now that disableOutgoingRequestInstrumentation: FULLY_SUPPORTS_HTTP_DIAGNOSTICS_CHANNEL disables OTEL's outgoing handling entirely, requests matching ignoreOutgoingRequests no longer receive sentry-trace/baggage headers. This is a behavioral regression for users who ignore spans/breadcrumbs for certain URLs but still expect distributed tracing to work for those endpoints.
Additional Locations (1)
4 participants
Registers diagnostics channels for outgoing requests on Node >= 22 that takes
care of creating spans, rather than relying on OTEL instrumentation.
Closes #18497 (added automatically)