Uh oh!
There was an error while loading. Please reload this page.
ref(node): Streamline undici (node-fetch) instrumentation - #21650
Conversation
Refactor the vendored `UndiciInstrumentation` (the span-emitting half of the
NodeFetch integration) to use Sentry's span APIs instead of OpenTelemetry's
tracing/propagation APIs.
- Span creation: `tracer.startSpan` -> `startInactiveSpan({ kind: CLIENT })`
(the span is created on `undici:request:create` and ended later in
`onDone`/`onError`, so it must be inactive).
- Status: `SpanStatusCode` -> `SPAN_STATUS_ERROR`; drop the no-op
`recordException`.
- Trace propagation: replace `propagation.inject(trace.setSpan(...))` with
`withActiveSpan(span, () => getTraceData(...))` gated by
`shouldPropagateTraceForUrl`. This keeps the propagated `sentry-trace`
referencing the http.client span (not the parent), matching prior output.
Passing `{ span }` to `getTraceData` alone is insufficient: for an inactive
span it resolves to the span's captured scope, whose active span is the
parent.
- Drop the OTel metrics (no MeterProvider is wired up) and the dead
`requireParentforSpans` code path (the SDK always passes `false`).
- Replace the diag logger with `debug` + `DEBUG_BUILD`, swap OTel type imports
for `@sentry/core`, inline the semantic-convention constants into a local
`semconv.ts`, and drop the blanket `eslint-disable`s.
The only remaining `@opentelemetry/api` usage is `SpanKind`; the
`@opentelemetry/instrumentation` patching base is kept.
Adds integration coverage for the error path (`internal_error` span) and for
`headersToSpanAttributes` (request + response header capture).size-limit report 📦
|
| name: requestMethod === '_OTHER' ? 'HTTP' : requestMethod, | ||
| kind: SpanKind.CLIENT, | ||
| attributes, | ||
| }); |
There was a problem hiding this comment.
Missing http.client sentry.op
High Severity
startInactiveSpan is called without sentry.op (http.client) on the span or in merged attributes. The integration tests and prior OTel export path expect outgoing fetch spans to use op http.client, but native SentrySpan JSON only reads op from attributes and does not infer it from kind or http.request.method.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 2b3dbd0. Configure here.
| name: requestMethod === '_OTHER' ? 'HTTP' : requestMethod, | ||
| kind: SpanKind.CLIENT, | ||
| attributes, | ||
| }); |
There was a problem hiding this comment.
Fetch span description missing URL
Medium Severity
Outgoing fetch spans are started with name set to the HTTP method only (e.g. GET), while integration tests expect descriptions like GET http://host/path. The OTel exporter used to derive that description from URL attributes and CLIENT kind; that inference no longer runs for these native spans.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2b3dbd0. Configure here.
| span.recordException(error); | ||
| span.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| code: SPAN_STATUS_ERROR, |
There was a problem hiding this comment.
Error status not internal_error
Medium Severity
On undici:request:error, the span status message is set to the raw error.message, so serialized status is typically the system error string rather than internal_error. The new fetch-error integration test expects status: 'internal_error', consistent with @sentry/core fetch instrumentation.
Reviewed by Cursor Bugbot for commit 2b3dbd0. Configure here.
mydea
left a comment
There was a problem hiding this comment.
this is good IMHO, for this case we can actually already de-otelify this right? And just make this a regular integration that registers the channel hooks...? but we can do this in a follow up 👍
suzunn
commented
Jun 19, 2026
I noticed the new coverage exercises the error path and header-to-attribute mapping, but the riskiest behavior in this refactor is the propagation handoff from OTel context APIs to |
logaretm
commented
Jun 19, 2026
Drop the @opentelemetry/instrumentation base from the vendored undici instrumentation. Since undici reports via diagnostics_channel rather than module patching, InstrumentationBase provided nothing here — it's now a plain class that the integration subscribes/unsubscribes directly, off OTel's registerInstrumentations path. Also drops the remaining @opentelemetry/api usage (SpanKind, inlined) and safeExecuteInTheMiddle (local helper). Pure refactor: span emission, trace propagation, and breadcrumbs are unchanged.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3962e7f. Configure here.
| if (!_undiciInstrumentation) { | ||
| _undiciInstrumentation = new UndiciInstrumentation(_getConfigWithDefaults(options)); | ||
| } | ||
| _undiciInstrumentation.enable(); |
There was a problem hiding this comment.
Undici config frozen at first init
Low Severity
UndiciInstrumentation is constructed only when _undiciInstrumentation is first created; later instrumentNodeFetchSpans(options) calls only invoke enable(). Updated integration options (hooks, header mapping, ignore callbacks) from a subsequent setup path are not applied to the singleton instance.
Reviewed by Cursor Bugbot for commit 3962e7f. Configure here.
| function instrumentNodeFetchSpans(options: NodeFetchOptions): void { | ||
| if (!_undiciInstrumentation) { | ||
| _undiciInstrumentation = new UndiciInstrumentation(_getConfigWithDefaults(options)); | ||
| } | ||
| _undiciInstrumentation.enable(); |
There was a problem hiding this comment.
Bug: The nativeNodeFetchIntegration configuration is frozen after the first initialization. Subsequent calls to Sentry.init() with different options for this integration are silently ignored.
Severity: MEDIUM
Suggested Fix
To allow for configuration updates, implement a setConfig() method on the UndiciInstrumentation class. Then, modify instrumentNodeFetchSpans to call this setConfig() method on the existing singleton instance whenever new options are provided, similar to how generateInstrumentOnce handles configuration updates.
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/node-fetch/index.ts#L60-L64
Potential issue: The `nativeNodeFetchIntegration` uses a module-level singleton
`_undiciInstrumentation` that is initialized only once. The `UndiciInstrumentation`
class does not have a method to update its configuration after instantiation.
Consequently, if `Sentry.init()` is called multiple times in the same process with
different options for this integration (e.g., different `headersToSpanAttributes`), the
configuration from the first call is permanently used, and all subsequent configuration
changes are silently ignored. This primarily affects testing scenarios or applications
that re-initialize Sentry.
Did we get this right? 👍 / 👎 to inform future reviews.
Uh oh!
There was an error while loading. Please reload this page.
Streamlines the vendored
UndiciInstrumentation(the span-emitting half of the NodeFetch integration) onto Sentry's span APIs.closes#20743