feat(nextjs): remove tracing from pages router API routes - #18394
feat(nextjs): remove tracing from pages router API routes#18394logaretm wants to merge 7 commits into
Conversation
2af20e5 to
2516846
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.
|
c055f2d to
8102dd9
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR removes explicit tracing instrumentation from Pages Router API route wrappers (wrapApiHandlerWithSentry) for both Node.js server and Edge runtimes, relying instead on Next.js's built-in OpenTelemetry instrumentation to create transaction spans. The wrappers now only handle error capture, transaction name setting on isolation scope, and route backfilling.
Key Changes:
- Removed manual span creation (
startSpanManual,startSpan) from API route wrappers - Wrappers now set transaction names on isolation scope and use route backfill attributes for parameterized route names
- Updated test expectations to reflect origin change from
'auto.http.nextjs'to'auto'(from Next.js OTEL) - Added comprehensive Next.js 16 Pages Router test application with 36+ test cases
Reviewed changes
Copilot reviewed 74 out of 76 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts |
Removed tracing logic; now only captures errors and sets transaction metadata via isolation scope and route backfill |
packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts |
Removed tracing logic from edge runtime wrapper; simplified to error monitoring only |
packages/nextjs/src/server/index.ts |
Added SEMANTIC_ATTRIBUTE_SENTRY_SOURCE to route backfill logic to ensure proper transaction source attribution |
packages/nextjs/test/config/withSentry.test.ts |
Deleted unit test that verified explicit span creation (no longer applicable) |
dev-packages/e2e-tests/test-applications/nextjs-13/tests/**/*.test.ts |
Updated assertions to expect 'auto' origin instead of 'auto.http.nextjs' |
dev-packages/e2e-tests/test-applications/create-next-app/tests/**/*.test.ts |
Updated assertions for new tracing behavior with flexible parent span matching |
dev-packages/e2e-tests/test-applications/nextjs-pages-dir/tests/edge-route.test.ts |
Removed runtime context assertions and skipped scope isolation test |
dev-packages/e2e-tests/test-applications/nextjs-16-pages-dir/**/* |
New comprehensive test application with 36 test cases for Next.js 16 Pages Router covering both webpack and turbopack |
dev-packages/e2e-tests/test-applications/nextjs-16-pages-dir/TEST_STATUS.md |
Detailed documentation of test status, known issues, and future work items |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // as the runtime freezes as soon as the error is thrown below | ||
| await flushSafelyWithTimeout(); | ||
|
|
||
| // We rethrow here so that nextjs can do with the error whatever it would normally do. (Sometimes "whatever it | ||
| // would normally do" is to allow the error to bubble up to the global handlers - another reason we need to mark | ||
| // the error as already having been captured.) | ||
| throw objectifiedErr; | ||
| } | ||
| }, | ||
| }); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I switched to a blocking await here before throwing which should work better than calling waitUntil then throwing as the runtime can shutdown as soon as an unhandled error like these are thrown.
This matches the logic we have in the other API handler wrapper.
6de8d44 to
9c04b47
Compare
4538dfe to
ecfe19b
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 |
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Mind adding one specific node runtime api handler e2e test here 🙏
| await flushSafelyWithTimeout(); | ||
| // we need to await the flush here to ensure that the error is captured | ||
| // as the runtime freezes as soon as the error is thrown below | ||
| await flushSafelyWithTimeout(); |
There was a problem hiding this comment.
l: Might make sense to flush in finally
There was a problem hiding this comment.
Wouldn't it double flush for the error path if we do it in finally? Right now we flush (non-blocking) on success and flush (blocking) on error.
0730e09 to
2b50fce
Compare
2b50fce to
43824e1
Compare
|
👋 @nicohrubec — Please review this PR when you get a chance! |
43824e1 to
a858ac9
Compare
a858ac9 to
2fee66b
Compare
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 2fee66b. Configure here.
| try { | ||
| return await wrappingTarget.apply(thisArg, args); |
There was a problem hiding this comment.
Bug: The edge runtime's wrapApiHandlerWithSentry is missing a waitUntil(flushSafelyWithTimeout()) call on the success path, which can lead to lost Sentry events in serverless environments.
Severity: HIGH
Suggested Fix
Add waitUntil(flushSafelyWithTimeout()) in the success path of the edge wrapApiHandlerWithSentry function, similar to the implementation in the pages router, to ensure events are flushed before the serverless runtime freezes.
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/nextjs/src/edge/wrapApiHandlerWithSentry.ts#L47-L48
Potential issue: In the edge runtime's `wrapApiHandlerWithSentry` function, the success
path does not explicitly flush Sentry events before returning a response. In serverless
edge environments, the runtime can freeze immediately after the response is sent. This
omission of `waitUntil(flushSafelyWithTimeout())` means pending Sentry transactions and
other events may be lost if they are not flushed before the runtime freezes. This
behavior is inconsistent with the pages router implementation, which correctly includes
this call to ensure data is sent.
| const path = spanName.replace(ATTR_NEXT_PAGES_API_ROUTE_TYPE, '').trim(); | ||
| // eslint-disable-next-line typescript/no-deprecated | ||
| const method = attributes[HTTP_REQUEST_METHOD] ?? attributes[HTTP_METHOD]; | ||
| span.setName(`${typeof method === 'string' ? method : 'GET'} ${path}`); |
There was a problem hiding this comment.
Bug: The enhanceRunHandlerRootSpan function uses the raw URL path from the span name, creating high-cardinality transaction names for parameterized Edge API routes.
Severity: HIGH
Suggested Fix
Update the Edge runtime wrapper for API handlers (wrapApiHandlerWithSentry) to set the TRANSACTION_ATTR_SENTRY_ROUTE_BACKFILL attribute on the root span, similar to how the Node.js runtime wrapper does. This will allow the existing backfill mechanism to correctly use the parameterized route for the transaction name.
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/nextjs/src/edge/enhanceRunHandlerRootSpan.ts#L38-L41
Potential issue: For Pages Router API routes running on the Edge runtime, the
`enhanceRunHandlerRootSpan` function derives the transaction name from the
`Node.runHandler` span's name. This span name contains the raw, unparameterized URL path
(e.g., `/api/users/123`) instead of the parameterized route (`/api/users/[id]`). As a
result, every unique URL path generates a distinct transaction, leading to
high-cardinality transaction names in Sentry. This undermines the benefit of
parameterized routes for grouping and analysis. The Node.js runtime wrapper correctly
handles this by setting a backfill attribute, but the Edge wrapper does not.
Drop the trace-wrapping logic from wrapApiHandlerWithSentry on both the Node server and Edge runtimes. The wrappers now only capture errors and set the transaction name on the isolation scope; the transaction itself comes from Next.js's own OTEL span, which we backfill with the right op, source and name.
…tion The wrapper forks a fresh isolation scope via withIsolationScope, but the transaction is now the Next.js auto-instrumentation root span, which captured a different scope. Request data, tags, and breadcrumbs set during the handler were therefore lost from the transaction. Bind the wrapper's scope to the active root span so they land on it again.
…e-runtime e2e test
The pages-router API wrapper no longer creates its own transaction and instead binds onto Next.js's BaseServer.handleRequest root span. For an API route that span carries no http.route/next.route, so enhanceHandleRequestRootSpan never sets the route source and the transaction keeps a "custom" source. Set the route backfill attribute the same way the data-fetcher wrapper does so the transaction gets a "route" source and a parameterized http.route.
The root span enhancers rewrite sentry.source on the transaction's trace-context attributes during preprocessEvent, but transaction_info.source is snapshotted at span end and never updated. Pages-router API routes bind onto Next.js's BaseServer.handleRequest root span, which carries no http.route at span end, so the source stays "custom" in transaction_info even though the enhancer (fed by the route backfill) sets it to "route" on the trace context. Mirror the enhanced source onto transaction_info so backfilled routes report "route".
a9c2503 to
3de7fde
Compare
Removes the trace-wrapping logic from
wrapApiHandlerWithSentryon both the Node server and Edge runtimes, so the wrappers now only capture errors. Instead of creating our ownforceTransactionspan, we rely on Next.js's own OTEL span becoming the transaction and backfill the op, source and parameterized name onto it.closes #18450