feat(nextjs): Extract tracing logic from server component wrapper templates #18408
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Comment on lines
40
to
45
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| const isolationScope = getIsolationScope(); | ||
| const span = getActiveSpan(); | ||
| const { componentRoute, componentType } = context; | ||
| isolationScope.setTransactionName(`${componentType} Server Component (${componentRoute})`); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
58
to
68
|
|
||
| return startSpanManual( | ||
| { | ||
| op: 'function.nextjs', | ||
| name: `${componentType} Server Component (${componentRoute})`, | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs.server_component', | ||
| 'sentry.nextjs.ssr.function.type': componentType, | ||
| 'sentry.nextjs.ssr.function.route': componentRoute, | ||
| }, | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Bug: Missing isolation scope context breaks request metadata attachment
The refactored code retrieves an isolation scope via commonObjectToIsolationScope at line 29 and sets normalizedRequest metadata on it, but the handleCallbackErrors callback is not wrapped with withIsolationScope. When captureException is called in the error handler, getIsolationScope() at line 42 returns the current global isolation scope - not the one with the request metadata. This causes captured exceptions to be missing request context (like headers) that was set up earlier. The original code used withIsolationScope(isolationScope, ...) to ensure all nested code ran with the correct isolation scope.
Member
Author
There was a problem hiding this comment.
the scope already gets forked earlier
Contributor
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.
|
Comment on lines
53
to
63
| } else if (isRedirectNavigationError(error)) { | ||
| shouldCapture = false; | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| } | ||
| } | ||
|
|
||
| return startSpanManual( | ||
| { | ||
| op: 'function.nextjs', | ||
| name: `${componentType} Server Component (${componentRoute})`, | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs.server_component', | ||
| 'sentry.nextjs.ssr.function.type': componentType, | ||
| 'sentry.nextjs.ssr.function.route': componentRoute, | ||
| if (shouldCapture) { | ||
| captureException(error, { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
+64
to
+74
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| page, | ||
| }) => { | ||
| const serverTransactionEventPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { | ||
| console.log(transactionEvent?.transaction); |
There was a problem hiding this comment.
Bug: Debug console.log left in test code
A console.log(transactionEvent?.transaction) statement appears to have been left in the test code, likely from debugging. While this is in test code, it will produce unnecessary output during test runs.
Comment on lines
+64
to
+74
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
Author
There was a problem hiding this comment.
this is just relevant for the error case
Comment on lines
67
to
77
| }, | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| }); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
size-limit report 📦
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes https://linear.app/getsentry/issue/JS-1207/remove-tracing-from-app-router-server-components-templates
closes #18307