From e1841f29457571ba725be4ae80787998f0f9f866 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 14 Dec 2023 12:34:50 +0000 Subject: [PATCH 1/3] 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 6dd49ddc773f422e36b2791f3597aff0154a4324 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 14 Dec 2023 13:16:10 +0000 Subject: [PATCH 2/3] . --- 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 3/3] 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');