From 052cc544c599d644e212d404328909add2f838ff Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Tue, 22 Oct 2024 10:21:57 +0200 Subject: [PATCH 1/2] combine server action transactions --- .../common/withServerActionInstrumentation.ts | 152 +++++++++--------- 1 file changed, 74 insertions(+), 78 deletions(-) diff --git a/packages/nextjs/src/common/withServerActionInstrumentation.ts b/packages/nextjs/src/common/withServerActionInstrumentation.ts index d1a37a7cab76..b13d3ebef3dd 100644 --- a/packages/nextjs/src/common/withServerActionInstrumentation.ts +++ b/packages/nextjs/src/common/withServerActionInstrumentation.ts @@ -14,7 +14,6 @@ import { logger, vercelWaitUntil } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils'; import { flushSafelyWithTimeout } from './utils/responseEnd'; -import { dropNextjsRootContext, escapeNextjsTracing } from './utils/tracingUtils'; interface Options { formData?: FormData; @@ -68,89 +67,86 @@ async function withServerActionInstrumentationImplementation> { - dropNextjsRootContext(); - return escapeNextjsTracing(() => { - return withIsolationScope(async isolationScope => { - const sendDefaultPii = getClient()?.getOptions().sendDefaultPii; + return withIsolationScope(async isolationScope => { + const sendDefaultPii = getClient()?.getOptions().sendDefaultPii; - let sentryTraceHeader; - let baggageHeader; - const fullHeadersObject: Record = {}; - try { - const awaitedHeaders: Headers = await options.headers; - sentryTraceHeader = awaitedHeaders?.get('sentry-trace') ?? undefined; - baggageHeader = awaitedHeaders?.get('baggage'); - awaitedHeaders?.forEach((value, key) => { - fullHeadersObject[key] = value; - }); - } catch (e) { - DEBUG_BUILD && - logger.warn( - "Sentry wasn't able to extract the tracing headers for a server action. Will not trace this request.", - ); - } - - isolationScope.setTransactionName(`serverAction/${serverActionName}`); - isolationScope.setSDKProcessingMetadata({ - request: { - headers: fullHeadersObject, - }, + let sentryTraceHeader; + let baggageHeader; + const fullHeadersObject: Record = {}; + try { + const awaitedHeaders: Headers = await options.headers; + sentryTraceHeader = awaitedHeaders?.get('sentry-trace') ?? undefined; + baggageHeader = awaitedHeaders?.get('baggage'); + awaitedHeaders?.forEach((value, key) => { + fullHeadersObject[key] = value; }); + } catch (e) { + DEBUG_BUILD && + logger.warn( + "Sentry wasn't able to extract the tracing headers for a server action. Will not trace this request.", + ); + } - return continueTrace( - { - sentryTrace: sentryTraceHeader, - baggage: baggageHeader, - }, - async () => { - try { - return await startSpan( - { - op: 'function.server_action', - name: `serverAction/${serverActionName}`, - forceTransaction: true, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - }, - }, - async span => { - const result = await handleCallbackErrors(callback, error => { - 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)) { - // Don't do anything for redirects - } else { - span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); - captureException(error, { - mechanism: { - handled: false, - }, - }); - } - }); - - if (options.recordResponse !== undefined ? options.recordResponse : sendDefaultPii) { - getIsolationScope().setExtra('server_action_result', result); - } + isolationScope.setTransactionName(`serverAction/${serverActionName}`); + isolationScope.setSDKProcessingMetadata({ + request: { + headers: fullHeadersObject, + }, + }); - if (options.formData) { - options.formData.forEach((value, key) => { - getIsolationScope().setExtra( - `server_action_form_data.${key}`, - typeof value === 'string' ? value : '[non-string value]', - ); + return continueTrace( + { + sentryTrace: sentryTraceHeader, + baggage: baggageHeader, + }, + async () => { + try { + return await startSpan( + { + op: 'function.server_action', + name: `serverAction/${serverActionName}`, + forceTransaction: true, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + }, + async span => { + const result = await handleCallbackErrors(callback, error => { + 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)) { + // Don't do anything for redirects + } else { + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + captureException(error, { + mechanism: { + handled: false, + }, }); } + }); - return result; - }, - ); - } finally { - vercelWaitUntil(flushSafelyWithTimeout()); - } - }, - ); - }); + if (options.recordResponse !== undefined ? options.recordResponse : sendDefaultPii) { + getIsolationScope().setExtra('server_action_result', result); + } + + if (options.formData) { + options.formData.forEach((value, key) => { + getIsolationScope().setExtra( + `server_action_form_data.${key}`, + typeof value === 'string' ? value : '[non-string value]', + ); + }); + } + + return result; + }, + ); + } finally { + vercelWaitUntil(flushSafelyWithTimeout()); + } + }, + ); }); } From b3b89ae6353159f4d52b2945ce67362403f1b53e Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Tue, 22 Oct 2024 14:32:17 +0200 Subject: [PATCH 2/2] add e2e test for checking span order --- .../nextjs-app-dir/tests/transactions.test.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/transactions.test.ts index 8d2489bab34d..278b6b1074eb 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/transactions.test.ts @@ -76,6 +76,34 @@ test('Should send a transaction for instrumented server actions', async ({ page expect(Object.keys(transactionEvent.request?.headers || {}).length).toBeGreaterThan(0); }); +test('Should send a wrapped server action as a child of a nextjs transaction', async ({ page }) => { + const nextjsVersion = packageJson.dependencies.next; + const nextjsMajor = Number(nextjsVersion.split('.')[0]); + test.skip(!isNaN(nextjsMajor) && nextjsMajor < 14, 'only applies to nextjs apps >= version 14'); + test.skip(process.env.TEST_ENV === 'development', 'this magically only works in production'); + + const nextjsPostTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { + return ( + transactionEvent?.transaction === 'POST /server-action' && transactionEvent.contexts?.trace?.origin === 'auto' + ); + }); + + const serverActionTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { + return transactionEvent?.transaction === 'serverAction/myServerAction'; + }); + + await page.goto('/server-action'); + await page.getByText('Run Action').click(); + + const nextjsTransaction = await nextjsPostTransactionPromise; + const serverActionTransaction = await serverActionTransactionPromise; + + expect(nextjsTransaction).toBeDefined(); + expect(serverActionTransaction).toBeDefined(); + + expect(nextjsTransaction.contexts?.trace?.span_id).toBe(serverActionTransaction.contexts?.trace?.parent_span_id); +}); + test('Should set not_found status for server actions calling notFound()', async ({ page }) => { const nextjsVersion = packageJson.dependencies.next; const nextjsMajor = Number(nextjsVersion.split('.')[0]);