feat(nextjs): Extract tracing logic from server component wrapper templates #18408
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| () => 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.
|
|
||
| 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.
There was a problem hiding this comment.
the scope already gets forked earlier
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.
|
| } 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, { |
There was a problem hiding this comment.
Bug: The shouldCapture flag is never set to true for general errors, preventing captureException() from being called.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The shouldCapture flag is initialized to false and is only explicitly set to false for isNotFoundNavigationError and isRedirectNavigationError. In the else branch, which handles all other errors, shouldCapture is never set to true. Consequently, the if (shouldCapture) block containing captureException(error, ...) never executes, preventing any actual errors from being reported to Sentry.
💡 Suggested Fix
In the else branch of the error handling logic, add shouldCapture = true; to ensure that general errors are captured by Sentry.
🤖 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/wrapServerComponentWithSentry.ts#L45-L63
Potential issue: The `shouldCapture` flag is initialized to `false` and is only
explicitly set to `false` for `isNotFoundNavigationError` and
`isRedirectNavigationError`. In the `else` branch, which handles all other errors,
`shouldCapture` is never set to `true`. Consequently, the `if (shouldCapture)` block
containing `captureException(error, ...)` never executes, preventing any actual errors
from being reported to Sentry.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5790254
| 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()); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Bug: The created isolation scope is not activated with withIsolationScope(), causing metadata loss during error handling.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
An isolation scope is created and enriched with metadata at the beginning of wrapServerComponentWithSentry, but the subsequent execution of the server component's callback is not wrapped with withIsolationScope(). As a result, the error handler retrieves the current ambient isolation scope instead of the specifically created and enriched one, leading to a loss of context and metadata during error processing.
💡 Suggested Fix
Wrap the execution of the server component's callback within withIsolationScope(isolationScope, () => { ... }) to ensure the correct scope is active during error handling.
🤖 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/wrapServerComponentWithSentry.ts#L29-L74
Potential issue: An isolation scope is created and enriched with metadata at the
beginning of `wrapServerComponentWithSentry`, but the subsequent execution of the server
component's callback is not wrapped with `withIsolationScope()`. As a result, the error
handler retrieves the current ambient isolation scope instead of the specifically
created and enriched one, leading to a loss of context and metadata during error
processing.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5790254
| }); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Exceptions never captured due to unset shouldCapture flag
The shouldCapture variable is initialized to false on line 45 and never set to true. In the original code, captureException was called directly in the else branch for actual errors. Now it's guarded by if (shouldCapture), but the else branch (lines 57-58) only sets the span status without setting shouldCapture = true. This means real exceptions from server components will never be captured by Sentry.
| 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.
closes https://linear.app/getsentry/issue/JS-1207/remove-tracing-from-app-router-server-components-templates
closes #18307