From e1841f29457571ba725be4ae80787998f0f9f866 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 14 Dec 2023 12:34:50 +0000 Subject: [PATCH 1/8] ref(nextjs): Simplify `wrapServerComponentWithSentry` --- packages/core/src/tracing/trace.ts | 12 +- .../common/wrapServerComponentWithSentry.ts | 121 ++++++------------ 2 files changed, 49 insertions(+), 84 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 667bedaaef6c..baa4ee66b28b 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -23,7 +23,9 @@ export function trace( context: TransactionContext, callback: (span?: Span) => T, // eslint-disable-next-line @typescript-eslint/no-empty-function - onError: (error: unknown) => void = () => {}, + onError: (error: unknown, span?: Span) => void = () => {}, + // eslint-disable-next-line @typescript-eslint/no-empty-function + afterFinish: () => void = () => {}, ): T { const ctx = normalizeContext(context); @@ -45,8 +47,9 @@ export function trace( maybePromiseResult = callback(activeSpan); } catch (e) { activeSpan && activeSpan.setStatus('internal_error'); - onError(e); + onError(e, activeSpan); finishAndSetSpan(); + afterFinish(); throw e; } @@ -54,15 +57,18 @@ export function trace( Promise.resolve(maybePromiseResult).then( () => { finishAndSetSpan(); + afterFinish(); }, e => { activeSpan && activeSpan.setStatus('internal_error'); - onError(e); + onError(e, activeSpan); finishAndSetSpan(); + afterFinish(); }, ); } else { finishAndSetSpan(); + afterFinish(); } return maybePromiseResult; diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index dade931bf074..b708e5e47b5d 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -1,11 +1,5 @@ -import { - addTracingExtensions, - captureException, - getCurrentHub, - runWithAsyncContext, - startTransaction, -} from '@sentry/core'; -import { tracingContextFromHeaders, winterCGHeadersToDict } from '@sentry/utils'; +import { addTracingExtensions, captureException, continueTrace, runWithAsyncContext, trace } from '@sentry/core'; +import { winterCGHeadersToDict } from '@sentry/utils'; import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; import type { ServerComponentContext } from '../common/types'; @@ -28,90 +22,55 @@ export function wrapServerComponentWithSentry any> return new Proxy(appDirComponent, { apply: (originalFunction, thisArg, args) => { return runWithAsyncContext(() => { - const hub = getCurrentHub(); - const currentScope = hub.getScope(); - - let maybePromiseResult; - const completeHeadersDict: Record = context.headers ? winterCGHeadersToDict(context.headers) : {}; - const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders( + const transactionContext = continueTrace({ // eslint-disable-next-line deprecation/deprecation - context.sentryTraceHeader ?? completeHeadersDict['sentry-trace'], + sentryTrace: context.sentryTraceHeader ?? completeHeadersDict['sentry-trace'], // eslint-disable-next-line deprecation/deprecation - context.baggageHeader ?? completeHeadersDict['baggage'], - ); - currentScope.setPropagationContext(propagationContext); - - const transaction = startTransaction({ - op: 'function.nextjs', - name: `${componentType} Server Component (${componentRoute})`, - status: 'ok', - origin: 'auto.function.nextjs', - ...traceparentData, - metadata: { - request: { - headers: completeHeadersDict, - }, - source: 'component', - dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, - }, + baggage: context.baggageHeader ?? completeHeadersDict['baggage'], }); - currentScope.setSpan(transaction); - - const handleErrorCase = (e: unknown): void => { - if (isNotFoundNavigationError(e)) { - // We don't want to report "not-found"s - transaction.setStatus('not_found'); - } else if (isRedirectNavigationError(e)) { - // We don't want to report redirects - } else { - transaction.setStatus('internal_error'); - - captureException(e, { - mechanism: { - handled: false, + const res = trace( + { + ...transactionContext, + op: 'function.nextjs', + name: `${componentType} Server Component (${componentRoute})`, + status: 'ok', + origin: 'auto.function.nextjs', + metadata: { + ...transactionContext.metadata, + request: { + headers: completeHeadersDict, }, - }); - } - - transaction.finish(); - }; - - try { - maybePromiseResult = originalFunction.apply(thisArg, args); - } catch (e) { - handleErrorCase(e); - void flushQueue(); - throw e; - } + source: 'component', + }, + }, + () => originalFunction.apply(thisArg, args), + (e, span) => { + if (isNotFoundNavigationError(e)) { + // We don't want to report "not-found"s + span.setStatus('not_found'); + } else if (isRedirectNavigationError(e)) { + // We don't want to report redirects + } else { + span.setStatus('internal_error'); - if (typeof maybePromiseResult === 'object' && maybePromiseResult !== null && 'then' in maybePromiseResult) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - Promise.resolve(maybePromiseResult) - .then( - () => { - transaction.finish(); - }, - e => { - handleErrorCase(e); - }, - ) - .finally(() => { - void flushQueue(); - }); + captureException(e, { + mechanism: { + handled: false, + }, + }); + } + }, + () => { + void flushQueue(); + }, + ); - // It is very important that we return the original promise here, because Next.js attaches various properties - // to that promise and will throw if they are not on the returned value. - return maybePromiseResult; - } else { - transaction.finish(); - void flushQueue(); - return maybePromiseResult; - } + return res; }); }, }); From 49d6e4c8aa2e7e7c1039dd14af1d92024ca22f07 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 14 Dec 2023 13:15:30 +0000 Subject: [PATCH 2/8] feat(nextjs): Connect server component transactions if there is no incoming trace --- .../src/common/utils/commonObjectTracing.ts | 23 +++++++++++++++++ .../common/wrapServerComponentWithSentry.ts | 25 ++++++++++++++++--- .../src/config/loaders/wrappingLoader.ts | 2 -- 3 files changed, 45 insertions(+), 5 deletions(-) create mode 100644 packages/nextjs/src/common/utils/commonObjectTracing.ts diff --git a/packages/nextjs/src/common/utils/commonObjectTracing.ts b/packages/nextjs/src/common/utils/commonObjectTracing.ts new file mode 100644 index 000000000000..67cc7f60731a --- /dev/null +++ b/packages/nextjs/src/common/utils/commonObjectTracing.ts @@ -0,0 +1,23 @@ +import type { PropagationContext } from '@sentry/types'; + +const commonMap = new WeakMap(); + +/** + * 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. + */ +export function commonObjectToPropagationContext( + commonObject: unknown, + propagationContext: PropagationContext, +): PropagationContext { + if (typeof commonObject === 'object' && commonObject) { + const memoTraceId = commonMap.get(commonObject); + if (memoTraceId) { + return memoTraceId; + } else { + commonMap.set(commonObject, propagationContext); + return propagationContext; + } + } else { + return propagationContext; + } +} diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index b708e5e47b5d..03785767a6c0 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -1,8 +1,16 @@ -import { addTracingExtensions, captureException, continueTrace, runWithAsyncContext, trace } from '@sentry/core'; +import { + addTracingExtensions, + captureException, + continueTrace, + getCurrentScope, + runWithAsyncContext, + trace, +} from '@sentry/core'; import { winterCGHeadersToDict } from '@sentry/utils'; import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; import type { ServerComponentContext } from '../common/types'; +import { commonObjectToPropagationContext } from './utils/commonObjectTracing'; import { flushQueue } from './utils/responseEnd'; /** @@ -33,6 +41,17 @@ export function wrapServerComponentWithSentry any> baggage: context.baggageHeader ?? completeHeadersDict['baggage'], }); + const propagationContext = getCurrentScope().getPropagationContext(); + + if (!transactionContext.traceId && !transactionContext.parentSpanId) { + const { traceId: commonTraceId, spanId: commonSpanId } = commonObjectToPropagationContext( + context.headers, + propagationContext, + ); + transactionContext.traceId = commonTraceId; + transactionContext.parentSpanId = commonSpanId; + } + const res = trace( { ...transactionContext, @@ -52,11 +71,11 @@ export function wrapServerComponentWithSentry any> (e, span) => { if (isNotFoundNavigationError(e)) { // We don't want to report "not-found"s - span.setStatus('not_found'); + span?.setStatus('not_found'); } else if (isRedirectNavigationError(e)) { // We don't want to report redirects } else { - span.setStatus('internal_error'); + span?.setStatus('internal_error'); captureException(e, { mechanism: { diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index a6b852af8b28..3d2fd7c80bb4 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -157,8 +157,6 @@ export default function wrappingLoader( .replace(/(.*)/, '/$1') // Pull off the file name .replace(/\/[^/]+\.(js|ts|jsx|tsx)$/, '') - // Remove routing groups: https://beta.nextjs.org/docs/routing/defining-routes#example-creating-multiple-root-layouts - .replace(/\/(\(.*?\)\/)+/g, '/') // In case all of the above have left us with an empty string (which will happen if we're dealing with the // homepage), sub back in the root route .replace(/^$/, '/'); From 6dd49ddc773f422e36b2791f3597aff0154a4324 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 14 Dec 2023 13:16:10 +0000 Subject: [PATCH 3/8] . --- packages/nextjs/src/common/wrapServerComponentWithSentry.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index b708e5e47b5d..7d16c5eb23b2 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -52,11 +52,11 @@ export function wrapServerComponentWithSentry any> (e, span) => { if (isNotFoundNavigationError(e)) { // We don't want to report "not-found"s - span.setStatus('not_found'); + span?.setStatus('not_found'); } else if (isRedirectNavigationError(e)) { // We don't want to report redirects } else { - span.setStatus('internal_error'); + span?.setStatus('internal_error'); captureException(e, { mechanism: { From 9e8e0aa3c38568d1b0841c5c353fa59556e23ae5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 18 Dec 2023 10:16:06 +0000 Subject: [PATCH 4/8] Set status to ok on redirect --- packages/nextjs/src/common/wrapServerComponentWithSentry.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index 7d16c5eb23b2..d7ff31f3afd9 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -55,6 +55,8 @@ export function wrapServerComponentWithSentry any> span?.setStatus('not_found'); } else if (isRedirectNavigationError(e)) { // We don't want to report redirects + // Since `trace` will automatically set the span status to "internal_error" we need to set it back to "ok" + span?.setStatus('ok'); } else { span?.setStatus('internal_error'); From 3618f73f1ba40cdcb01f11deaecef4078e3f73c5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 18 Dec 2023 10:58:36 +0000 Subject: [PATCH 5/8] Connect generation functions --- .../src/common/wrapGenerationFunctionWithSentry.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index 5aa9c436beef..d97302c2f530 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -3,6 +3,7 @@ import { captureException, continueTrace, getCurrentHub, + getCurrentScope, runWithAsyncContext, trace, } from '@sentry/core'; @@ -10,6 +11,7 @@ import type { WebFetchHeaders } from '@sentry/types'; import { winterCGHeadersToDict } from '@sentry/utils'; import type { GenerationFunctionContext } from '../common/types'; +import { commonObjectToPropagationContext } from './utils/commonObjectTracing'; /** * Wraps a generation function (e.g. generateMetadata) with Sentry error and performance instrumentation. @@ -45,6 +47,18 @@ export function wrapGenerationFunctionWithSentry a baggage: headers?.get('baggage'), sentryTrace: headers?.get('sentry-trace') ?? undefined, }); + + const propagationContext = getCurrentScope().getPropagationContext(); + + if (!transactionContext.traceId && !transactionContext.parentSpanId) { + const { traceId: commonTraceId, spanId: commonSpanId } = commonObjectToPropagationContext( + headers, + propagationContext, + ); + transactionContext.traceId = commonTraceId; + transactionContext.parentSpanId = commonSpanId; + } + return trace( { op: 'function.nextjs', From 1ae5c147cfbefc80ee9abd9aadb8c4f407c6e7c1 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 18 Dec 2023 11:52:46 +0000 Subject: [PATCH 6/8] Add comment explaining. --- packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts | 3 ++- packages/nextjs/src/common/wrapServerComponentWithSentry.ts | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index d97302c2f530..80f7d62cc447 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -48,8 +48,9 @@ export function wrapGenerationFunctionWithSentry a sentryTrace: headers?.get('sentry-trace') ?? undefined, }); + // If there is no incoming trace, we are setting the transaction context to one that is shared between all other + // transactions for this request. We do this based on the `headers` object, which is the same for all components. const propagationContext = getCurrentScope().getPropagationContext(); - if (!transactionContext.traceId && !transactionContext.parentSpanId) { const { traceId: commonTraceId, spanId: commonSpanId } = commonObjectToPropagationContext( headers, diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index 1d911f56cc59..8312121ae12c 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -41,8 +41,9 @@ export function wrapServerComponentWithSentry any> baggage: context.baggageHeader ?? completeHeadersDict['baggage'], }); + // If there is no incoming trace, we are setting the transaction context to one that is shared between all other + // transactions for this request. We do this based on the `headers` object, which is the same for all components. const propagationContext = getCurrentScope().getPropagationContext(); - if (!transactionContext.traceId && !transactionContext.parentSpanId) { const { traceId: commonTraceId, spanId: commonSpanId } = commonObjectToPropagationContext( context.headers, From 9b871f3015d76c4fccce1298c7df1f0ef35be2a4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 18 Dec 2023 12:47:19 +0000 Subject: [PATCH 7/8] Add e2e test --- .../app/(nested-layout)/layout.tsx | 12 +++++ .../(nested-layout)/nested-layout/layout.tsx | 12 +++++ .../(nested-layout)/nested-layout/page.tsx | 11 ++++ .../connected-servercomponent-trace.test.ts | 50 +++++++++++++++++++ 4 files changed, 85 insertions(+) create mode 100644 packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/layout.tsx create mode 100644 packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/nested-layout/layout.tsx create mode 100644 packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/nested-layout/page.tsx create mode 100644 packages/e2e-tests/test-applications/nextjs-app-dir/tests/connected-servercomponent-trace.test.ts diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/layout.tsx b/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/layout.tsx new file mode 100644 index 000000000000..ace0c2f086b7 --- /dev/null +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/layout.tsx @@ -0,0 +1,12 @@ +import { PropsWithChildren } from 'react'; + +export const dynamic = 'force-dynamic'; + +export default function Layout({ children }: PropsWithChildren<{}>) { + return ( +
+

Layout

+ {children} +
+ ); +} diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/nested-layout/layout.tsx b/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/nested-layout/layout.tsx new file mode 100644 index 000000000000..ace0c2f086b7 --- /dev/null +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/nested-layout/layout.tsx @@ -0,0 +1,12 @@ +import { PropsWithChildren } from 'react'; + +export const dynamic = 'force-dynamic'; + +export default function Layout({ children }: PropsWithChildren<{}>) { + return ( +
+

Layout

+ {children} +
+ ); +} diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/nested-layout/page.tsx b/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/nested-layout/page.tsx new file mode 100644 index 000000000000..8077c14d23ca --- /dev/null +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/app/(nested-layout)/nested-layout/page.tsx @@ -0,0 +1,11 @@ +export const dynamic = 'force-dynamic'; + +export default function Page() { + return

Hello World!

; +} + +export async function generateMetadata() { + return { + title: 'I am generated metadata', + }; +} diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/tests/connected-servercomponent-trace.test.ts b/packages/e2e-tests/test-applications/nextjs-app-dir/tests/connected-servercomponent-trace.test.ts new file mode 100644 index 000000000000..4acc41814d3c --- /dev/null +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/tests/connected-servercomponent-trace.test.ts @@ -0,0 +1,50 @@ +import { expect, test } from '@playwright/test'; +import { waitForTransaction } from '../event-proxy-server'; + +test('Will capture a connected trace for all server components and generation functions when visiting a page', async ({ + page, +}) => { + const someConnectedEvent = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return ( + transactionEvent?.transaction === 'Layout Server Component (/(nested-layout)/nested-layout)' || + transactionEvent?.transaction === 'Layout Server Component (/(nested-layout))' || + transactionEvent?.transaction === 'Page Server Component (/(nested-layout)/nested-layout)' || + transactionEvent?.transaction === 'Page.generateMetadata (/(nested-layout)/nested-layout)' + ); + }); + + const layout1Transaction = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return ( + transactionEvent?.transaction === 'Layout Server Component (/(nested-layout)/nested-layout)' && + (await someConnectedEvent).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id + ); + }); + + const layout2Transaction = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return ( + transactionEvent?.transaction === 'Layout Server Component (/(nested-layout))' && + (await someConnectedEvent).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id + ); + }); + + const pageTransaction = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return ( + transactionEvent?.transaction === 'Page Server Component (/(nested-layout)/nested-layout)' && + (await someConnectedEvent).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id + ); + }); + + const generateMetadataTransaction = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return ( + transactionEvent?.transaction === 'Page.generateMetadata (/(nested-layout)/nested-layout)' && + (await someConnectedEvent).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id + ); + }); + + await page.goto('/nested-layout'); + + expect(await layout1Transaction).toBeDefined(); + expect(await layout2Transaction).toBeDefined(); + expect(await pageTransaction).toBeDefined(); + expect(await generateMetadataTransaction).toBeDefined(); +}); From edcda0d1fdad1c50085166cfb9ecb4aa4f756b3a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 18 Dec 2023 14:24:40 +0000 Subject: [PATCH 8/8] Pr feedback --- packages/nextjs/src/common/utils/commonObjectTracing.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/common/utils/commonObjectTracing.ts b/packages/nextjs/src/common/utils/commonObjectTracing.ts index 67cc7f60731a..bb5cf130bab1 100644 --- a/packages/nextjs/src/common/utils/commonObjectTracing.ts +++ b/packages/nextjs/src/common/utils/commonObjectTracing.ts @@ -10,9 +10,9 @@ export function commonObjectToPropagationContext( propagationContext: PropagationContext, ): PropagationContext { if (typeof commonObject === 'object' && commonObject) { - const memoTraceId = commonMap.get(commonObject); - if (memoTraceId) { - return memoTraceId; + const memoPropagationContext = commonMap.get(commonObject); + if (memoPropagationContext) { + return memoPropagationContext; } else { commonMap.set(commonObject, propagationContext); return propagationContext;