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, { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| 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.
| }); | ||
| }); | ||
| }); | ||
| } |
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.
| 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 transaction name for successful server component requests is not being set because setTransactionName() is only called within the error handler.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
In wrapServerComponentWithSentry, transaction names for successful server component requests are no longer being set. The call to scope.setTransactionName() was moved into the error handler of handleCallbackErrors. Consequently, for any server component that executes without throwing an error, the transaction name will not be set, which is a functional regression from the previous implementation where it was set for both success and error paths. This affects transaction identification in Sentry for all successful server component renders.
💡 Suggested Fix
Move the setTransactionName() call out of the error handler and into a withScope or withIsolationScope block that wraps the component's execution, similar to the previous implementation. This will ensure the transaction name is set for both successful and failed executions.
🤖 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#L39-L74
Potential issue: In `wrapServerComponentWithSentry`, transaction names for successful
server component requests are no longer being set. The call to
`scope.setTransactionName()` was moved into the error handler of `handleCallbackErrors`.
Consequently, for any server component that executes without throwing an error, the
transaction name will not be set, which is a functional regression from the previous
implementation where it was set for both success and error paths. This affects
transaction identification in Sentry for all successful server component renders.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7443279
There was a problem hiding this comment.
this is just relevant for the error case
| }, | ||
| }, | ||
| 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 sentry-trace header is no longer being extracted and set as the TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL attribute, which breaks distributed tracing for server components.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL attribute is no longer being set on the root span for server components. The refactored wrapServerComponentWithSentry function omits the logic that extracts the sentry-trace header and sets it as an attribute on the span. Downstream event processors rely on this attribute to correctly backfill trace context and make sampling decisions. Its absence breaks distributed tracing for server components, as the trace context from incoming requests will not be propagated.
💡 Suggested Fix
Reintroduce the logic to read the sentry-trace header from the request and set it as the TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL attribute on the root span. This logic could be added back to wrapServerComponentWithSentry or into the new handleOnSpanStart function to align with wrapGenerationFunctionWithSentry.
🤖 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#L36-L77
Potential issue: The `TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL` attribute is no longer
being set on the root span for server components. The refactored
`wrapServerComponentWithSentry` function omits the logic that extracts the
`sentry-trace` header and sets it as an attribute on the span. Downstream event
processors rely on this attribute to correctly backfill trace context and make sampling
decisions. Its absence breaks distributed tracing for server components, as the trace
context from incoming requests will not be propagated.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7443279
size-limit report 📦
|
closes https://linear.app/getsentry/issue/JS-1207/remove-tracing-from-app-router-server-components-templates
closes #18307