Repository navigation
ref(nitro): Move channel-based instrumentation into @sentry/server-utils - #24861
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
e8a310d to
810fa6c
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| { exportName: 'redisIntegration', modules: ['redis', '@redis/client', 'ioredis'] }, | ||
| { exportName: 'dataloaderIntegration', modules: ['dataloader'] }, | ||
| { exportName: 'nitroIntegration', modules: ['h3'] }, | ||
| { exportName: 'nitroServerTimingIntegration', modules: ['unstorage'] }, |
There was a problem hiding this comment.
Bug: The nitroServerTimingIntegration may silently fail to register on Cloudflare if unstorage is tree-shaken from a minimal Nitro app, breaking trace propagation.
Severity: MEDIUM
Suggested Fix
To ensure robust registration, the module dependency for nitroServerTimingIntegration should be changed from ['unstorage'] to ['h3']. Since h3 is a guaranteed dependency in any Nitro application, this change ensures the integration is always registered when needed. The configuration should be updated to { exportName: 'nitroServerTimingIntegration', modules: ['h3'] }.
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/server-utils/src/orchestrion/config/channel-integration-definitions.ts#L64
Potential issue: The registration of `nitroServerTimingIntegration` for Cloudflare
Workers is configured to trigger upon detection of the `unstorage` module. This relies
on the assumption that `unstorage` is a core, non-tree-shakable dependency in all Nitro
applications. However, if a minimal Nitro application can be built without `unstorage`,
the integration will silently fail to register. This would result in the `sentry-trace`
and `baggage` headers not being set, breaking distributed tracing for affected
applications without any error or warning.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
I added a fix for this in a separate PR on this stack here: #24897
260b8fd to
a2ec8d9
Compare
logaretm
left a comment
logaretm
left a comment
There was a problem hiding this comment.
Should we merge both nitro integrations into one integration that enables either instrumentation with options?
nitroIntegration({
// defaults, or flip em to `disabeXXX`
serverTiming: true,
storageInstrumentation: true,
// other instrumentations
})
d93c0f9 to
a6ecc88
Compare
Yea, that was never the intention of them I think. This happened as a side effect of other changes. I think rarely anyone will use one without the other. |
| const span = startInactiveSpan({ | ||
| name: isUnparameterizedStreamedServerSpan |
There was a problem hiding this comment.
This PR moves code, so this is not blocking. But nitroIntegration is now a default in every Node-based SDK.
httpServerSpansIntegration from our Node package already creates the http.server root span. srvx.request adds a second http.server child span, and h3.request adds a third. Maybe you can check what happens there.
Suggestion: if an http.server span is already active, don't start a new one, but set the route/status/etc. on that span.
There was a problem hiding this comment.
FWIW I also had this with Next.js on Cloudflare (there would have been 3 http.server spans). I solved it by simply ignoring the specific spans:
sentry-javascript/packages/nextjs/src/cloudflare/index.ts
Lines 71 to 78 in 01935a5
Not sure if this would be applicable here too.
There was a problem hiding this comment.
#25295 --> good point, added a follow up here. it does change structure though, but I think it's worth it...
| return [...getDefaultNodeIntegrations(options)]; | ||
| // `nitroIntegration` is also part of the Node tracing integrations, but add it explicitly so it is | ||
| // always present for the Nitro SDK regardless of that gating (deduped by name when both are added). | ||
| return [...getDefaultNodeIntegrations(options), nitroIntegration()]; |
There was a problem hiding this comment.
This adds nitroIntegration also when tracing is off, no? We should gate this.
There was a problem hiding this comment.
this is fine here, as we know nitro is relevant when using the nitro sdk I'd say (it is also the same behavior as before, where we always added this)?
73ba6de to
eb9579a
Compare
69a9819 to
d900bc4
Compare
Extract Nitro's diagnostics-channel instrumentation out of `@sentry/nitro` into `@sentry/server-utils`, so any server SDK can provide it: - `nitroIntegration` (h3/srvx HTTP + middleware spans, unstorage cache spans) is a tracing integration, registered via `getTracingIntegrations()` and so available in node, deno and bun. - `nitroServerTimingIntegration` sets the `Server-Timing` trace-propagation headers. It is a default (non-tracing) integration so it also runs in tracing-without-performance mode, and can be opted out separately. Error capture stays in `@sentry/nitro`'s runtime plugin (its Nitro `error` hook), which a channel-based integration cannot replace without losing coverage. `@sentry/nitro` keeps getting all of this through the underlying `@sentry/node` defaults, so nothing changes for existing Nitro-on-Node apps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…orchestrion Add registration-only orchestrion configs so the Nitro integrations are auto-registered on bundler-only SDKs like `@sentry/cloudflare`, which discover a bundled module via the module-injected event and instantiate its factory. The snippet maps one integration per module, so two anchors are used, both core Nitro dependencies evaluated at startup: `h3` (dist/h3.mjs) registers `nitroIntegration` (spans) and `unstorage` (dist/index.mjs) registers `nitroServerTimingIntegration`. These are registration-only (no channels are injected, the libraries publish their own), so they are excluded from the runtime loader and are a no-op on node/deno/bun, where the integrations are registered statically. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…trion config Document why the h3/unstorage registration-only ranges use the `-0` suffix: the orchestrion matcher is the vendored `semifies` (not node-semver), where a range carrying a prerelease tag matches prereleases across patch tuples — so `>=2.0.0-0` matches the `2.0.1-rc.*` / `2.0.0-alpha.*` versions Nitro 3 ships. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a raw Nitro 3 app deployed to Cloudflare (Workers, `cloudflare_module` preset) that exercises the orchestrion registration-only path on a bundler-only SDK: the app imports no Nitro instrumentation, and `nitroIntegration` / `nitroServerTimingIntegration` are auto-injected via the orchestrion transform over h3/unstorage and instantiated by `@sentry/cloudflare` at init. Tests assert `auto.http.nitro.*` spans, `auto.cache.nitro` cache spans, and the `Server-Timing` trace-propagation header — i.e. the whole auto-injection path, including that the tracing channels bind under Cloudflare's async-context strategy. Init mirrors what @sentry/nuxt does for Nitro-on-Cloudflare (no first-class raw-Nitro Cloudflare helper exists): a Nitro server plugin wraps `nitroApp.fetch` with `wrapRequestHandler`, and the orchestrion rollup/rolldown plugin is wired into the Nitro build config. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This reverts commit ab0b58f.
The `mod.test.ts` snapshots captured the full `sdk.integrations` list, so any change to the default integrations (such as the new nitro integrations) broke them even though the list is incidental to what these tests verify. Normalize `sdk.integrations` to a placeholder, like the other volatile fields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stacked on develop's hono size bump: the nitro integrations add on top, so @sentry/node needs 146 KB and the no-channel-injection variant 125 KB. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`nitroIntegration` is part of the Node tracing integrations, but the Nitro SDK should always include it regardless of that gating. Add it explicitly to the Nitro SDK's default integrations (deduped by name when both are added), and re-export `nitroIntegration`/`nitroServerTimingIntegration` from `@sentry/node`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nitroIntegration and nitroServerTimingIntegration are exported from @sentry/node, so every SDK with an explicit re-export list needs them too (caught by the node-exports-test-app consistency check). Deno and Bun also enable them by default via getTracingIntegrations(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Jan Peer Stöcklmair <jan.oster94@gmail.com>
Drives h3 v2, srvx and unstorage with their tracing plugins enabled, which publish the same diagnostics channels as a Nitro app, so the integrations are covered on Node without a Nitro build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ite` Bun 1.3 drops the newline of a `console.log` call when the string is exactly 4096 bytes, so an envelope of that size gets merged with the next one into a single line that the integration test runner can't parse. The runner then waits for an envelope it already received until the test times out, which made the Bun 1.3 integration tests flaky (e.g. `captureMessage/with_level`). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f7da752 to
78f185c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 78f185c. Configure here.
| setupOnce() { | ||
| captureTracingEvents(); | ||
| captureStorageEvents(); | ||
| }, |
There was a problem hiding this comment.
Span setup skips tracing-channel wait
Medium Severity
nitroIntegration calls captureTracingEvents() and captureStorageEvents() directly from setupOnce, without waitForTracingChannelBinding. Sibling native-channel integrations (dataloader, knex, native GraphQL) wait so bindTracingChannelToSpan can parent spans. If the async-context binding is not ready yet — common with a custom OpenTelemetry setup — the bind no-ops, __SENTRY_NITRO_HTTP_CHANNELS_INSTRUMENTED__ is still set, and Nitro HTTP/cache spans are never created. The old runtime plugin ran after init(), so this race is new.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 78f185c. Configure here.
|
|
||
| diagnosticsChannel.tracingChannel<H3TracingRequestEvent>('h3.request').start.subscribe(message => { | ||
| setServerTimingHeaders((message as H3TracingRequestEvent).event); | ||
| }); |
There was a problem hiding this comment.
Server-Timing subscriber can throw into h3
Low Severity
The h3.request start subscriber calls setServerTimingHeaders with no safeChannelCallback (or other try/catch). A throw from getTraceData(), a missing event, or headers.append on immutable headers propagates through diagnostics_channel into h3’s request path and can fail the user request. This integration is now a default on every Node-based SDK, so the blast radius is larger than the old Nitro plugin.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 78f185c. Configure here.
…tion` (#25349) Stacked on #24861. Replaces #24897. `nitroServerTimingIntegration` only existed as a separate integration because `nitroIntegration` was tracing-gated, while Server-Timing trace propagation must work in tracing-without-performance mode. That split meant Cloudflare auto-injection needed a second anchor module (`unstorage`), since a module registers a single integration, and a Nitro app that never loaded `unstorage` would silently lose the headers. #24897 tried to fix that by letting one module register several integrations, which complicated the global marker shape. Instead, there is now a single `nitroIntegration` with options to opt out of parts of it, both enabled by default: ```ts nitroIntegration({ serverTiming: false, // no Server-Timing sentry-trace/baggage headers storage: false, // no unstorage cache spans }); ``` It's registered as a default integration rather than a tracing one, like express/hono/koa: without tracing, the spans it starts are non-recording, and the Server-Timing headers still propagate. It's anchored on `h3` alone, which every HTTP-serving Nitro app loads, so the marker keeps its single-factory shape. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
4 participants
Moves Nitro's
node:diagnostics_channel-based instrumentation out of@sentry/nitroand into@sentry/server-utils, so it is no longer tied to the Nitro package and can be provided by any server SDK. This mirrors the Hono → server-utils move.The runtime instrumentation splits into two integrations:
nitroIntegration— h3/srvx HTTP + middleware spans and unstorage cache spans. It only creates spans, so it is a tracing integration, registered viagetTracingIntegrations(). That makes it available (and on by default) in node, deno and bun.nitroServerTimingIntegration— sets theServer-Timingresponse headers that carrysentry-trace/baggageso the browser SDK can connect a pageload to the backend trace. This must work even when tracing is disabled (trace-without-performance), so it is a default (non-tracing) integration, registered viagetErrorIntegrations(). It is opt-out-able independently.Both integrations subscribe to h3's tracing channels, so they are inert unless a Nitro app (the only real emitter of those channels) is running.
Cloudflare (and other bundler-only SDKs) get both integrations auto-injected via orchestrion registration-only configs: h3 and unstorage are transformed at build time to register the matching factory, which
@sentry/cloudflareinstantiates atinit(). These are registration-only (no channels injected — the libraries publish their own), so they are excluded from the runtime loader and are a no-op on node/deno/bun, where the integrations are registered statically.Non-breaking:
@sentry/nitro'sinit()goes through@sentry/node's default integrations, which now include both of the above, so existing Nitro-on-Node apps get exactly the same spans and headers as before — just sourced from the integrations rather than the runtime plugin.@sentry/nitrono longer depends on@sentry/server-utils.Why error capture stays in
@sentry/nitroError capture is intentionally not moved. Nitro's
errorhook is a broad boundary (request/response-hook, plugin-init, cache and process-levelunhandled*errors, pluscontexts.nitrotags/method/path). The tracing channels only see errors thrown through a traced handler, so folding error capture into a channel-based integration would lose coverage and force an h3 coupling into the framework-neutral package. It remains in the Nitro runtime plugin'serrorhook.E2E coverage
A new
cloudflare-nitroe2e app (raw Nitro 3 on thecloudflare_modulepreset) exercises the Cloudflare auto-injection end to end — assertingauto.http.nitro.*spans,auto.cache.nitrocache spans, and theServer-Timingheader, which also confirms the tracing channels bind under Cloudflare's async-context strategy. Since raw Nitro has no first-class Cloudflare init helper yet, the app wires init by hand (a Nitro server plugin wrappingnitroApp.fetchwithwrapRequestHandler, mirroring@sentry/nuxt); a supported helper could be a good follow-up.🤖 Generated with Claude Code