feat(nextjs): remove tracing from pages router API routes#18394
feat(nextjs): remove tracing from pages router API routes#18394logaretm wants to merge 2 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 |
7dc96ff to
c65e749
Compare
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.
c65e749 to
ea4a63b
Compare
…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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 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 7d305c8. Configure here.
| // 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; | ||
| } |
There was a problem hiding this comment.
Missing flush on successful API responses
High Severity
The Node pages-router wrapper no longer calls waitUntil(flushSafelyWithTimeout()) on successful responses. Flushing only happens in the catch path, so on serverless (e.g. Vercel) the runtime can freeze before successful transactions and other pending telemetry are sent. Edge still flushes on spanEnd, and App Router’s wrapRouteHandlerWithSentry still flushes on completion.
Reviewed by Cursor Bugbot for commit 7d305c8. Configure here.
| 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.
Missing http.route on Edge API spans
Medium Severity
enhanceRunHandlerRootSpan backfills op, source, and transaction name for pages API routes, but never sets http.route. The Edge wrapper previously wrote HTTP_ROUTE from parameterizedRoute, and that write was removed without a replacement, so Edge pages API transactions lose the route attribute.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7d305c8. Configure here.
|
|
||
| expect(getName()).toBe('GET /api/edge-endpoint'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Feat PR lacks integration/E2E coverage
Low Severity
This is a feat PR that only adds/updates unit tests. Per the PR review guidelines, feat PRs need at least one integration or E2E test covering the new behavior (relying on Next.js’s root span and backfilling op/source/name instead of creating a forceTransaction). Flagged because it was mentioned in the rules file.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 7d305c8. Configure here.
| }); | ||
|
|
||
| // 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(); | ||
| // 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(); | ||
|
|
||
| // 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; | ||
| } | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| // 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; | ||
| } |
There was a problem hiding this comment.
Bug: In Node.js serverless environments, wrapApiHandlerWithSentry no longer flushes Sentry events for successful API requests, which can lead to lost transaction data if the function terminates quickly.
Severity: HIGH
Suggested Fix
Reintroduce a call to flushSafelyWithTimeout() in the success path for the Node.js wrapApiHandlerWithSentry wrapper. This can be done by restoring the proxy on res.end or by adding the flush call before the original handler's logic completes, ensuring event flushing for both successful and failed responses.
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/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts#L70-L98
Potential issue: The `wrapApiHandlerWithSentry` function for Node.js environments no
longer flushes Sentry events on successful API route executions. The previous
implementation called `flushSafelyWithTimeout()` on every response completion, but this
was removed from the success path. Unlike the Edge runtime, which has an automatic
flushing mechanism on the `spanEnd` hook, the Node.js server runtime relies on manual
flushing. This change means that for successful requests in a serverless Node.js
environment, pending events like transactions and breadcrumbs may be lost if the
function terminates after sending the response but before Sentry's background agent
sends the events.
Did we get this right? 👍 / 👎 to inform future reviews.
| span.setOp('http.server'); | ||
| attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] = 'route'; |
There was a problem hiding this comment.
Bug: The enhanceRunHandlerRootSpan function fails to set the SEMANTIC_ATTRIBUTE_SENTRY_OP attribute, causing the sentry.op attribute to be missing from some transaction events in the Edge runtime.
Severity: MEDIUM
Suggested Fix
In enhanceRunHandlerRootSpan, explicitly set the SEMANTIC_ATTRIBUTE_SENTRY_OP attribute on the span's attributes, similar to how it is done in enhanceHandleRequestRootSpan. This will ensure that sentry.op is correctly populated for both legacy transaction events and streamed 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/nextjs/src/edge/enhanceRunHandlerRootSpan.ts#L35-L36
Potential issue: In the Edge runtime, the `enhanceRunHandlerRootSpan` function only
calls `span.setOp()`, which does not set the `sentry.op` attribute for legacy
transaction events processed via the `preprocessEvent` path. This is inconsistent with
`enhanceHandleRequestRootSpan` which explicitly sets both the operation and the
corresponding `SEMANTIC_ATTRIBUTE_SENTRY_OP` attribute. This omission will cause the
`sentry.op` attribute to be missing from transaction events for pages-router API routes
on the Edge runtime when using the legacy span processing path, leading to incorrect
data in Sentry.
Did we get this right? 👍 / 👎 to inform future reviews.
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