getsentry/sentry-javascriptPublic

NotificationsYou must be signed in to change notification settings
Fork1.7k
Star8.5k

feat(nextjs): remove tracing from pages router API routes#18394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Draft

logaretm wants to merge12 commits intodevelop

base:develop

Choose a base branch

fromawad/js-1209-remove-tracing-from-pages-router-api-2

Draft

feat(nextjs): remove tracing from pages router API routes#18394

logaretm wants to merge12 commits intodevelopfromawad/js-1209-remove-tracing-from-pages-router-api-2

Conversation

Copy link

Collaborator

logaretm commentedDec 3, 2025•
edited by github-actionsbot
Loading

This is a big PR because of the tests added, the logic changes themselves are pretty minimal, so make sure to filter out the files when reviewing.

Actual Changes

Removed traces fromwrapApiHandlerWithSentry for both the server runtime and the edge runtime

Tests Added

Since we are dropping explicit trace wrapping logic from the pages router templates, we needed to have tests on Next 16 to make sure the page router there still works in both Turbopack and Webpack with minimal differences in the quality of the traces.

We have around 36 test case to check, but only 20 can pass at this time due to the wrapping logic still present, ideally as we move towards removing templates entirely, we can then unskip those tests and tighten many of the assertions we have to accommodate for it.

I added TODOs and aTEST_STATUS.md document to detail what we should update once the SDK drops more of the wrapping logic it currently has.

Closes#18450 (added automatically)

Copy link

linearbot commentedDec 3, 2025

JS-1209 Remove tracing from Pages Router API handler template

logaretm force-pushed theawad/js-1209-remove-tracing-from-pages-router-api-2 branch 2 times, most recently from2af20e5 to2516846Compare

December 3, 2025 10:49

Copy link

Contributor

github-actionsbot commentedDec 3, 2025•
edited
Loading

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.

Scenario	Requests/s	% of Baseline	Prev. Requests/s	Change %
GET Baseline	11,022	-	8,535	+29%
GET With Sentry	1,954	18%	1,608	+22%
GET With Sentry (error only)	7,856	71%	5,850	+34%
POST Baseline	1,145	-	1,166	-2%
POST With Sentry	578	50%	546	+6%
POST With Sentry (error only)	1,022	89%	1,023	-0%
MYSQL Baseline	4,044	-	3,188	+27%
MYSQL With Sentry	530	13%	427	+24%
MYSQL With Sentry (error only)	3,313	82%	2,607	+27%

View base workflow run

logaretm force-pushed theawad/js-1209-remove-tracing-from-pages-router-api-2 branch 3 times, most recently fromc055f2d to8102dd9Compare

December 9, 2025 14:43

This was linked to issuesDec 9, 2025

Remove tracing from Pages Router API handler template#18309

Open

Remove tracing from Edge API handler#18310

Open

logaretm marked this pull request as ready for review

December 10, 2025 08:45

CopilotAI review requested due to automatic review settings

December 10, 2025 08:45

github-actionsbot mentioned this pull request

Dec 10, 2025

feat(nextjs): remove tracing from pages router API routes#18450

Closed

logaretm requested a review fromchargome

December 10, 2025 08:45

Copilotstarted reviewing on behalf oflogaretm

December 10, 2025 08:45

View session

sentrybot reviewed

Dec 10, 2025

View reviewed changes

packages/nextjs/src/edge/wrapApiHandlerWithSentry.tsShow resolvedHide resolved

CopilotAI reviewed

Dec 10, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

.../e2e-tests/test-applications/nextjs-16-pages-dir/tests/server/excluded-api-endpoints.test.ts OutdatedShow resolvedHide resolved

dev-packages/e2e-tests/test-applications/nextjs-16-pages-dir/tests/edge-route.test.ts OutdatedShow resolvedHide resolved

dev-packages/e2e-tests/test-applications/nextjs-16-pages-dir/tests/pages-ssr-errors.test.ts OutdatedShow resolvedHide resolved

dev-packages/e2e-tests/test-applications/nextjs-16-pages-dir/tests/proxy.test.ts OutdatedShow resolvedHide resolved

dev-packages/e2e-tests/test-applications/nextjs-16-pages-dir/tests/transactions.test.ts OutdatedShow resolvedHide resolved

...kages/e2e-tests/test-applications/nextjs-16-pages-dir/pages/pages-router/ssr-error-class.tsx OutdatedShow resolvedHide resolved

...packages/e2e-tests/test-applications/nextjs-16-pages-dir/pages/pages-router/ssr-error-fc.tsx OutdatedShow resolvedHide resolved

cursorbot reviewed

Dec 10, 2025

View reviewed changes

dev-packages/e2e-tests/test-applications/nextjs-16-pages-dir/proxy.ts OutdatedShow resolvedHide resolved

sentrybot reviewed

Dec 10, 2025

View reviewed changes

packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts

Comment on lines 93 to 103

		// as the runtime freezes as soon as the error is thrown below
		awaitflushSafelyWithTimeout();

		// 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.)
		throwobjectifiedErr;
		}
		},
		});
		}

This comment was marked as outdated.

Copy link

CollaboratorAuthor

logaretmDec 10, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I switched to a blocking await here before throwing which should work better than callingwaitUntil 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.

cursorbot reviewed

Dec 10, 2025

View reviewed changes

packages/nextjs/src/server/index.tsShow resolvedHide resolved

logaretm added8 commits

December 16, 2025 12:08

feat(nextjs): remove tracing from pages router API routes

e2fef71

fix: metadata processing

6fce173

fix: set the transaction name on the isolation scope level

ec986a8

tests: try without paramaterization

31fac7a

fix: parameterization backfill

eb4c84d

test: update expectations and fix lint issues

596150f

test: update expectations

ea4d841

feat: drop tracing from edge pages runtime wrapping

bdb4be3

logaretm force-pushed theawad/js-1209-remove-tracing-from-pages-router-api-2 branch from949da51 tobdb4be3Compare

December 16, 2025 10:08

logaretm added2 commits

December 16, 2025 13:31

test: fix assertion expectation to expect parent id span

ceaa144

tests: adjust assertions to be consistent

03e7627

feat: remove transaction and SDK metadata setting from the wrapper

9c04b47

logaretm force-pushed theawad/js-1209-remove-tracing-from-pages-router-api-2 branch from6de8d44 to9c04b47Compare

December 16, 2025 12:24

cursorbot reviewed

Dec 16, 2025

View reviewed changes

packages/nextjs/src/edge/index.ts

		// Set transaction name on isolation scope to ensure parameterized routes are used
		// The HTTP server integration sets it on isolation scope, so we need to match that
		constmethod=event.request?.method\|\|'GET';
		path=path??event.request?.url??'/';

Copy link

cursorbotDec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Bug: Nullish coalescing won't handle empty path string

Thepath variable is assigned fromString(...).replace(...).trim() on line 161, which always returns a string (possibly empty""). On line 165,path = path ?? event.request?.url ?? '/' uses nullish coalescing (??), which only falls back fornull/undefined, not for empty strings. If the span name is exactly'executing api route (pages)' with no route suffix,path becomes"" and the fallback toevent.request?.url or/ never triggers. This results in malformed transaction names like"GET " instead of"GET /". Using the logical OR operator (||) would correctly handle empty strings.