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 📊✅ 27 passed | Total: 27 | Pass Rate: 100% | Execution Time: 11.46s All tests are passing successfully. 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
| 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}
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.
The assertion that both outgoing requests share the same trace ID only holds on Node 22+ (diagnostics channel path). On Node <22, OTEL creates separate spans per request, each with their own trace ID.
| if (typeof contentLengthHeader !== 'string') { | ||
| return contentLengthHeader; | ||
| } |
There was a problem hiding this comment.
Bug: The getContentLength function incorrectly uses OutgoingHttpHeaders instead of IncomingHttpHeaders, causing it to mishandle string[] header values and violate its return type.
Severity: MEDIUM
Suggested Fix
Update the getContentLength function signature to accept IncomingHttpHeaders. Add a check to handle cases where the content-length header is not a string (e.g., it's an array or undefined) by returning null or undefined, ensuring only a parsed number is returned if the header is a valid string.
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-core/src/integrations/http/SentryHttpInstrumentation.ts#L504-L506
Potential issue: The `getContentLength` function is declared to accept
`http.OutgoingHttpHeaders` but is called with `response.headers`, which are of type
`IncomingHttpHeaders`. The `IncomingHttpHeaders` type allows for header values to be a
`string[]`, but the function's logic does not handle this case. If the `content-length`
header is an array, the function returns it directly, violating its declared return type
of `number | undefined`. This results in setting span attributes with an array instead
of a numeric value, leading to incorrect telemetry data.
There was a problem hiding this comment.
I guess... adapted to accept both incoming and outgoing headers and treat them accordingly.
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)