ref(node-core): Move node fetch instrumentation into node-core#21873
Conversation
Move the `undici-instrumentation` (spans + breadcrumbs + trace propagation) and the vendored `node-fetch` types from the `node` package into `node-core`, and expose `instrumentUndici` / `NodeFetchOptions` from `@sentry/node-core`. `node-core`'s `nativeNodeFetchIntegration` now instruments via `instrumentUndici` directly (spans default to `true`). The `node` package keeps a thin `nativeNodeFetchIntegration` wrapper (`node-fetch.ts`) that derives the `spans` default from the client options (`skipOpenTelemetrySetup` / `hasSpansEnabled`) before delegating to `instrumentUndici`. `SentryNodeFetchInstrumentation` is now unused internally and is marked `@deprecated`; it stays exported for backwards compatibility and will be removed in a future major version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
requested review from
JPeer264 and
andreiborza
and removed request for
a team
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| SentryNodeFetchInstrumentation, | ||
| type SentryNodeFetchInstrumentationOptions, | ||
| } from './integrations/node-fetch/SentryNodeFetchInstrumentation'; | ||
| export { addFetchRequestBreadcrumb, addTracePropagationHeadersToFetchRequest } from './utils/outgoingFetchRequest'; |
There was a problem hiding this comment.
Removed public fetch helper exports
Medium Severity
This change drops addFetchRequestBreadcrumb and addTracePropagationHeadersToFetchRequest from the @sentry/node-core package entry without a deprecation period, which is a breaking public API removal for anyone importing those helpers from the main export.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit d4e9512. Configure here.
There was a problem hiding this comment.
this was just added in the base-stack PR so should be fine
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit db92a29. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // When spans are disabled we do not create a span, but we still inject trace propagation headers | ||
| // directly (unless disabled via `tracePropagation`). | ||
| if (config.spans === false) { | ||
| if (!config.spans) { |
There was a problem hiding this comment.
Bug: The check !config.spans incorrectly disables span creation when the spans option is undefined, which is the default for nativeNodeFetchIntegration().
Severity: HIGH
Suggested Fix
Revert the condition at line 193 from !config.spans to config.spans === false. This will correctly handle the undefined case as not explicitly disabled, aligning with the documented default behavior of creating spans.
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/node-fetch/undici-instrumentation.ts#L193
Potential issue: The condition to check if spans should be created for `undici`
instrumentation was changed from `config.spans === false` to `!config.spans`. When
`nativeNodeFetchIntegration()` is called without options, as is common in packages like
`@sentry/bun` and `@sentry/aws-serverless`, the `spans` option is `undefined`. The new
check `!undefined` evaluates to `true`, causing the function to return early and skip
the creation of fetch spans. This contradicts the documentation, which states that spans
are enabled by default, and will break distributed tracing for outgoing HTTP requests in
multiple packages.
Also affects:
packages/bun/src/sdk.ts:43packages/node-core/src/sdk/index.ts:63dev-packages/e2e-tests/test-applications/node-express/src/app.ts:18
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
If I'm not mistaken it has nothing to do with the nativeNodeFetchIntegration, the function just has the same name
Edit: Ok, I just checked (I just got confused with another with the same name) - this shouldn't be a problem, as spans should always be set by the in the node-fetch.ts and it will always be a boolean.
But I do wonder if there are differences now between nativeNodeFetchIntegration from node-core/light - but I think it is the plan to get rid of that if I understand #21778 correctly
There was a problem hiding this comment.
There was a problem hiding this comment.
yeah, this is on purpose and is this way to remain the behavior we have today - which is that in node-core we do not emit spans for this by default. So in node, we default this to true (same as before) generally speaking, and in node-core it is always default false, but users can now opt-in to it if they want (theoretically)
size-limit report 📦
|
| const clientOptions = getClient()?.getOptions(); | ||
| instrumentUndici({ | ||
| ...options, | ||
| spans: _shouldInstrumentSpans(options, clientOptions), |
There was a problem hiding this comment.
https://github.com/getsentry/sentry-javascript/pull/21873/changes#r3504416417
spans is always a boolean
There was a problem hiding this comment.
yes, in normal node setup we want to generally enable this, while in node-core it should always be opt-in to maintain the current behavior (where spans are never emitted)
…mentation (#21872) Enhances the node `undici-instrumentation` into a single instrumentation that emits `http.client` spans, records breadcrumbs, and propagates traces for outgoing fetch/undici requests. Previously this was split across two subscribers on the same diagnostics channels: `instrumentUndici` (spans) and `SentryNodeFetchInstrumentation` (breadcrumbs + trace propagation). What changes: - Span creation is gated by a `spans` option (default `true`). When spans are disabled, breadcrumbs are still recorded and trace propagation headers are still injected (gated by `tracePropagation`). - Trace propagation now goes through the dedup-aware `addTracePropagationHeadersToFetchRequest`, which gains an optional `span` argument so outgoing headers reference the `http.client` span while still de-duplicating against trace headers the user set manually (e.g. via `getTraceData()`). This preserves the double-baggage prevention that the two-instrumentation setup previously provided. - `@sentry/node-core` now exports `addFetchRequestBreadcrumb` and `addTracePropagationHeadersToFetchRequest` so the node instrumentation can reuse them. _Root cause:_ folding the two subscribers into one dropped the de-dup pass that ran after span-based header injection; routing propagation through `addTracePropagationHeadersToFetchRequest` (now span-aware) restores it. This PR is intentionally scoped to the `node` package (plus the minimal `node-core` support above) to keep the diff reviewable. A stacked follow-up (#21873) moves the instrumentation into `node-core`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Stacked on #21872.
Moves the
undici-instrumentation(spans + breadcrumbs + trace propagation) and the vendorednode-fetchtypes from thenodepackage intonode-core, and exposesinstrumentUndici/NodeFetchOptionsfrom@sentry/node-core.What changes:
node-core'snativeNodeFetchIntegrationnow instruments viainstrumentUndicidirectly (spans default totrue).nodepackage keeps a thinnativeNodeFetchIntegrationwrapper (node-fetch.ts) that derives thespansdefault from the client options (skipOpenTelemetrySetup/hasSpansEnabled) before delegating toinstrumentUndici. In v11 this will become the only implementation.SentryNodeFetchInstrumentationis now unused internally and is marked@deprecated. It stays exported for backwards compatibility and will be removed in a future major version.Review the first PR in the stack (#21872) first — this PR is purely the relocation.
🤖 Generated with Claude Code