feat(nitro): Instrument HTTP Server#19225
feat(nitro): Instrument HTTP Server#19225logaretm wants to merge 23 commits intoawad/create-nitro-sdk-metadatafrom
Conversation
Codecov Results 📊Generated by Codecov Action |
Codecov Results 📊✅ 25 passed | ⏭️ 5 skipped | Total: 30 | Pass Rate: 83.33% | Execution Time: 13.58s All tests are passing successfully. Generated by Codecov Action |
size-limit report 📦
|
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.
|
logaretm
force-pushed
the
awad/js-1128-nitro-use-tracing-channels-for-srvx-and-h3
branch
2 times, most recently
from
23a8b35 to
07ad5a1
Compare
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
81b555e to
161862d
Compare
logaretm
force-pushed
the
awad/js-1128-nitro-use-tracing-channels-for-srvx-and-h3
branch
2 times, most recently
from
c3fef9e to
6799854
Compare
| import { setServerTimingHeaders } from '../hooks/setServerTimingHeaders'; | ||
|
|
||
| export default definePlugin(nitroApp => { | ||
| // FIXME: Nitro hooks are not typed it seems |
There was a problem hiding this comment.
This has been fixed in nitrojs/nitro#4014
There was a problem hiding this comment.
Cool! thanks for fixing that and letting me know, I don't think it's released yet tho.
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
a95e594 to
88d95df
Compare
logaretm
force-pushed
the
awad/js-1128-nitro-use-tracing-channels-for-srvx-and-h3
branch
from
1923dda to
be2f037
Compare
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
1317257 to
2a86160
Compare
logaretm
force-pushed
the
awad/js-1128-nitro-use-tracing-channels-for-srvx-and-h3
branch
2 times, most recently
from
3b30e36 to
36c96b6
Compare
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you apply the label |
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
99ea611 to
9ded4c9
Compare
logaretm
force-pushed
the
awad/js-1128-nitro-use-tracing-channels-for-srvx-and-h3
branch
2 times, most recently
from
3cf1515 to
342ea3b
Compare
logaretm
force-pushed
the
awad/js-1128-nitro-use-tracing-channels-for-srvx-and-h3
branch
from
342ea3b to
8d44437
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| }); | ||
| } | ||
|
|
||
| function setupSrvxTracingChannels(): void { |
There was a problem hiding this comment.
Bug: A race condition on the shared requestParentSpan variable can cause trace hierarchies to become corrupted during concurrent requests.
Severity: HIGH
Suggested Fix
Isolate the requestParentSpan state on a per-request basis. Use an AsyncLocalStorage context to store the span for each request, ensuring that concurrent operations do not interfere with each other's tracing data.
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/nitro/src/runtime/hooks/captureTracingEvents.ts#L155
Potential issue: In a concurrent server environment, the shared closure variable
`requestParentSpan` can be overwritten by a new request before a previous,
still-in-flight request has finished using it. This race condition occurs because the
variable is not isolated per request. As a result, Sentry trace spans from one request
can be incorrectly assigned as children of another request's spans, leading to corrupted
and misleading trace hierarchies.
Did we get this right? 👍 / 👎 to inform future reviews.
| } | ||
|
|
||
| function onTraceError(data: { span?: Span; error: unknown }): void { | ||
| captureException(data.error); |
There was a problem hiding this comment.
Bug: The onTraceError function unconditionally reports 3xx and 4xx HTTPErrors to Sentry, creating unnecessary noise, unlike other handlers which filter them.
Severity: MEDIUM
Suggested Fix
Update the onTraceError function to include the same filtering logic present in captureErrorHook.ts. Before calling captureException, add a check to ignore HTTPError instances with a status code between 300 and 499.
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/nitro/src/runtime/hooks/captureTracingEvents.ts#L71
Potential issue: The `onTraceError` function captures and reports all errors to Sentry
unconditionally. This behavior is inconsistent with other error handlers in the
codebase, such as `captureErrorHook`, which explicitly filter out `HTTPError`s with 3xx
and 4xx status codes. This results in expected client errors (e.g., 404 Not Found) being
logged as exceptions in Sentry, creating significant noise and making it difficult to
identify genuine server-side failures.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Pull request overview
Adds Nitro v3 HTTP server instrumentation by subscribing to the new h3/srvx tracing channels, plus error capturing and trace propagation via Server-Timing. It also introduces a Nitro v3 E2E test application to validate transactions, middleware spans, isolation, and propagation.
Changes:
- Add Nitro runtime plugin that subscribes to
h3.request,srvx.request, andsrvx.middlewaretracing channels and creates spans/transactions. - Add Nitro error hook capturing (with 3xx/4xx filtering) and
Server-Timingheader propagation. - Add Nitro v3 E2E test application and wire it into the canary workflow matrix.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates dependency graph for Nitro v3 beta + tracing-channel support. |
| packages/nitro/vite.config.ts | Adds Vitest typecheck configuration for the Nitro package. |
| packages/nitro/tsconfig.test.json | Includes vite.config.ts in test typechecking. |
| packages/nitro/test/runtime/hooks/captureErrorHook.test.ts | Unit tests for Nitro error hook capture behavior and filtering. |
| packages/nitro/src/runtime/plugins/server.ts | Registers Nitro runtime hooks and enables tracing-channel subscriptions. |
| packages/nitro/src/runtime/hooks/setServerTimingHeaders.ts | Injects Server-Timing for sentry-trace/baggage propagation. |
| packages/nitro/src/runtime/hooks/captureTracingEvents.ts | Core tracing-channel instrumentation for h3 and srvx spans. |
| packages/nitro/src/runtime/hooks/captureErrorHook.ts | Captures Nitro hook errors with context and status filtering. |
| packages/nitro/src/runtime/README.md | Documents runtime packaging constraints for the Nitro SDK. |
| packages/nitro/src/module.ts | Calls server instrumentation during module setup. |
| packages/nitro/src/instruments/instrumentServer.ts | Adds runtime server plugin to Nitro config via resolver. |
| packages/nitro/src/config.ts | Enables tracingChannel and installs the Nitro module via config helpers. |
| packages/nitro/rollup.npm.config.mjs | Builds runtime plugin entrypoint and externalizes tracing deps. |
| packages/nitro/package.json | Bumps Nitro peer/dev deps and adds otel-tracing-channel dependency. |
| dev-packages/e2e-tests/test-applications/nitro-3/vite.config.ts | Test app Vite+Nitro config using withSentryConfig. |
| dev-packages/e2e-tests/test-applications/nitro-3/tsconfig.json | TS config for the Nitro v3 E2E app. |
| dev-packages/e2e-tests/test-applications/nitro-3/tests/transactions.test.ts | E2E assertions for transaction/span creation + status + route naming + headers. |
| dev-packages/e2e-tests/test-applications/nitro-3/tests/trace-propagation.test.ts | E2E assertions for server→client propagation via Server-Timing. |
| dev-packages/e2e-tests/test-applications/nitro-3/tests/middleware.test.ts | E2E assertions for middleware spans and error status. |
| dev-packages/e2e-tests/test-applications/nitro-3/tests/isolation.test.ts | E2E assertions preventing scope/tag leakage across requests. |
| dev-packages/e2e-tests/test-applications/nitro-3/tests/errors.test.ts | E2E assertions for error capture + 404 filtering. |
| dev-packages/e2e-tests/test-applications/nitro-3/start-event-proxy.mjs | Starts the event proxy server used by test-utils. |
| dev-packages/e2e-tests/test-applications/nitro-3/src/main.ts | Browser SDK init for propagation tests. |
| dev-packages/e2e-tests/test-applications/nitro-3/server/middleware/test.ts | Middleware route used to validate spans and error capture. |
| dev-packages/e2e-tests/test-applications/nitro-3/server/api/test-transaction.ts | Route for baseline successful transaction/spans. |
| dev-packages/e2e-tests/test-applications/nitro-3/server/api/test-param/[id].ts | Param route to validate route parameterization. |
| dev-packages/e2e-tests/test-applications/nitro-3/server/api/test-isolation/[id].ts | Route to validate isolation scope behavior. |
| dev-packages/e2e-tests/test-applications/nitro-3/server/api/test-error.ts | Route which throws to validate error capture. |
| dev-packages/e2e-tests/test-applications/nitro-3/server/api/index.ts | Root route for the app. |
| dev-packages/e2e-tests/test-applications/nitro-3/playwright.config.mjs | Playwright config using shared test-utils defaults. |
| dev-packages/e2e-tests/test-applications/nitro-3/package.json | Defines E2E app scripts/deps for Nitro v3 canary testing. |
| dev-packages/e2e-tests/test-applications/nitro-3/instrument.mjs | Server-side Sentry init loaded via --import. |
| dev-packages/e2e-tests/test-applications/nitro-3/index.html | Minimal page to exercise browser tracing. |
| dev-packages/e2e-tests/test-applications/nitro-3/.npmrc | Points test app deps to Verdaccio in CI. |
| .github/workflows/canary.yml | Adds nitro-3 E2E app to canary matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const method = errorContext.event?.req.method ?? ''; | ||
| let path: string | null = null; | ||
|
|
||
| try { | ||
| if (errorContext.event?.req.url) { | ||
| path = new URL(errorContext.event.req.url).pathname; | ||
| } | ||
| } catch { | ||
| // If URL parsing fails, leave path as null | ||
| } |
There was a problem hiding this comment.
Same issue as above: transaction naming uses new URL(errorContext.event.req.url) which will fail for relative request URLs and skip setting the transaction name. Use a URL parser that supports relative URLs (or pass a base) so GET /path naming works reliably.
| function setupSrvxTracingChannels(): void { | ||
| // Store the parent span for all middleware and fetch to share | ||
| // This ensures they all appear as siblings in the trace | ||
| let requestParentSpan: Span | null = null; | ||
|
|
||
| const fetchChannel = tracingChannel<SrvxRequestEvent>('srvx.request', data => { | ||
| const parsedUrl = data.request._url ? parseStringToURLObject(data.request._url.href) : undefined; | ||
| const [spanName, urlAttributes] = getHttpSpanDetailsFromUrlObject(parsedUrl, 'server', 'auto.http.nitro.srvx', { | ||
| method: data.request.method, | ||
| }); | ||
|
|
||
| const sendDefaultPii = getClient()?.getOptions().sendDefaultPii ?? false; | ||
| const headerAttributes = httpHeadersToSpanAttributes( | ||
| Object.fromEntries(data.request.headers.entries()), | ||
| sendDefaultPii, | ||
| ); | ||
|
|
||
| return startSpanManual( | ||
| { | ||
| name: spanName, | ||
| attributes: { | ||
| ...urlAttributes, | ||
| ...headerAttributes, | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.nitro.srvx', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: data.middleware ? 'middleware.nitro' : 'http.server', | ||
| 'server.port': data.server.options.port, | ||
| }, | ||
| // Use the same parent span as middleware to make them siblings | ||
| parentSpan: requestParentSpan || undefined, | ||
| }, |
There was a problem hiding this comment.
requestParentSpan is a single mutable variable shared across all requests. With concurrent/overlapping requests (common in Node servers), middleware from one request can overwrite the parent span used for another request, causing incorrect span parenting and trace corruption. Store the parent span per-request instead (e.g. attach it to data.request via a Symbol, or use a WeakMap<Request, Span> keyed by the request object).
| function onTraceError(data: { span?: Span; error: unknown }): void { | ||
| captureException(data.error); | ||
| data.span?.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| data.span?.end(); | ||
| } |
There was a problem hiding this comment.
onTraceError calls captureException for every tracing-channel error. Since the Nitro plugin also registers an 'error' hook (captureErrorHook) which captures errors, this setup is likely to emit duplicate error events for the same thrown error. Consider limiting tracing-channel handling to span status/end only, and keep error capture centralized in the Nitro error hook (or add a guard to ensure errors are only captured once).
| "start": "PORT=3030 NODE_OPTIONS='--import ./instrument.mjs' node .output/server/index.mjs", | ||
| "clean": "npx rimraf node_modules pnpm-lock.yaml .output", | ||
| "test": "playwright test", | ||
| "test:build": "pnpm install && pnpm build", |
There was a problem hiding this comment.
The canary workflow matrix runs yarn test:build-canary for this app, but this package.json only defines test:build (no test:build-canary). This will cause the canary CI job for nitro-3 to fail. Add a test:build-canary script (can alias to test:build if no special canary steps are needed) or update the workflow entry to match the existing script name.
| "test:build": "pnpm install && pnpm build", | |
| "test:build": "pnpm install && pnpm build", | |
| "test:build-canary": "pnpm install && pnpm build", |
| _moduleOptions?: SentryNitroOptions, | ||
| _serverConfigFile?: string, | ||
| ): NitroConfig { | ||
| if (!config.tracingChannel) { |
There was a problem hiding this comment.
setupSentryNitroModule forces config.tracingChannel = true even when the user explicitly set it to false (because !config.tracingChannel is true for both undefined and false). This prevents users from disabling tracing channels. Change the condition to only enable when the option is unset (e.g. check for undefined / use nullish coalescing).
| if (!config.tracingChannel) { | |
| if (config.tracingChannel === undefined) { |
| if (errorContext.event) { | ||
| ctx.method = errorContext.event.req.method; | ||
|
|
||
| try { | ||
| const url = new URL(errorContext.event.req.url); | ||
| ctx.path = url.pathname; | ||
| } catch { | ||
| // If URL parsing fails, leave path undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
new URL(errorContext.event.req.url) will throw for typical Node/Nitro request URLs which are often relative (e.g. /api/foo?x=1). Because the error is swallowed, path and the transaction name/structured context will be missing in real usage. Prefer parsing via parseStringToURLObject (which supports relative URLs) or provide a base URL when constructing URL.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 214e8f5. Configure here.
| "clean": "npx rimraf node_modules pnpm-lock.yaml .output", | ||
| "test": "playwright test", | ||
| "test:build": "pnpm install && pnpm build", | ||
| "test:assert": "pnpm test" |
There was a problem hiding this comment.
Missing test:build-canary script in nitro-3 package.json
Medium Severity
The canary workflow references build-command: 'test:build-canary' for the nitro-3 test application, but the package.json only defines test:build and test:assert scripts — no test:build-canary. Every other test application listed in the canary workflow with test:build-canary has it defined in their package.json (e.g., nextjs-14, angular-21). The canary CI job for nitro-3 will fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 214e8f5. Configure here.
| captureException(data.error); | ||
| data.span?.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| data.span?.end(); | ||
| } |
There was a problem hiding this comment.
captureException missing required mechanism in onTraceError
Medium Severity
onTraceError calls captureException(data.error) without setting a mechanism (requiring handled and type). This contrasts with captureErrorHook which properly provides mechanism: { handled: false, type: 'auto.function.nitro' }. Since onTraceError is used by h3, srvx fetch, and srvx middleware error handlers, errors captured through tracing channels will lack mechanism metadata.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 214e8f5. Configure here.
| function setupSrvxTracingChannels(): void { | ||
| // Store the parent span for all middleware and fetch to share | ||
| // This ensures they all appear as siblings in the trace | ||
| let requestParentSpan: Span | null = null; |
There was a problem hiding this comment.
Shared mutable requestParentSpan causes race condition across requests
Medium Severity
requestParentSpan is a single closure variable shared across all concurrent requests. When request A's first middleware sets it and request B's middleware overwrites it before request A's fetch handler reads it, request A gets the wrong parent span. This corrupts trace hierarchies under concurrent load.
Reviewed by Cursor Bugbot for commit 214e8f5. Configure here.
| captureException(data.error); | ||
| data.span?.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| data.span?.end(); | ||
| } |
There was a problem hiding this comment.
Errors captured twice via both tracing channel and error hook
Medium Severity
When an h3 handler throws, onTraceError calls captureException(data.error) via the tracing channel error subscriber, and then captureErrorHook also calls captureException via the Nitro error hook — both registered in server.ts. This results in the same error being reported to Sentry twice. The deduplication integration may not catch this since captureErrorHook receives an HTTPError wrapper while onTraceError receives the original error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 214e8f5. Configure here.
Implements HTTP server instrumentation for both
h3andsrvxby listening to their tracing channel events.h3TC PR: feat: experimental tracing support h3js/h3#1251srvxTC PR: feat: experimental tracing channel support h3js/srvx#141Closes #18123
This PR is part of a stack: