-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(nextjs): Extract tracing logic from server component wrapper templates #18408
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
45f14ab
fbb8f37
2a2613d
960dcd1
40f32cc
805e1af
076c4df
6b15148
c6751b3
8f07b99
e30ea5c
4fec975
ecf4dab
1b8b439
01cbe44
ba33529
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import { PropsWithChildren } from 'react'; | ||
|
|
||
| export const dynamic = 'force-dynamic'; | ||
|
|
||
| export default function Layout({ children }: PropsWithChildren<{}>) { | ||
| return ( | ||
| <div> | ||
| <p>DynamicLayout</p> | ||
| {children} | ||
| </div> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| export const dynamic = 'force-dynamic'; | ||
|
|
||
| export default async function Page() { | ||
| return ( | ||
| <div> | ||
| <p>Dynamic Page</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| export async function generateMetadata() { | ||
| return { | ||
| title: 'I am dynamic page generated metadata', | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,23 @@ | ||
| import type { PropagationContext, SpanAttributes } from '@sentry/core'; | ||
| import { debug, getActiveSpan, getRootSpan, GLOBAL_OBJ, Scope, spanToJSON, startNewTrace } from '@sentry/core'; | ||
| import { ATTR_HTTP_ROUTE } from '@opentelemetry/semantic-conventions'; | ||
| import type { PropagationContext, Span, SpanAttributes } from '@sentry/core'; | ||
| import { | ||
| debug, | ||
| getActiveSpan, | ||
| getRootSpan, | ||
| GLOBAL_OBJ, | ||
| Scope, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
| spanToJSON, | ||
| startNewTrace, | ||
| } from '@sentry/core'; | ||
| import { DEBUG_BUILD } from '../debug-build'; | ||
| import { ATTR_NEXT_SEGMENT, ATTR_NEXT_SPAN_NAME, ATTR_NEXT_SPAN_TYPE } from '../nextSpanAttributes'; | ||
| import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; | ||
|
|
||
| const commonPropagationContextMap = new WeakMap<object, PropagationContext>(); | ||
|
|
||
| const PAGE_SEGMENT = '__PAGE__'; | ||
|
|
||
| /** | ||
| * Takes a shared (garbage collectable) object between resources, e.g. a headers object shared between Next.js server components and returns a common propagation context. | ||
| * | ||
|
|
@@ -126,16 +138,44 @@ export function isResolveSegmentSpan(spanAttributes: SpanAttributes): boolean { | |
| /** | ||
| * Returns the enhanced name for a resolve segment span. | ||
| * @param segment The segment of the resolve segment span. | ||
| * @param route The route of the resolve segment span. | ||
| * @returns The enhanced name for the resolve segment span. | ||
| */ | ||
| export function getEnhancedResolveSegmentSpanName(segment: string): string { | ||
| if (segment === '__PAGE__') { | ||
| return 'resolve page module'; | ||
| export function getEnhancedResolveSegmentSpanName({ segment, route }: { segment: string; route: string }): string { | ||
| if (segment === PAGE_SEGMENT) { | ||
| return `resolve page server component "${route}"`; | ||
| } | ||
|
|
||
| if (segment === '') { | ||
| return 'resolve root layout module'; | ||
| return 'resolve root layout server component'; | ||
| } | ||
|
|
||
| return `resolve layout server component "${segment}"`; | ||
| } | ||
|
|
||
| /** | ||
| * Maybe enhances the span name for a resolve segment span. | ||
| * If the span is not a resolve segment span, this function does nothing. | ||
| * @param activeSpan The active span. | ||
| * @param spanAttributes The attributes of the span to check. | ||
| * @param rootSpanAttributes The attributes of the according root span. | ||
| */ | ||
| export function maybeEnhanceServerComponentSpanName( | ||
| activeSpan: Span, | ||
| spanAttributes: SpanAttributes, | ||
| rootSpanAttributes: SpanAttributes, | ||
| ): void { | ||
| if (!isResolveSegmentSpan(spanAttributes)) { | ||
| return; | ||
| } | ||
|
|
||
| return `resolve layout module "${segment}"`; | ||
| const segment = spanAttributes[ATTR_NEXT_SEGMENT] as string; | ||
| const route = rootSpanAttributes[ATTR_HTTP_ROUTE]; | ||
| const enhancedName = getEnhancedResolveSegmentSpanName({ segment, route: typeof route === 'string' ? route : '' }); | ||
| activeSpan.updateName(enhancedName); | ||
| activeSpan.setAttributes({ | ||
| 'sentry.nextjs.ssr.function.type': segment === PAGE_SEGMENT ? 'Page' : 'Layout', | ||
| 'sentry.nextjs.ssr.function.route': route, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Inconsistent type handling for route span attributeThe |
||
| }); | ||
| activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'function.nextjs'); | ||
| } | ||
|
chargome marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,13 @@ | ||
| import type { RequestEventData } from '@sentry/core'; | ||
| import { captureException, handleCallbackErrors, winterCGHeadersToDict, withIsolationScope } from '@sentry/core'; | ||
| import { | ||
| captureException, | ||
| getActiveSpan, | ||
| getIsolationScope, | ||
| handleCallbackErrors, | ||
| SPAN_STATUS_ERROR, | ||
| SPAN_STATUS_OK, | ||
| winterCGHeadersToDict, | ||
| } from '@sentry/core'; | ||
| import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; | ||
| import type { ServerComponentContext } from '../common/types'; | ||
| import { flushSafelyWithTimeout, waitUntil } from '../common/utils/responseEnd'; | ||
|
|
@@ -28,28 +36,37 @@ export function wrapServerComponentWithSentry<F extends (...args: any[]) => any> | |
| } satisfies RequestEventData, | ||
| }); | ||
|
|
||
| return withIsolationScope(isolationScope, () => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| const isolationScope = getIsolationScope(); | ||
| const span = getActiveSpan(); | ||
| const { componentRoute, componentType } = context; | ||
| isolationScope.setTransactionName(`${componentType} Server Component (${componentRoute})`); | ||
|
Comment on lines
+39
to
+44
This comment was marked as outdated.
Sorry, something went wrong. |
||
|
|
||
| if (span) { | ||
| 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 { | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| } | ||
|
Comment on lines
+47
to
57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The refactored 🔍 Detailed AnalysisIn the refactored 💡 Suggested FixEnsure the active span is explicitly ended after the wrapped function executes. A potential fix is to get the active span before the function runs and call 🤖 Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we delegate the entire span lifecycle to next |
||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Navigation errors captured when no active span existsThe navigation error checks (
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Next will always emit a span here |
||
|
|
||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
|
cursor[bot] marked this conversation as resolved.
Outdated
|
||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
|
Comment on lines
59
to
+71
This comment was marked as outdated.
Sorry, something went wrong. |
||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Missing isolation scope context breaks request metadata attachmentThe refactored code retrieves an isolation scope via
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the scope already gets forked earlier
Comment on lines
+62
to
+72
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
+62
to
+72
This comment was marked as outdated.
Sorry, something went wrong.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is just relevant for the error case |
||
| }, | ||
| }); | ||
| } | ||
|
Comment on lines
65
to
75
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
65
to
75
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
65
to
75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The refactoring removed propagation context processing from 🔍 Detailed AnalysisThe refactoring removed logic that processes 💡 Suggested FixReintroduce the logic to parse 🤖 Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.