From 7034d964cbcaad3dd6a1be60faeb82312d20fbfe Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 2 Jul 2024 13:13:42 +0000 Subject: [PATCH 01/21] this will break in a million ways --- .../wrapGenerationFunctionWithSentry.ts | 147 ++++++++---------- .../common/wrapServerComponentWithSentry.ts | 118 +++++++------- packages/nextjs/src/server/index.ts | 29 +++- 3 files changed, 144 insertions(+), 150 deletions(-) diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index 0c76821afa28..15b20eef6d8d 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -5,22 +5,19 @@ import { captureException, getActiveSpan, getClient, + getRootSpan, handleCallbackErrors, startSpanManual, withIsolationScope, withScope, } from '@sentry/core'; import type { WebFetchHeaders } from '@sentry/types'; -import { propagationContextFromHeaders, uuid4, winterCGHeadersToDict } from '@sentry/utils'; +import { winterCGHeadersToDict } from '@sentry/utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import type { GenerationFunctionContext } from '../common/types'; import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils'; -import { - commonObjectToIsolationScope, - commonObjectToPropagationContext, - escapeNextjsTracing, -} from './utils/tracingUtils'; +import { commonObjectToIsolationScope } from './utils/tracingUtils'; /** * Wraps a generation function (e.g. generateMetadata) with Sentry error and performance instrumentation. @@ -33,88 +30,80 @@ export function wrapGenerationFunctionWithSentry a const { requestAsyncStorage, componentRoute, componentType, generationFunctionIdentifier } = context; return new Proxy(generationFunction, { apply: (originalFunction, thisArg, args) => { - const requestTraceId = getActiveSpan()?.spanContext().traceId; - return escapeNextjsTracing(() => { - let headers: WebFetchHeaders | undefined = undefined; - // We try-catch here just in case anything goes wrong with the async storage here goes wrong since it is Next.js internal API - try { - headers = requestAsyncStorage?.getStore()?.headers; - } catch (e) { - /** empty */ - } + const activeSpan = getActiveSpan(); + if (activeSpan) { + getRootSpan(activeSpan).setAttribute('sentry.rsc', true); + } - let data: Record | undefined = undefined; - if (getClient()?.getOptions().sendDefaultPii) { - const props: unknown = args[0]; - const params = props && typeof props === 'object' && 'params' in props ? props.params : undefined; - const searchParams = - props && typeof props === 'object' && 'searchParams' in props ? props.searchParams : undefined; - data = { params, searchParams }; - } + let headers: WebFetchHeaders | undefined = undefined; + // We try-catch here just in case anything goes wrong with the async storage here goes wrong since it is Next.js internal API + try { + headers = requestAsyncStorage?.getStore()?.headers; + } catch (e) { + /** empty */ + } - const headersDict = headers ? winterCGHeadersToDict(headers) : undefined; + let data: Record | undefined = undefined; + if (getClient()?.getOptions().sendDefaultPii) { + const props: unknown = args[0]; + const params = props && typeof props === 'object' && 'params' in props ? props.params : undefined; + const searchParams = + props && typeof props === 'object' && 'searchParams' in props ? props.searchParams : undefined; + data = { params, searchParams }; + } - const isolationScope = commonObjectToIsolationScope(headers); + const headersDict = headers ? winterCGHeadersToDict(headers) : undefined; - return withIsolationScope(isolationScope, () => { - return withScope(scope => { - scope.setTransactionName(`${componentType}.${generationFunctionIdentifier} (${componentRoute})`); + const isolationScope = commonObjectToIsolationScope(headers); - isolationScope.setSDKProcessingMetadata({ - request: { - headers: headersDict, - }, - }); + return withIsolationScope(isolationScope, () => { + return withScope(scope => { + scope.setTransactionName(`${componentType}.${generationFunctionIdentifier} (${componentRoute})`); - const propagationContext = commonObjectToPropagationContext( - headers, - headersDict?.['sentry-trace'] - ? propagationContextFromHeaders(headersDict['sentry-trace'], headersDict['baggage']) - : { - traceId: requestTraceId || uuid4(), - spanId: uuid4().substring(16), - }, - ); + isolationScope.setSDKProcessingMetadata({ + request: { + headers: headersDict, + }, + }); - scope.setExtra('route_data', data); - scope.setPropagationContext(propagationContext); + scope.setExtra('route_data', data); - return startSpanManual( - { - op: 'function.nextjs', - name: `${componentType}.${generationFunctionIdentifier} (${componentRoute})`, - forceTransaction: true, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs', - }, + return startSpanManual( + { + op: 'function.nextjs', + name: `${componentType}.${generationFunctionIdentifier} (${componentRoute})`, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs', }, - span => { - return handleCallbackErrors( - () => originalFunction.apply(thisArg, args), - err => { - if (isNotFoundNavigationError(err)) { - // We don't want to report "not-found"s - span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); - } else if (isRedirectNavigationError(err)) { - // We don't want to report redirects - span.setStatus({ code: SPAN_STATUS_OK }); - } else { - span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); - captureException(err, { - mechanism: { - handled: false, - }, - }); - } - }, - () => { - span.end(); - }, - ); - }, - ); - }); + }, + span => { + return handleCallbackErrors( + () => originalFunction.apply(thisArg, args), + err => { + if (isNotFoundNavigationError(err)) { + // We don't want to report "not-found"s + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); + getRootSpan(span).setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); + } else if (isRedirectNavigationError(err)) { + // We don't want to report redirects + span.setStatus({ code: SPAN_STATUS_OK }); + } else { + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + getRootSpan(span).setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + captureException(err, { + mechanism: { + handled: false, + }, + }); + } + }, + () => { + span.end(); + }, + ); + }, + ); }); }); }, diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index 0d1e224bdf47..0a06d5983c89 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -4,22 +4,19 @@ import { SPAN_STATUS_OK, captureException, getActiveSpan, + getRootSpan, handleCallbackErrors, startSpanManual, withIsolationScope, withScope, } from '@sentry/core'; -import { propagationContextFromHeaders, uuid4, winterCGHeadersToDict } from '@sentry/utils'; +import { winterCGHeadersToDict } from '@sentry/utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; import type { ServerComponentContext } from '../common/types'; import { flushSafelyWithTimeout } from './utils/responseEnd'; -import { - commonObjectToIsolationScope, - commonObjectToPropagationContext, - escapeNextjsTracing, -} from './utils/tracingUtils'; +import { commonObjectToIsolationScope } from './utils/tracingUtils'; import { vercelWaitUntil } from './utils/vercelWaitUntil'; /** @@ -36,70 +33,61 @@ export function wrapServerComponentWithSentry any> // hook. 🤯 return new Proxy(appDirComponent, { apply: (originalFunction, thisArg, args) => { - const requestTraceId = getActiveSpan()?.spanContext().traceId; - return escapeNextjsTracing(() => { - const isolationScope = commonObjectToIsolationScope(context.headers); - - const headersDict = context.headers ? winterCGHeadersToDict(context.headers) : undefined; + const activeSpan = getActiveSpan(); + if (activeSpan) { + getRootSpan(activeSpan).setAttribute('sentry.rsc', true); + } - isolationScope.setSDKProcessingMetadata({ - request: { - headers: headersDict, - }, - }); + const isolationScope = commonObjectToIsolationScope(context.headers); - return withIsolationScope(isolationScope, () => { - return withScope(scope => { - scope.setTransactionName(`${componentType} Server Component (${componentRoute})`); + const headersDict = context.headers ? winterCGHeadersToDict(context.headers) : undefined; - const propagationContext = commonObjectToPropagationContext( - context.headers, - headersDict?.['sentry-trace'] - ? propagationContextFromHeaders(headersDict['sentry-trace'], headersDict['baggage']) - : { - traceId: requestTraceId || uuid4(), - spanId: uuid4().substring(16), - }, - ); + isolationScope.setSDKProcessingMetadata({ + request: { + headers: headersDict, + }, + }); - scope.setPropagationContext(propagationContext); - return startSpanManual( - { - op: 'function.nextjs', - name: `${componentType} Server Component (${componentRoute})`, - forceTransaction: true, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs', - }, - }, - span => { - return handleCallbackErrors( - () => originalFunction.apply(thisArg, args), - 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)) { - // 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, - }, - }); - } - }, - () => { - span.end(); - vercelWaitUntil(flushSafelyWithTimeout()); - }, - ); + return withIsolationScope(isolationScope, () => { + return withScope(scope => { + scope.setTransactionName(`${componentType} Server Component (${componentRoute})`); + return startSpanManual( + { + op: 'function.nextjs', + name: `${componentType} Server Component (${componentRoute})`, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs', }, - ); - }); + }, + span => { + return handleCallbackErrors( + () => originalFunction.apply(thisArg, args), + error => { + if (isNotFoundNavigationError(error)) { + // We don't want to report "not-found"s + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); + getRootSpan(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' }); + getRootSpan(span).setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + captureException(error, { + mechanism: { + handled: false, + }, + }); + } + }, + () => { + span.end(); + vercelWaitUntil(flushSafelyWithTimeout()); + }, + ); + }, + ); }); }); }, diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 741c4092c61b..24cd8706b08e 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -3,15 +3,15 @@ import { getDefaultIntegrations, init as nodeInit } from '@sentry/node'; import type { NodeClient, NodeOptions } from '@sentry/node'; import { GLOBAL_OBJ, logger } from '@sentry/utils'; +import type { EventProcessor } from '@sentry/types'; import { DEBUG_BUILD } from '../common/debug-build'; import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolicationEventProcessor'; import { getVercelEnv } from '../common/getVercelEnv'; import { isBuild } from '../common/utils/isBuild'; import { distDirRewriteFramesIntegration } from './distDirRewriteFramesIntegration'; +import { httpIntegration } from './httpIntegration'; export * from '@sentry/node'; -import type { EventProcessor } from '@sentry/types'; -import { httpIntegration } from './httpIntegration'; export { httpIntegration }; @@ -129,11 +129,18 @@ export function init(options: NodeOptions): NodeClient | undefined { const client = nodeInit(opts); + // If we encounter a span emitted by Next.js, we do not want to sample it + // The reason for this is that the data quality of the spans varies, it is different per version of Next, + // and we need to keep our manual instrumentation around for the edge runtime anyhow. + // BUT we only do this if we don't have a parent span with a sampling decision yet (or if the parent is remote) client?.on('beforeSampling', ({ spanAttributes, spanName, parentSampled, parentContext }, samplingDecision) => { - // If we encounter a span emitted by Next.js, we do not want to sample it - // The reason for this is that the data quality of the spans varies, it is different per version of Next, - // and we need to keep our manual instrumentation around for the edge runtime anyhow. - // BUT we only do this if we don't have a parent span with a sampling decision yet (or if the parent is remote) + // We "whitelist" the "BaseServer.handleRequest" span, since that one is responsible for App Router requests, which are actually useful for us. + // HOWEVER, that span is not only responsible for App Router requests, which is why we additionally filter for certain transactions in an + // event processor further below. + if (spanAttributes['next.span_type'] === 'BaseServer.handleRequest') { + return; + } + if ( (spanAttributes['next.span_type'] || NEXTJS_SPAN_NAME_PREFIXES.some(prefix => spanName.startsWith(prefix))) && (parentSampled === undefined || parentContext?.isRemote) @@ -153,6 +160,16 @@ export function init(options: NodeOptions): NodeClient | undefined { return null; } + // 'BaseServer.handleRequest' spans are the only Next.js spans we sample. However, we only want to actually keep these spans/transactions, + // when they server RSCs (app router routes). We mark these transactions with a "sentry.rsc" attribute in our RSC wrappers so we can + // filter them here. + if ( + event.contexts?.trace?.data?.['next.span_type'] === 'BaseServer.handleRequest' && + event.contexts?.trace?.data?.['sentry.rsc'] !== true + ) { + return null; + } + // Filter out transactions for requests to the tunnel route if ( globalWithInjectedValues.__sentryRewritesTunnelPath__ && From c4fc30b8c402a4bf0270a412d8b6b1d3213c4a0b Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 2 Jul 2024 15:15:22 +0000 Subject: [PATCH 02/21] Apply isolation scope to transaction --- .../common/wrapGenerationFunctionWithSentry.ts | 18 +++++++++++------- .../common/wrapServerComponentWithSentry.ts | 10 +++++++--- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index 15b20eef6d8d..361c6a646a49 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -2,11 +2,13 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, SPAN_STATUS_ERROR, SPAN_STATUS_OK, + Scope, captureException, getActiveSpan, getClient, getRootSpan, handleCallbackErrors, + setCapturedScopesOnSpan, startSpanManual, withIsolationScope, withScope, @@ -30,11 +32,6 @@ export function wrapGenerationFunctionWithSentry a const { requestAsyncStorage, componentRoute, componentType, generationFunctionIdentifier } = context; return new Proxy(generationFunction, { apply: (originalFunction, thisArg, args) => { - const activeSpan = getActiveSpan(); - if (activeSpan) { - getRootSpan(activeSpan).setAttribute('sentry.rsc', true); - } - let headers: WebFetchHeaders | undefined = undefined; // We try-catch here just in case anything goes wrong with the async storage here goes wrong since it is Next.js internal API try { @@ -43,6 +40,15 @@ export function wrapGenerationFunctionWithSentry a /** empty */ } + const isolationScope = commonObjectToIsolationScope(headers); + + const activeSpan = getActiveSpan(); + if (activeSpan) { + const rootSpan = getRootSpan(activeSpan); + rootSpan.setAttribute('sentry.rsc', true); + setCapturedScopesOnSpan(rootSpan, new Scope(), isolationScope); + } + let data: Record | undefined = undefined; if (getClient()?.getOptions().sendDefaultPii) { const props: unknown = args[0]; @@ -54,8 +60,6 @@ export function wrapGenerationFunctionWithSentry a const headersDict = headers ? winterCGHeadersToDict(headers) : undefined; - const isolationScope = commonObjectToIsolationScope(headers); - return withIsolationScope(isolationScope, () => { return withScope(scope => { scope.setTransactionName(`${componentType}.${generationFunctionIdentifier} (${componentRoute})`); diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index 0a06d5983c89..dc09c42d2665 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -2,10 +2,12 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, SPAN_STATUS_ERROR, SPAN_STATUS_OK, + Scope, captureException, getActiveSpan, getRootSpan, handleCallbackErrors, + setCapturedScopesOnSpan, startSpanManual, withIsolationScope, withScope, @@ -33,13 +35,15 @@ export function wrapServerComponentWithSentry any> // hook. 🤯 return new Proxy(appDirComponent, { apply: (originalFunction, thisArg, args) => { + const isolationScope = commonObjectToIsolationScope(context.headers); + const activeSpan = getActiveSpan(); if (activeSpan) { - getRootSpan(activeSpan).setAttribute('sentry.rsc', true); + const rootSpan = getRootSpan(activeSpan); + rootSpan.setAttribute('sentry.rsc', true); + setCapturedScopesOnSpan(rootSpan, new Scope(), isolationScope); } - const isolationScope = commonObjectToIsolationScope(context.headers); - const headersDict = context.headers ? winterCGHeadersToDict(context.headers) : undefined; isolationScope.setSDKProcessingMetadata({ From e4b3f1d87ce21521eeef96d5531a333f09c9d48d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 2 Jul 2024 15:16:36 +0000 Subject: [PATCH 03/21] Fix generation function tests --- .../tests/generation-functions.test.ts | 53 +++++++++++++------ 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/generation-functions.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/generation-functions.test.ts index 7739e9ac17de..303582ec1b24 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/generation-functions.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/generation-functions.test.ts @@ -1,19 +1,29 @@ import { expect, test } from '@playwright/test'; import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; -test('Should send a transaction event for a generateMetadata() function invokation', async ({ page }) => { - const testTitle = 'foobarasdf'; +test('Should emit a span for a generateMetadata() function invokation', async ({ page }) => { + const testTitle = 'should-emit-span'; const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => { return ( - transactionEvent?.transaction === 'Page.generateMetadata (/generation-functions)' && - (transactionEvent.extra?.route_data as any)?.searchParams?.metadataTitle === testTitle + transactionEvent.contexts?.trace?.data?.['http.target'] === `/generation-functions?metadataTitle=${testTitle}` ); }); await page.goto(`/generation-functions?metadataTitle=${testTitle}`); - expect(await transactionPromise).toBeDefined(); + const transaction = await transactionPromise; + + expect(transaction.spans).toContainEqual( + expect.objectContaining({ + description: 'generateMetadata /generation-functions/page', + origin: 'manual', + parent_span_id: expect.any(String), + span_id: expect.any(String), + status: 'ok', + trace_id: expect.any(String), + }), + ); const pageTitle = await page.title(); expect(pageTitle).toBe(testTitle); @@ -22,12 +32,12 @@ test('Should send a transaction event for a generateMetadata() function invokati test('Should send a transaction and an error event for a faulty generateMetadata() function invokation', async ({ page, }) => { - const testTitle = 'foobarbaz'; + const testTitle = 'should-emit-error'; const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => { return ( - transactionEvent.transaction === 'Page.generateMetadata (/generation-functions)' && - (transactionEvent.extra?.route_data as any)?.searchParams?.metadataTitle === testTitle + transactionEvent.contexts?.trace?.data?.['http.target'] === + `/generation-functions?metadataTitle=${testTitle}&shouldThrowInGenerateMetadata=1` ); }); @@ -54,14 +64,23 @@ test('Should send a transaction event for a generateViewport() function invokati const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => { return ( - transactionEvent?.transaction === 'Page.generateViewport (/generation-functions)' && - (transactionEvent.extra?.route_data as any)?.searchParams?.viewportThemeColor === testTitle + transactionEvent.contexts?.trace?.data?.['http.target'] === + `/generation-functions?viewportThemeColor=${testTitle}` ); }); await page.goto(`/generation-functions?viewportThemeColor=${testTitle}`); - expect(await transactionPromise).toBeDefined(); + expect((await transactionPromise).spans).toContainEqual( + expect.objectContaining({ + description: 'generateViewport /generation-functions/page', + origin: 'manual', + parent_span_id: expect.any(String), + span_id: expect.any(String), + status: 'ok', + trace_id: expect.any(String), + }), + ); }); test('Should send a transaction and an error event for a faulty generateViewport() function invokation', async ({ @@ -71,8 +90,8 @@ test('Should send a transaction and an error event for a faulty generateViewport const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => { return ( - transactionEvent?.transaction === 'Page.generateViewport (/generation-functions)' && - (transactionEvent.extra?.route_data as any)?.searchParams?.viewportThemeColor === testTitle + transactionEvent.contexts?.trace?.data?.['http.target'] === + `/generation-functions?viewportThemeColor=${testTitle}&shouldThrowInGenerateViewport=1` ); }); @@ -97,8 +116,8 @@ test('Should send a transaction event with correct status for a generateMetadata const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => { return ( - transactionEvent?.transaction === 'Page.generateMetadata (/generation-functions/with-redirect)' && - (transactionEvent.extra?.route_data as any)?.searchParams?.metadataTitle === testTitle + transactionEvent.contexts?.trace?.data?.['http.target'] === + `/generation-functions/with-redirect?metadataTitle=${testTitle}` ); }); @@ -114,8 +133,8 @@ test('Should send a transaction event with correct status for a generateMetadata const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => { return ( - transactionEvent?.transaction === 'Page.generateMetadata (/generation-functions/with-notfound)' && - (transactionEvent.extra?.route_data as any)?.searchParams?.metadataTitle === testTitle + transactionEvent.contexts?.trace?.data?.['http.target'] === + `/generation-functions/with-notfound?metadataTitle=${testTitle}` ); }); From 5bd6e0bb501424779175f8940df9d54e27842ed2 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 2 Jul 2024 15:18:55 +0000 Subject: [PATCH 04/21] Fix request instrumentation tests --- .../nextjs-14/tests/request-instrumentation.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts index be6bfab11b84..061c9d3cc5d6 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts @@ -3,7 +3,7 @@ import { waitForTransaction } from '@sentry-internal/test-utils'; test('Should send a transaction with a fetch span', async ({ page }) => { const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/request-instrumentation)'; + return transactionEvent?.transaction === 'GET /request-instrumentation'; }); await page.goto(`/request-instrumentation`); From d35762ae0bb28fee7be228fc58a7a819804c6703 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 2 Jul 2024 15:34:43 +0000 Subject: [PATCH 05/21] Fix next-15 e2e tests --- .../nextjs-15/tests/pageload-tracing.test.ts | 27 +++++-------------- .../nextjs-15/tests/ppr-error.test.ts | 9 ++++--- .../nextjs-15/tests/suspense-error.test.ts | 7 ++--- 3 files changed, 17 insertions(+), 26 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/pageload-tracing.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/pageload-tracing.test.ts index 38325fa6a0e0..9c48408474fd 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/pageload-tracing.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/pageload-tracing.test.ts @@ -2,16 +2,8 @@ import { expect, test } from '@playwright/test'; import { waitForTransaction } from '@sentry-internal/test-utils'; test('all server component transactions should be attached to the pageload request span', async ({ page }) => { - const pageServerComponentTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/pageload-tracing)'; - }); - - const layoutServerComponentTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { - return transactionEvent?.transaction === 'Layout Server Component (/pageload-tracing)'; - }); - - const metadataTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { - return transactionEvent?.transaction === 'Page.generateMetadata (/pageload-tracing)'; + const serverTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { + return transactionEvent?.transaction === 'GET /pageload-tracing'; }); const pageloadTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { @@ -20,18 +12,13 @@ test('all server component transactions should be attached to the pageload reque await page.goto(`/pageload-tracing`); - const [pageServerComponentTransaction, layoutServerComponentTransaction, metadataTransaction, pageloadTransaction] = - await Promise.all([ - pageServerComponentTransactionPromise, - layoutServerComponentTransactionPromise, - metadataTransactionPromise, - pageloadTransactionPromise, - ]); + const [serverTransaction, pageloadTransaction] = await Promise.all([ + serverTransactionPromise, + pageloadTransactionPromise, + ]); const pageloadTraceId = pageloadTransaction.contexts?.trace?.trace_id; expect(pageloadTraceId).toBeTruthy(); - expect(pageServerComponentTransaction.contexts?.trace?.trace_id).toBe(pageloadTraceId); - expect(layoutServerComponentTransaction.contexts?.trace?.trace_id).toBe(pageloadTraceId); - expect(metadataTransaction.contexts?.trace?.trace_id).toBe(pageloadTraceId); + expect(serverTransaction.contexts?.trace?.trace_id).toBe(pageloadTraceId); }); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/ppr-error.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/ppr-error.test.ts index 6fc1a6716127..00ad3b4a3e03 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/ppr-error.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/ppr-error.test.ts @@ -3,18 +3,21 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; test('should not capture React-internal errors for PPR rendering', async ({ page }) => { const pageServerComponentTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/ppr-error/[param])'; + return transactionEvent?.transaction === 'GET /ppr-error/[param]'; }); let errorEventReceived = false; - waitForError('nextjs-15', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/ppr-error/[param])'; + waitForError('nextjs-15', async errorEvent => { + return errorEvent?.transaction === 'Page Server Component (/ppr-error/[param])'; }).then(() => { errorEventReceived = true; }); await page.goto(`/ppr-error/foobar?id=1`); + // Just to be a little bit more sure + await page.waitForTimeout(5000); + const pageServerComponentTransaction = await pageServerComponentTransactionPromise; expect(pageServerComponentTransaction).toBeDefined(); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/suspense-error.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/suspense-error.test.ts index 6c9a58dab4f3..e158e87ae19f 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/suspense-error.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/suspense-error.test.ts @@ -3,18 +3,19 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; test('should not capture serverside suspense errors', async ({ page }) => { const pageServerComponentTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/suspense-error)'; + return transactionEvent?.transaction === 'GET /suspense-error'; }); let errorEvent; - waitForError('nextjs-15', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/suspense-error)'; + waitForError('nextjs-15', async errorEvent => { + return errorEvent?.transaction === 'Page Server Component (/suspense-error)'; }).then(event => { errorEvent = event; }); await page.goto(`/suspense-error`); + // Just to be a little bit more sure await page.waitForTimeout(5000); const pageServerComponentTransaction = await pageServerComponentTransactionPromise; From 79fabbfb6813398ac18da2aeae9ac6fabcef6b03 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 3 Jul 2024 08:29:25 +0000 Subject: [PATCH 06/21] Fix some nextjs-app-dir tests --- ...client-app-routing-instrumentation.test.ts | 2 +- .../connected-servercomponent-trace.test.ts | 49 ++++--------------- .../tests/server-components.test.ts | 48 ++++++------------ 3 files changed, 26 insertions(+), 73 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index 41e63f910f79..ca4ee0b09bdd 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -39,7 +39,7 @@ test('Creates a navigation transaction for app router routes', async ({ page }) const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { return ( - transactionEvent?.transaction === 'Page Server Component (/server-component/parameter/[...parameters])' && + transactionEvent?.transaction === 'GET /server-component/parameter/foo/bar/baz' && (await clientNavigationTransactionPromise).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id ); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/connected-servercomponent-trace.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/connected-servercomponent-trace.test.ts index 287c1e8f8633..3d2f29358d54 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/connected-servercomponent-trace.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/connected-servercomponent-trace.test.ts @@ -1,50 +1,21 @@ import { expect, test } from '@playwright/test'; import { waitForTransaction } from '@sentry-internal/test-utils'; -test('Will capture a connected trace for all server components and generation functions when visiting a page', async ({ +test('Will create a transaction with spans for every server component and metadata generation functions when visiting a page', async ({ page, }) => { - const someConnectedEvent = waitForTransaction('nextjs-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 serverTransactionEventPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { + return transactionEvent?.transaction === 'GET /nested-layout'; }); - const layout1Transaction = waitForTransaction('nextjs-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-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-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 - ); - }); + await page.goto('/nested-layout'); - const generateMetadataTransaction = waitForTransaction('nextjs-app-dir', async transactionEvent => { - return ( - transactionEvent?.transaction === 'Page.generateMetadata (/(nested-layout)/nested-layout)' && - (await someConnectedEvent).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id - ); + const spanDescriptions = (await serverTransactionEventPromise).spans?.map(span => { + return span.description; }); - await page.goto('/nested-layout'); - - expect(await layout1Transaction).toBeDefined(); - expect(await layout2Transaction).toBeDefined(); - expect(await pageTransaction).toBeDefined(); - expect(await generateMetadataTransaction).toBeDefined(); + expect(spanDescriptions).toContainEqual('Layout Server Component (/(nested-layout)/nested-layout)'); + expect(spanDescriptions).toContainEqual('Layout Server Component (/(nested-layout))'); + expect(spanDescriptions).toContainEqual('Page Server Component (/(nested-layout)/nested-layout)'); + expect(spanDescriptions).toContainEqual('Page.generateMetadata (/(nested-layout)/nested-layout)'); }); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts index 6f0413d0cc61..c50a5e2bdec5 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts @@ -1,15 +1,9 @@ import { expect, test } from '@playwright/test'; import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; -test('Sends a transaction for a server component', async ({ page }) => { - // TODO: Fix that this is flakey on dev server - might be an SDK bug - test.skip(process.env.TEST_ENV === 'production', 'Flakey on dev-server'); - +test('Sends a transaction for a request to app router', async ({ page }) => { const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { - return ( - transactionEvent?.contexts?.trace?.op === 'function.nextjs' && - transactionEvent?.transaction === 'Page Server Component (/server-component/parameter/[...parameters])' - ); + return transactionEvent?.transaction === 'GET /server-component/parameter/[...parameters]'; }); await page.goto('/server-component/parameter/1337/42'); @@ -18,13 +12,13 @@ test('Sends a transaction for a server component', async ({ page }) => { expect(transactionEvent.contexts?.trace).toEqual({ data: expect.objectContaining({ - 'sentry.op': 'function.nextjs', - 'sentry.origin': 'auto.function.nextjs', + 'sentry.op': 'http.server', + 'sentry.origin': 'manual', 'sentry.sample_rate': 1, - 'sentry.source': 'component', + 'sentry.source': 'route', }), - op: 'function.nextjs', - origin: 'auto.function.nextjs', + op: 'http.server', + origin: 'manual', span_id: expect.any(String), status: 'ok', trace_id: expect.any(String), @@ -37,22 +31,15 @@ test('Sends a transaction for a server component', async ({ page }) => { headers: expect.any(Object), url: expect.any(String), }, - transaction: 'Page Server Component (/server-component/parameter/[...parameters])', - type: 'transaction', - transaction_info: { - source: 'component', - }, - spans: [], }), ); -}); -test('Should not set an error status on a server component transaction when it redirects', async ({ page }) => { - // TODO: Fix that this is flakey on dev server - might be an SDK bug - test.skip(process.env.TEST_ENV === 'production', 'Flakey on dev-server'); + expect(Object.keys(transactionEvent.request?.headers!).length).toBeGreaterThan(0); +}); +test('Should not set an error status on an app router transaction when it redirects', async ({ page }) => { const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/server-component/redirect)'; + return transactionEvent?.transaction === 'GET /server-component/redirect'; }); await page.goto('/server-component/redirect'); @@ -62,14 +49,9 @@ test('Should not set an error status on a server component transaction when it r expect(transactionEvent.contexts?.trace?.status).not.toBe('internal_error'); }); -test('Should set a "not_found" status on a server component transaction when notFound() is called', async ({ - page, -}) => { - // TODO: Fix that this is flakey on dev server - might be an SDK bug - test.skip(process.env.TEST_ENV === 'production', 'Flakey on dev-server'); - +test('Should set a "not_found" status on an app router transaction when notFound() is called', async ({ page }) => { const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/server-component/not-found)'; + return transactionEvent?.transaction === 'GET /server-component/not-found'; }); await page.goto('/server-component/not-found'); @@ -79,9 +61,9 @@ test('Should set a "not_found" status on a server component transaction when not expect(transactionEvent.contexts?.trace?.status).toBe('not_found'); }); -test('Should capture an error and transaction with correct status for a faulty server component', async ({ page }) => { +test('Should capture an error and transaction with correct status for a app router page', async ({ page }) => { const transactionEventPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { - return transactionEvent?.transaction === 'Page Server Component (/server-component/faulty)'; + return transactionEvent?.transaction === 'GET /server-component/faulty'; }); const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => { From 1cdfdc9918acb38284258739d662d27fb8251735 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 3 Jul 2024 11:42:26 +0000 Subject: [PATCH 07/21] test: Fix e2e test race condition by buffering events --- .../test-utils/src/event-proxy-server.ts | 159 +++++++++++------- 1 file changed, 101 insertions(+), 58 deletions(-) diff --git a/dev-packages/test-utils/src/event-proxy-server.ts b/dev-packages/test-utils/src/event-proxy-server.ts index 30bedadc38bb..dcc92b9f6166 100644 --- a/dev-packages/test-utils/src/event-proxy-server.ts +++ b/dev-packages/test-utils/src/event-proxy-server.ts @@ -1,3 +1,5 @@ +/* eslint-disable max-lines */ + import * as fs from 'fs'; import * as http from 'http'; import type { AddressInfo } from 'net'; @@ -30,12 +32,22 @@ interface SentryRequestCallbackData { sentryResponseStatusCode?: number; } +interface EventCallbackListener { + (data: string): void; +} + type OnRequest = ( - eventCallbackListeners: Set<(data: string) => void>, + eventCallbackListeners: Set, proxyRequest: http.IncomingMessage, proxyRequestBody: string, + eventBuffer: BufferedEvent[], ) => Promise<[number, string, Record | undefined]>; +interface BufferedEvent { + timestamp: number; + data: string; +} + /** * Start a generic proxy server. * The `onRequest` callback receives the incoming request and the request body, @@ -51,7 +63,8 @@ export async function startProxyServer( }, onRequest?: OnRequest, ): Promise { - const eventCallbackListeners: Set<(data: string) => void> = new Set(); + const eventBuffer: BufferedEvent[] = []; + const eventCallbackListeners: Set = new Set(); const proxyServer = http.createServer((proxyRequest, proxyResponse) => { const proxyRequestChunks: Uint8Array[] = []; @@ -76,7 +89,9 @@ export async function startProxyServer( const callback: OnRequest = onRequest || - (async (eventCallbackListeners, proxyRequest, proxyRequestBody) => { + (async (eventCallbackListeners, proxyRequest, proxyRequestBody, eventBuffer) => { + eventBuffer.push({ data: proxyRequestBody, timestamp: Date.now() }); + eventCallbackListeners.forEach(listener => { listener(proxyRequestBody); }); @@ -84,7 +99,7 @@ export async function startProxyServer( return [200, '{}', {}]; }); - callback(eventCallbackListeners, proxyRequest, proxyRequestBody) + callback(eventCallbackListeners, proxyRequest, proxyRequestBody, eventBuffer) .then(([statusCode, responseBody, responseHeaders]) => { proxyResponse.writeHead(statusCode, responseHeaders); proxyResponse.write(responseBody, 'utf-8'); @@ -110,12 +125,24 @@ export async function startProxyServer( eventCallbackResponse.statusCode = 200; eventCallbackResponse.setHeader('connection', 'keep-alive'); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const searchParams = new URL(eventCallbackRequest.url!, 'http://justsomerandombasesothattheurlisparseable.com/') + .searchParams; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const listenerTimestamp = Number(searchParams.get('timestamp')!); + const callbackListener = (data: string): void => { eventCallbackResponse.write(data.concat('\n'), 'utf8'); }; eventCallbackListeners.add(callbackListener); + eventBuffer.forEach(bufferedEvent => { + if (bufferedEvent.timestamp > listenerTimestamp) { + callbackListener(bufferedEvent.data); + } + }); + eventCallbackRequest.on('close', () => { eventCallbackListeners.delete(callbackListener); }); @@ -142,7 +169,7 @@ export async function startProxyServer( * option to this server (like this `tunnel: http://localhost:${port option}/`). */ export async function startEventProxyServer(options: EventProxyServerOptions): Promise { - await startProxyServer(options, async (eventCallbackListeners, proxyRequest, proxyRequestBody) => { + await startProxyServer(options, async (eventCallbackListeners, proxyRequest, proxyRequestBody, eventBuffer) => { const envelopeHeader: EnvelopeItem[0] = JSON.parse(proxyRequestBody.split('\n')[0] as string); const shouldForwardEventToSentry = options.forwardToSentry != null ? options.forwardToSentry : true; @@ -199,8 +226,12 @@ export async function startEventProxyServer(options: EventProxyServerOptions): P sentryResponseStatusCode: res.status, }; + const dataString = Buffer.from(JSON.stringify(data)).toString('base64'); + + eventBuffer.push({ data: dataString, timestamp: Date.now() }); + eventCallbackListeners.forEach(listener => { - listener(Buffer.from(JSON.stringify(data)).toString('base64')); + listener(dataString); }); const resHeaders: Record = {}; @@ -221,24 +252,28 @@ export async function waitForPlainRequest( const eventCallbackServerPort = await retrieveCallbackServerPort(proxyServerName); return new Promise((resolve, reject) => { - const request = http.request(`http://localhost:${eventCallbackServerPort}/`, {}, response => { - let eventContents = ''; - - response.on('error', err => { - reject(err); - }); + const request = http.request( + `http://localhost:${eventCallbackServerPort}/?timestamp=${Date.now()}`, + {}, + response => { + let eventContents = ''; + + response.on('error', err => { + reject(err); + }); - response.on('data', (chunk: Buffer) => { - const chunkString = chunk.toString('utf8'); + response.on('data', (chunk: Buffer) => { + const chunkString = chunk.toString('utf8'); - eventContents = eventContents.concat(chunkString); + eventContents = eventContents.concat(chunkString); - if (callback(eventContents)) { - response.destroy(); - return resolve(eventContents); - } - }); - }); + if (callback(eventContents)) { + response.destroy(); + return resolve(eventContents); + } + }); + }, + ); request.end(); }); @@ -247,49 +282,54 @@ export async function waitForPlainRequest( /** Wait for a request to be sent. */ export async function waitForRequest( proxyServerName: string, + timestamp: number, callback: (eventData: SentryRequestCallbackData) => Promise | boolean, ): Promise { const eventCallbackServerPort = await retrieveCallbackServerPort(proxyServerName); return new Promise((resolve, reject) => { - const request = http.request(`http://localhost:${eventCallbackServerPort}/`, {}, response => { - let eventContents = ''; - - response.on('error', err => { - reject(err); - }); + const request = http.request( + `http://localhost:${eventCallbackServerPort}/?timestamp=${timestamp}`, + {}, + response => { + let eventContents = ''; + + response.on('error', err => { + reject(err); + }); - response.on('data', (chunk: Buffer) => { - const chunkString = chunk.toString('utf8'); - chunkString.split('').forEach(char => { - if (char === '\n') { - const eventCallbackData: SentryRequestCallbackData = JSON.parse( - Buffer.from(eventContents, 'base64').toString('utf8'), - ); - const callbackResult = callback(eventCallbackData); - if (typeof callbackResult !== 'boolean') { - callbackResult.then( - match => { - if (match) { - response.destroy(); - resolve(eventCallbackData); - } - }, - err => { - throw err; - }, + response.on('data', (chunk: Buffer) => { + const chunkString = chunk.toString('utf8'); + chunkString.split('').forEach(char => { + if (char === '\n') { + const eventCallbackData: SentryRequestCallbackData = JSON.parse( + Buffer.from(eventContents, 'base64').toString('utf8'), ); - } else if (callbackResult) { - response.destroy(); - resolve(eventCallbackData); + const callbackResult = callback(eventCallbackData); + if (typeof callbackResult !== 'boolean') { + callbackResult.then( + match => { + if (match) { + response.destroy(); + resolve(eventCallbackData); + } + }, + err => { + throw err; + }, + ); + } else if (callbackResult) { + response.destroy(); + resolve(eventCallbackData); + } + eventContents = ''; + } else { + eventContents = eventContents.concat(char); } - eventContents = ''; - } else { - eventContents = eventContents.concat(char); - } + }); }); - }); - }); + }, + ); request.end(); }); @@ -298,10 +338,11 @@ export async function waitForRequest( /** Wait for a specific envelope item to be sent. */ export function waitForEnvelopeItem( proxyServerName: string, + timestamp: number, callback: (envelopeItem: EnvelopeItem) => Promise | boolean, ): Promise { return new Promise((resolve, reject) => { - waitForRequest(proxyServerName, async eventData => { + waitForRequest(proxyServerName, timestamp, async eventData => { const envelopeItems = eventData.envelope[1]; for (const envelopeItem of envelopeItems) { if (await callback(envelopeItem)) { @@ -319,8 +360,9 @@ export function waitForError( proxyServerName: string, callback: (transactionEvent: Event) => Promise | boolean, ): Promise { + const timestamp = Date.now(); return new Promise((resolve, reject) => { - waitForEnvelopeItem(proxyServerName, async envelopeItem => { + waitForEnvelopeItem(proxyServerName, timestamp, async envelopeItem => { const [envelopeItemHeader, envelopeItemBody] = envelopeItem; if (envelopeItemHeader.type === 'event' && (await callback(envelopeItemBody as Event))) { resolve(envelopeItemBody as Event); @@ -336,8 +378,9 @@ export function waitForTransaction( proxyServerName: string, callback: (transactionEvent: Event) => Promise | boolean, ): Promise { + const timestamp = Date.now(); return new Promise((resolve, reject) => { - waitForEnvelopeItem(proxyServerName, async envelopeItem => { + waitForEnvelopeItem(proxyServerName, timestamp, async envelopeItem => { const [envelopeItemHeader, envelopeItemBody] = envelopeItem; if (envelopeItemHeader.type === 'transaction' && (await callback(envelopeItemBody as Event))) { resolve(envelopeItemBody as Event); From 9ca81378cb59c5fd58019997475ac38f7c924757 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 3 Jul 2024 15:49:50 +0000 Subject: [PATCH 08/21] stuff --- .../test-outgoing-fetch-external-disallowed/route.ts | 6 +++--- .../nextjs-14/app/propagation/test-outgoing-fetch/route.ts | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch-external-disallowed/route.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch-external-disallowed/route.ts index b57d873f3ce7..be38866a9e94 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch-external-disallowed/route.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch-external-disallowed/route.ts @@ -3,8 +3,8 @@ import { NextResponse } from 'next/server'; export const dynamic = 'force-dynamic'; export async function GET() { - const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch-external-disallowed/check`).then( - res => res.json(), - ); + const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch-external-disallowed/check`, { + cache: 'no-store', + }).then(res => res.json()); return NextResponse.json(data); } diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch/route.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch/route.ts index df9f2e772931..4ee7fac97f83 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch/route.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch/route.ts @@ -3,6 +3,8 @@ import { NextResponse } from 'next/server'; export const dynamic = 'force-dynamic'; export async function GET() { - const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch/check`).then(res => res.json()); + const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch/check`, { cache: 'no-store' }).then( + res => res.json(), + ); return NextResponse.json(data); } From 0ba02be18db0ea48208be482c6661796555ec79a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 3 Jul 2024 16:13:21 +0000 Subject: [PATCH 09/21] interesting --- .../tests/client-app-routing-instrumentation.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index ca4ee0b09bdd..3ea9d6272c28 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -39,7 +39,10 @@ test('Creates a navigation transaction for app router routes', async ({ page }) const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { return ( - transactionEvent?.transaction === 'GET /server-component/parameter/foo/bar/baz' && + // Next.js 14+ + (transactionEvent?.transaction === 'GET /server-component/parameter/foo/bar/baz' || + // Next.js 13 + transactionEvent?.transaction === 'GET /server-component/parameter/[...parameters]') && (await clientNavigationTransactionPromise).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id ); From 8e75acf4048be9129c5adb6373e35c3bf58e964d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 4 Jul 2024 07:32:10 +0000 Subject: [PATCH 10/21] wut --- .../tests/client-app-routing-instrumentation.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index 3ea9d6272c28..3869fa8039a0 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -28,7 +28,7 @@ test('Creates a navigation transaction for app router routes', async ({ page }) await page.goto(`/server-component/parameter/${randomRoute}`); await clientPageloadTransactionPromise; - await page.getByText('Page (/server-component/parameter/[parameter])').isVisible(); + await page.getByText('Page (/server-component/[parameter])').isVisible(); const clientNavigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { return ( @@ -38,6 +38,7 @@ test('Creates a navigation transaction for app router routes', async ({ page }) }); const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { + console.log({ t: transactionEvent.transaction }); return ( // Next.js 14+ (transactionEvent?.transaction === 'GET /server-component/parameter/foo/bar/baz' || From 18fe54d4f472093e69251273cd58387a2f45cf3a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 4 Jul 2024 08:40:54 +0000 Subject: [PATCH 11/21] test --- .../tests/client-app-routing-instrumentation.test.ts | 11 ++++++----- .../src/common/wrapGenerationFunctionWithSentry.ts | 6 +++++- .../src/common/wrapServerComponentWithSentry.ts | 6 +++++- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index 3869fa8039a0..a57ba7b92e57 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -38,12 +38,13 @@ test('Creates a navigation transaction for app router routes', async ({ page }) }); const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { - console.log({ t: transactionEvent.transaction }); + console.log({ + t: transactionEvent.transaction, + tid: transactionEvent.contexts?.trace?.trace_id, + ctid: (await clientNavigationTransactionPromise).contexts?.trace?.trace_id, + }); return ( - // Next.js 14+ - (transactionEvent?.transaction === 'GET /server-component/parameter/foo/bar/baz' || - // Next.js 13 - transactionEvent?.transaction === 'GET /server-component/parameter/[...parameters]') && + transactionEvent?.transaction === 'GET /server-component/parameter/foo/bar/baz' && (await clientNavigationTransactionPromise).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id ); diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index 361c6a646a49..32c5a1218358 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -5,6 +5,7 @@ import { Scope, captureException, getActiveSpan, + getCapturedScopesOnSpan, getClient, getRootSpan, handleCallbackErrors, @@ -45,8 +46,11 @@ export function wrapGenerationFunctionWithSentry a const activeSpan = getActiveSpan(); if (activeSpan) { const rootSpan = getRootSpan(activeSpan); + const { scope } = getCapturedScopesOnSpan(rootSpan); + setCapturedScopesOnSpan(rootSpan, scope ?? new Scope(), isolationScope); + + // We mark the root span as an app router span so we can allow-list it in our span processor that would normally filter out all Next.js transactions/spans rootSpan.setAttribute('sentry.rsc', true); - setCapturedScopesOnSpan(rootSpan, new Scope(), isolationScope); } let data: Record | undefined = undefined; diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index dc09c42d2665..b9f0d4f7ba0e 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -5,6 +5,7 @@ import { Scope, captureException, getActiveSpan, + getCapturedScopesOnSpan, getRootSpan, handleCallbackErrors, setCapturedScopesOnSpan, @@ -40,8 +41,11 @@ export function wrapServerComponentWithSentry any> const activeSpan = getActiveSpan(); if (activeSpan) { const rootSpan = getRootSpan(activeSpan); + const { scope } = getCapturedScopesOnSpan(rootSpan); + setCapturedScopesOnSpan(rootSpan, scope ?? new Scope(), isolationScope); + + // We mark the root span as an app router span so we can allow-list it in our span processor that would normally filter out all Next.js transactions/spans rootSpan.setAttribute('sentry.rsc', true); - setCapturedScopesOnSpan(rootSpan, new Scope(), isolationScope); } const headersDict = context.headers ? winterCGHeadersToDict(context.headers) : undefined; From d4aa8f1df71e01bb88f17c74d350a602d3ff049f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 4 Jul 2024 09:42:05 +0000 Subject: [PATCH 12/21] . --- dev-packages/test-utils/src/event-proxy-server.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/dev-packages/test-utils/src/event-proxy-server.ts b/dev-packages/test-utils/src/event-proxy-server.ts index 61e7c37dcde9..e4eb48f03076 100644 --- a/dev-packages/test-utils/src/event-proxy-server.ts +++ b/dev-packages/test-utils/src/event-proxy-server.ts @@ -282,7 +282,6 @@ export async function waitForPlainRequest( /** Wait for a request to be sent. */ export async function waitForRequest( proxyServerName: string, - timestamp: number, callback: (eventData: SentryRequestCallbackData) => Promise | boolean, timestamp: number = Date.now(), ): Promise { @@ -339,7 +338,6 @@ export async function waitForRequest( /** Wait for a specific envelope item to be sent. */ export function waitForEnvelopeItem( proxyServerName: string, - timestamp: number, callback: (envelopeItem: EnvelopeItem) => Promise | boolean, timestamp: number = Date.now(), ): Promise { From 84d9bd701cd01a6f3236ad6c86d1895b6db4a249 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 4 Jul 2024 13:42:50 +0000 Subject: [PATCH 13/21] logic --- .../nextjs-15/app/pageload-tracing/page.tsx | 2 +- .../nextjs-15/tests/pageload-tracing.test.ts | 2 +- ...client-app-routing-instrumentation.test.ts | 5 ----- .../wrapGenerationFunctionWithSentry.ts | 16 +++++++++++++-- .../common/wrapServerComponentWithSentry.ts | 20 +++++++++++++++++-- packages/nextjs/src/server/index.ts | 19 +++++------------- 6 files changed, 39 insertions(+), 25 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-15/app/pageload-tracing/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-15/app/pageload-tracing/page.tsx index 4d2763b992b5..41552d578fd4 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-15/app/pageload-tracing/page.tsx +++ b/dev-packages/e2e-tests/test-applications/nextjs-15/app/pageload-tracing/page.tsx @@ -6,7 +6,7 @@ export default async function Page() { } export async function generateMetadata() { - (await fetch('http://example.com/')).text(); + (await fetch('http://example.com/', { cache: 'no-store' })).text(); return { title: 'my title', diff --git a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/pageload-tracing.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/pageload-tracing.test.ts index 9c48408474fd..3e41c04e2644 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/pageload-tracing.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/pageload-tracing.test.ts @@ -1,7 +1,7 @@ import { expect, test } from '@playwright/test'; import { waitForTransaction } from '@sentry-internal/test-utils'; -test('all server component transactions should be attached to the pageload request span', async ({ page }) => { +test('App router transactions should be attached to the pageload request span', async ({ page }) => { const serverTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { return transactionEvent?.transaction === 'GET /pageload-tracing'; }); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index a57ba7b92e57..8645d36c4c8a 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -38,11 +38,6 @@ test('Creates a navigation transaction for app router routes', async ({ page }) }); const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { - console.log({ - t: transactionEvent.transaction, - tid: transactionEvent.contexts?.trace?.trace_id, - ctid: (await clientNavigationTransactionPromise).contexts?.trace?.trace_id, - }); return ( transactionEvent?.transaction === 'GET /server-component/parameter/foo/bar/baz' && (await clientNavigationTransactionPromise).contexts?.trace?.trace_id === diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index 32c5a1218358..e0ad6e41dd41 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -15,12 +15,12 @@ import { withScope, } from '@sentry/core'; import type { WebFetchHeaders } from '@sentry/types'; -import { winterCGHeadersToDict } from '@sentry/utils'; +import { propagationContextFromHeaders, uuid4, winterCGHeadersToDict } from '@sentry/utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import type { GenerationFunctionContext } from '../common/types'; import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils'; -import { commonObjectToIsolationScope } from './utils/tracingUtils'; +import { commonObjectToIsolationScope, commonObjectToPropagationContext } from './utils/tracingUtils'; /** * Wraps a generation function (e.g. generateMetadata) with Sentry error and performance instrumentation. @@ -33,6 +33,7 @@ export function wrapGenerationFunctionWithSentry a const { requestAsyncStorage, componentRoute, componentType, generationFunctionIdentifier } = context; return new Proxy(generationFunction, { apply: (originalFunction, thisArg, args) => { + const requestTraceId = getActiveSpan()?.spanContext().traceId; let headers: WebFetchHeaders | undefined = undefined; // We try-catch here just in case anything goes wrong with the async storage here goes wrong since it is Next.js internal API try { @@ -74,6 +75,17 @@ export function wrapGenerationFunctionWithSentry a }, }); + const propagationContext = commonObjectToPropagationContext( + headers, + headersDict?.['sentry-trace'] + ? propagationContextFromHeaders(headersDict['sentry-trace'], headersDict['baggage']) + : { + traceId: requestTraceId || uuid4(), + spanId: uuid4().substring(16), + }, + ); + scope.setPropagationContext(propagationContext); + scope.setExtra('route_data', data); return startSpanManual( diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index b9f0d4f7ba0e..ae315fd6ae7d 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -13,13 +13,13 @@ import { withIsolationScope, withScope, } from '@sentry/core'; -import { winterCGHeadersToDict } from '@sentry/utils'; +import { propagationContextFromHeaders, uuid4, winterCGHeadersToDict } from '@sentry/utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; import type { ServerComponentContext } from '../common/types'; import { flushSafelyWithTimeout } from './utils/responseEnd'; -import { commonObjectToIsolationScope } from './utils/tracingUtils'; +import { commonObjectToIsolationScope, commonObjectToPropagationContext } from './utils/tracingUtils'; import { vercelWaitUntil } from './utils/vercelWaitUntil'; /** @@ -36,6 +36,7 @@ export function wrapServerComponentWithSentry any> // hook. 🤯 return new Proxy(appDirComponent, { apply: (originalFunction, thisArg, args) => { + const requestTraceId = getActiveSpan()?.spanContext().traceId; const isolationScope = commonObjectToIsolationScope(context.headers); const activeSpan = getActiveSpan(); @@ -59,6 +60,21 @@ export function wrapServerComponentWithSentry any> return withIsolationScope(isolationScope, () => { return withScope(scope => { scope.setTransactionName(`${componentType} Server Component (${componentRoute})`); + + if (process.env.NEXT_RUNTIME === 'edge') { + const propagationContext = commonObjectToPropagationContext( + context.headers, + headersDict?.['sentry-trace'] + ? propagationContextFromHeaders(headersDict['sentry-trace'], headersDict['baggage']) + : { + traceId: requestTraceId || uuid4(), + spanId: uuid4().substring(16), + }, + ); + + scope.setPropagationContext(propagationContext); + } + return startSpanManual( { op: 'function.nextjs', diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 24cd8706b08e..a4bce478bc64 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -1,4 +1,4 @@ -import { applySdkMetadata, getClient, getGlobalScope } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, applySdkMetadata, getClient, getGlobalScope } from '@sentry/core'; import { getDefaultIntegrations, init as nodeInit } from '@sentry/node'; import type { NodeClient, NodeOptions } from '@sentry/node'; import { GLOBAL_OBJ, logger } from '@sentry/utils'; @@ -86,14 +86,7 @@ export function init(options: NodeOptions): NodeClient | undefined { return; } - const customDefaultIntegrations = [ - ...getDefaultIntegrations(options).filter( - integration => - // Next.js comes with its own Http instrumentation for OTel which would lead to double spans for route handler requests - integration.name !== 'Http', - ), - httpIntegration(), - ]; + const customDefaultIntegrations = getDefaultIntegrations(options); // Turn off Next.js' own fetch instrumentation // https://github.com/lforst/nextjs-fork/blob/1994fd186defda77ad971c36dc3163db263c993f/packages/next/src/server/lib/patch-fetch.ts#L245 @@ -128,7 +121,6 @@ export function init(options: NodeOptions): NodeClient | undefined { applySdkMetadata(opts, 'nextjs', ['nextjs', 'node']); const client = nodeInit(opts); - // If we encounter a span emitted by Next.js, we do not want to sample it // The reason for this is that the data quality of the spans varies, it is different per version of Next, // and we need to keep our manual instrumentation around for the edge runtime anyhow. @@ -160,11 +152,10 @@ export function init(options: NodeOptions): NodeClient | undefined { return null; } - // 'BaseServer.handleRequest' spans are the only Next.js spans we sample. However, we only want to actually keep these spans/transactions, - // when they server RSCs (app router routes). We mark these transactions with a "sentry.rsc" attribute in our RSC wrappers so we can - // filter them here. + // We only want to use our HTTP integration/instrumentation for app router requests, which are marked with the `sentry.rsc` attribute. if ( - event.contexts?.trace?.data?.['next.span_type'] === 'BaseServer.handleRequest' && + (event.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] === 'auto.http.otel.http' || + event.contexts?.trace?.data?.['next.span_type'] === 'BaseServer.handleRequest') && event.contexts?.trace?.data?.['sentry.rsc'] !== true ) { return null; From a949cce945df5f1b4569af877d25ca06a946e017 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 4 Jul 2024 20:23:02 +0000 Subject: [PATCH 14/21] fix some stuff --- .../create-next-app/package.json | 1 - .../nextjs-14/playwright.config.ts | 14 +++++-- .../nextjs-15/playwright.config.mjs | 14 +++++-- .../nextjs-15/tests/ppr-error.test.ts | 3 -- .../nextjs-app-dir/package.json | 1 - .../nextjs-app-dir/playwright.config.mjs | 14 +++++-- .../tests/server-components.test.ts | 39 +++++++++++++++---- .../test-utils/src/playwright-config.ts | 2 +- .../wrapGenerationFunctionWithSentry.ts | 2 + .../common/wrapServerComponentWithSentry.ts | 4 +- packages/nextjs/src/server/index.ts | 27 ++++++++++++- 11 files changed, 93 insertions(+), 28 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/create-next-app/package.json b/dev-packages/e2e-tests/test-applications/create-next-app/package.json index 2c0945051ee5..9c240942b3b7 100644 --- a/dev-packages/e2e-tests/test-applications/create-next-app/package.json +++ b/dev-packages/e2e-tests/test-applications/create-next-app/package.json @@ -12,7 +12,6 @@ "test:assert": "pnpm test:prod && pnpm test:dev" }, "dependencies": { - "@next/font": "13.0.7", "@sentry/nextjs": "latest || *", "@types/node": "18.11.17", "@types/react": "18.0.26", diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/playwright.config.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/playwright.config.ts index c675d003853a..8448829443d6 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-14/playwright.config.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-14/playwright.config.ts @@ -5,9 +5,15 @@ if (!testEnv) { throw new Error('No test env defined'); } -const config = getPlaywrightConfig({ - startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030', - port: 3030, -}); +const config = getPlaywrightConfig( + { + startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030', + port: 3030, + }, + { + // This comes with the risk of tests leaking into each other but the tests run quite slow so we should parallelize + workers: '100%', + }, +); export default config; diff --git a/dev-packages/e2e-tests/test-applications/nextjs-15/playwright.config.mjs b/dev-packages/e2e-tests/test-applications/nextjs-15/playwright.config.mjs index c675d003853a..8448829443d6 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-15/playwright.config.mjs +++ b/dev-packages/e2e-tests/test-applications/nextjs-15/playwright.config.mjs @@ -5,9 +5,15 @@ if (!testEnv) { throw new Error('No test env defined'); } -const config = getPlaywrightConfig({ - startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030', - port: 3030, -}); +const config = getPlaywrightConfig( + { + startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030', + port: 3030, + }, + { + // This comes with the risk of tests leaking into each other but the tests run quite slow so we should parallelize + workers: '100%', + }, +); export default config; diff --git a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/ppr-error.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/ppr-error.test.ts index 00ad3b4a3e03..7c7c0b91eed2 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-15/tests/ppr-error.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-15/tests/ppr-error.test.ts @@ -15,9 +15,6 @@ test('should not capture React-internal errors for PPR rendering', async ({ page await page.goto(`/ppr-error/foobar?id=1`); - // Just to be a little bit more sure - await page.waitForTimeout(5000); - const pageServerComponentTransaction = await pageServerComponentTransactionPromise; expect(pageServerComponentTransaction).toBeDefined(); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/package.json b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/package.json index 4e47e84efc8b..8ccad25e6ab4 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/package.json +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/package.json @@ -15,7 +15,6 @@ "test:assert": "pnpm test:test-build && pnpm test:prod && pnpm test:dev" }, "dependencies": { - "@next/font": "13.0.7", "@sentry/nextjs": "latest || *", "@types/node": "18.11.17", "@types/react": "18.0.26", diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/playwright.config.mjs b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/playwright.config.mjs index c675d003853a..8448829443d6 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/playwright.config.mjs +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/playwright.config.mjs @@ -5,9 +5,15 @@ if (!testEnv) { throw new Error('No test env defined'); } -const config = getPlaywrightConfig({ - startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030', - port: 3030, -}); +const config = getPlaywrightConfig( + { + startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030', + port: 3030, + }, + { + // This comes with the risk of tests leaking into each other but the tests run quite slow so we should parallelize + workers: '100%', + }, +); export default config; diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts index c50a5e2bdec5..e62700a36aca 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts @@ -13,12 +13,18 @@ test('Sends a transaction for a request to app router', async ({ page }) => { expect(transactionEvent.contexts?.trace).toEqual({ data: expect.objectContaining({ 'sentry.op': 'http.server', - 'sentry.origin': 'manual', + 'sentry.origin': 'auto.http.otel.http', 'sentry.sample_rate': 1, 'sentry.source': 'route', + 'http.method': 'GET', + 'http.response.status_code': 200, + 'http.route': '/server-component/parameter/[...parameters]', + 'http.status_code': 200, + 'http.target': '/server-component/parameter/1337/42', + 'otel.kind': 'SERVER', }), op: 'http.server', - origin: 'manual', + origin: 'auto.http.otel.http', span_id: expect.any(String), status: 'ok', trace_id: expect.any(String), @@ -49,7 +55,9 @@ test('Should not set an error status on an app router transaction when it redire expect(transactionEvent.contexts?.trace?.status).not.toBe('internal_error'); }); -test('Should set a "not_found" status on an app router transaction when notFound() is called', async ({ page }) => { +test('Should set a "not_found" status on a server component span when notFound() is called and the request span should have status ok', async ({ + page, +}) => { const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { return transactionEvent?.transaction === 'GET /server-component/not-found'; }); @@ -58,10 +66,18 @@ test('Should set a "not_found" status on an app router transaction when notFound const transactionEvent = await serverComponentTransactionPromise; - expect(transactionEvent.contexts?.trace?.status).toBe('not_found'); + // Transaction should have status ok, because the http status is ok, but the server component span should be not_found + expect(transactionEvent.contexts?.trace?.status).toBe('ok'); + expect(transactionEvent.spans).toContainEqual( + expect.objectContaining({ + description: 'Page Server Component (/server-component/not-found)', + op: 'function.nextjs', + status: 'not_found', + }), + ); }); -test('Should capture an error and transaction with correct status for a app router page', async ({ page }) => { +test('Should capture an error and transaction for a app router page', async ({ page }) => { const transactionEventPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { return transactionEvent?.transaction === 'GET /server-component/faulty'; }); @@ -75,10 +91,19 @@ test('Should capture an error and transaction with correct status for a app rout const transactionEvent = await transactionEventPromise; const errorEvent = await errorEventPromise; - expect(transactionEvent.contexts?.trace?.status).toBe('internal_error'); - + // Error event should have the right transaction name expect(errorEvent.transaction).toBe(`Page Server Component (/server-component/faulty)`); + // Transaction should have status ok, because the http status is ok, but the server component span should be internal_error + expect(transactionEvent.contexts?.trace?.status).toBe('ok'); + expect(transactionEvent.spans).toContainEqual( + expect.objectContaining({ + description: 'Page Server Component (/server-component/faulty)', + op: 'function.nextjs', + status: 'internal_error', + }), + ); + expect(errorEvent.tags?.['my-isolated-tag']).toBe(true); expect(errorEvent.tags?.['my-global-scope-isolated-tag']).not.toBeDefined(); expect(transactionEvent.tags?.['my-isolated-tag']).toBe(true); diff --git a/dev-packages/test-utils/src/playwright-config.ts b/dev-packages/test-utils/src/playwright-config.ts index 4f2ea54bc3b4..33de29f5a7fc 100644 --- a/dev-packages/test-utils/src/playwright-config.ts +++ b/dev-packages/test-utils/src/playwright-config.ts @@ -37,7 +37,7 @@ export function getPlaywrightConfig( /* In dev mode some apps are flaky, so we allow retry there... */ retries: testEnv === 'development' ? 3 : 0, /* Reporter to use. See https://playwright.dev/docs/test-reporters */ - reporter: 'list', + reporter: process.env.CI ? 'line' : 'list', /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ use: { /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index e0ad6e41dd41..5944b520f6ea 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -101,6 +101,8 @@ export function wrapGenerationFunctionWithSentry a return handleCallbackErrors( () => originalFunction.apply(thisArg, args), err => { + // 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(err)) { // We don't want to report "not-found"s span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index ae315fd6ae7d..e8d734a90ff3 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -88,16 +88,16 @@ export function wrapServerComponentWithSentry any> 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' }); - getRootSpan(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' }); - getRootSpan(span).setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); captureException(error, { mechanism: { handled: false, diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index a4bce478bc64..46ad98c30073 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -1,8 +1,16 @@ -import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, applySdkMetadata, getClient, getGlobalScope } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + applySdkMetadata, + getClient, + getGlobalScope, + getRootSpan, + spanToJSON, +} from '@sentry/core'; import { getDefaultIntegrations, init as nodeInit } from '@sentry/node'; import type { NodeClient, NodeOptions } from '@sentry/node'; import { GLOBAL_OBJ, logger } from '@sentry/utils'; +import { SEMATTRS_HTTP_METHOD, SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions'; import type { EventProcessor } from '@sentry/types'; import { DEBUG_BUILD } from '../common/debug-build'; import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolicationEventProcessor'; @@ -141,6 +149,23 @@ export function init(options: NodeOptions): NodeClient | undefined { } }); + // What we do in this glorious piece of code, is hoist any information about parameterized routes from spans emitted + // by Next.js via the `next.route` attribure, up to the transaction by setting the http.route attribute. + client?.on('spanStart', span => { + const spanAttributes = spanToJSON(span).data; + if (spanAttributes?.['next.route']) { + const rootSpan = getRootSpan(span); + const rootSpanAttributes = spanToJSON(rootSpan).data; + + // Only hoist the http.route attribute if the transaction doesn't already have it + if (rootSpanAttributes?.[SEMATTRS_HTTP_METHOD] && !rootSpanAttributes?.[SEMATTRS_HTTP_ROUTE]) { + rootSpan.setAttribute(SEMATTRS_HTTP_ROUTE, spanAttributes['next.route']); + } + } + }); + + client?.on('spanEnd'); + getGlobalScope().addEventProcessor( Object.assign( (event => { From 1dd7c772b38c804ce9a854698609a0a9ecd5c602 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 4 Jul 2024 20:34:58 +0000 Subject: [PATCH 15/21] dum dum --- packages/nextjs/src/server/index.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 46ad98c30073..2c51004103c6 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -164,8 +164,6 @@ export function init(options: NodeOptions): NodeClient | undefined { } }); - client?.on('spanEnd'); - getGlobalScope().addEventProcessor( Object.assign( (event => { From 1a8a2dbc2133ffa9b181d9da92b22fa2cdb02258 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 4 Jul 2024 20:51:16 +0000 Subject: [PATCH 16/21] meep moop --- packages/nextjs/package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index ba6baffaa678..1b5ce7dc00f3 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -69,6 +69,7 @@ }, "dependencies": { "@opentelemetry/instrumentation-http": "0.52.1", + "@opentelemetry/semantic-conventions": "^1.25.1", "@rollup/plugin-commonjs": "26.0.1", "@sentry/core": "8.14.0", "@sentry/node": "8.14.0", From 542bcdae6a04b0749e078afaa0b099ffa329d3c4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 5 Jul 2024 08:07:31 +0000 Subject: [PATCH 17/21] drop ingest spans --- packages/nextjs/src/server/index.ts | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 2c51004103c6..dba5ef0fe3b4 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -10,7 +10,7 @@ import { getDefaultIntegrations, init as nodeInit } from '@sentry/node'; import type { NodeClient, NodeOptions } from '@sentry/node'; import { GLOBAL_OBJ, logger } from '@sentry/utils'; -import { SEMATTRS_HTTP_METHOD, SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions'; +import { SEMATTRS_HTTP_METHOD, SEMATTRS_HTTP_ROUTE, SEMATTRS_HTTP_TARGET } from '@opentelemetry/semantic-conventions'; import type { EventProcessor } from '@sentry/types'; import { DEBUG_BUILD } from '../common/debug-build'; import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolicationEventProcessor'; @@ -129,10 +129,6 @@ export function init(options: NodeOptions): NodeClient | undefined { applySdkMetadata(opts, 'nextjs', ['nextjs', 'node']); const client = nodeInit(opts); - // If we encounter a span emitted by Next.js, we do not want to sample it - // The reason for this is that the data quality of the spans varies, it is different per version of Next, - // and we need to keep our manual instrumentation around for the edge runtime anyhow. - // BUT we only do this if we don't have a parent span with a sampling decision yet (or if the parent is remote) client?.on('beforeSampling', ({ spanAttributes, spanName, parentSampled, parentContext }, samplingDecision) => { // We "whitelist" the "BaseServer.handleRequest" span, since that one is responsible for App Router requests, which are actually useful for us. // HOWEVER, that span is not only responsible for App Router requests, which is why we additionally filter for certain transactions in an @@ -141,12 +137,28 @@ export function init(options: NodeOptions): NodeClient | undefined { return; } + // If we encounter a span emitted by Next.js, we do not want to sample it + // The reason for this is that the data quality of the spans varies, it is different per version of Next, + // and we need to keep our manual instrumentation around for the edge runtime anyhow. + // BUT we only do this if we don't have a parent span with a sampling decision yet (or if the parent is remote) if ( (spanAttributes['next.span_type'] || NEXTJS_SPAN_NAME_PREFIXES.some(prefix => spanName.startsWith(prefix))) && (parentSampled === undefined || parentContext?.isRemote) ) { samplingDecision.decision = false; } + + // There are situations where the Next.js Node.js server forwards requests for the Edge Runtime server (e.g. in + // middleware) and this causes spans for Sentry ingest requests to be created. These are not exempt from our tracing + // because we didn't get the chance to do `suppressTracing`, since this happens outside of userland. + // We need to drop these spans. + if ( + typeof spanAttributes[SEMATTRS_HTTP_TARGET] === 'string' && + spanAttributes[SEMATTRS_HTTP_TARGET].includes('sentry_key') && + spanAttributes[SEMATTRS_HTTP_TARGET].includes('sentry_client') + ) { + samplingDecision.decision = false; + } }); // What we do in this glorious piece of code, is hoist any information about parameterized routes from spans emitted From 19260d6ef8e98cb87df086932b33701570c257df Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 5 Jul 2024 08:56:47 +0000 Subject: [PATCH 18/21] span quality --- packages/nextjs/src/server/httpIntegration.ts | 48 ------------------- packages/nextjs/src/server/index.ts | 20 ++++++-- 2 files changed, 15 insertions(+), 53 deletions(-) delete mode 100644 packages/nextjs/src/server/httpIntegration.ts diff --git a/packages/nextjs/src/server/httpIntegration.ts b/packages/nextjs/src/server/httpIntegration.ts deleted file mode 100644 index 4fdc615deb92..000000000000 --- a/packages/nextjs/src/server/httpIntegration.ts +++ /dev/null @@ -1,48 +0,0 @@ -import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; -import { httpIntegration as originalHttpIntegration } from '@sentry/node'; -import type { IntegrationFn } from '@sentry/types'; - -/** - * Next.js handles incoming requests itself, - * but it does not handle outgoing requests. - * Today, it is not possible to use the HttpInstrumentation for only outgoing requests - - * until https://github.com/open-telemetry/opentelemetry-js/pull/4643 is merged & released. - * So in the meanwhile, we extend the base HttpInstrumentation to not wrap incoming requests. - */ -class CustomNextjsHttpIntegration extends HttpInstrumentation { - // Instead of the default behavior, we just don't do any wrapping for incoming requests - protected _getPatchIncomingRequestFunction(_component: 'http' | 'https') { - return ( - original: (event: string, ...args: unknown[]) => boolean, - ): ((this: unknown, event: string, ...args: unknown[]) => boolean) => { - return function incomingRequest(this: unknown, event: string, ...args: unknown[]): boolean { - return original.apply(this, [event, ...args]); - }; - }; - } -} - -interface HttpOptions { - /** - * Whether breadcrumbs should be recorded for requests. - * Defaults to true - */ - breadcrumbs?: boolean; - - /** - * Do not capture spans or breadcrumbs for outgoing HTTP requests to URLs where the given callback returns `true`. - * This controls both span & breadcrumb creation - spans will be non recording if tracing is disabled. - */ - ignoreOutgoingRequests?: (url: string) => boolean; -} - -/** - * The http integration instruments Node's internal http and https modules. - * It creates breadcrumbs and spans for outgoing HTTP requests which will be attached to the currently active span. - */ -export const httpIntegration = ((options: HttpOptions = {}) => { - return originalHttpIntegration({ - ...options, - _instrumentation: CustomNextjsHttpIntegration, - }); -}) satisfies IntegrationFn; diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index dba5ef0fe3b4..26b4b00c4e4f 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -17,12 +17,9 @@ import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolica import { getVercelEnv } from '../common/getVercelEnv'; import { isBuild } from '../common/utils/isBuild'; import { distDirRewriteFramesIntegration } from './distDirRewriteFramesIntegration'; -import { httpIntegration } from './httpIntegration'; export * from '@sentry/node'; -export { httpIntegration }; - export { captureUnderscoreErrorException } from '../common/_error'; const globalWithInjectedValues = GLOBAL_OBJ as typeof GLOBAL_OBJ & { @@ -161,10 +158,11 @@ export function init(options: NodeOptions): NodeClient | undefined { } }); - // What we do in this glorious piece of code, is hoist any information about parameterized routes from spans emitted - // by Next.js via the `next.route` attribure, up to the transaction by setting the http.route attribute. client?.on('spanStart', span => { const spanAttributes = spanToJSON(span).data; + + // What we do in this glorious piece of code, is hoist any information about parameterized routes from spans emitted + // by Next.js via the `next.route` attribure, up to the transaction by setting the http.route attribute. if (spanAttributes?.['next.route']) { const rootSpan = getRootSpan(span); const rootSpanAttributes = spanToJSON(rootSpan).data; @@ -174,6 +172,18 @@ export function init(options: NodeOptions): NodeClient | undefined { rootSpan.setAttribute(SEMATTRS_HTTP_ROUTE, spanAttributes['next.route']); } } + + // We want to skip span data inference for any spans generated by Next.js. Reason being that Next.js emits spans + // with patterns (e.g. http.server spans) that will produce confusing data. + if (spanAttributes?.['next.span_type'] !== undefined) { + span.setAttribute('sentry.skip_span_data_inference', true); + } + + // We want to rename these spans because they look like "GET /path/to/route" and we already emit spans that look + // like this with our own http instrumentation. + if (spanAttributes?.['next.span_type'] === 'BaseServer.handleRequest') { + span.updateName('next server handler'); // This is all lowercase because the spans that Next.js emits by itself generally look like this. + } }); getGlobalScope().addEventProcessor( From 88f9a54e88168293275a4d186e6695aa4088a414 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 5 Jul 2024 10:14:00 +0000 Subject: [PATCH 19/21] Add test --- .../nextjs-app-dir/tests/server-components.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts index e62700a36aca..ba232ad558b0 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts @@ -41,6 +41,14 @@ test('Sends a transaction for a request to app router', async ({ page }) => { ); expect(Object.keys(transactionEvent.request?.headers!).length).toBeGreaterThan(0); + + // The transaction should not contain any spans with the same name as the transaction + // e.g. "GET /server-component/parameter/[...parameters]" + expect( + transactionEvent.spans?.filter(span => { + return span.description === transactionEvent.transaction; + }), + ).toHaveLength(0); }); test('Should not set an error status on an app router transaction when it redirects', async ({ page }) => { From df0c49e9051760c9f502057aab49b5726b598f52 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 8 Jul 2024 07:53:50 +0000 Subject: [PATCH 20/21] Review feedback --- packages/nextjs/src/server/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 26b4b00c4e4f..1132a6e1eed2 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -127,7 +127,7 @@ export function init(options: NodeOptions): NodeClient | undefined { const client = nodeInit(opts); client?.on('beforeSampling', ({ spanAttributes, spanName, parentSampled, parentContext }, samplingDecision) => { - // We "whitelist" the "BaseServer.handleRequest" span, since that one is responsible for App Router requests, which are actually useful for us. + // We allowlist the "BaseServer.handleRequest" span, since that one is responsible for App Router requests, which are actually useful for us. // HOWEVER, that span is not only responsible for App Router requests, which is why we additionally filter for certain transactions in an // event processor further below. if (spanAttributes['next.span_type'] === 'BaseServer.handleRequest') { @@ -162,7 +162,7 @@ export function init(options: NodeOptions): NodeClient | undefined { const spanAttributes = spanToJSON(span).data; // What we do in this glorious piece of code, is hoist any information about parameterized routes from spans emitted - // by Next.js via the `next.route` attribure, up to the transaction by setting the http.route attribute. + // by Next.js via the `next.route` attribute, up to the transaction by setting the http.route attribute. if (spanAttributes?.['next.route']) { const rootSpan = getRootSpan(span); const rootSpanAttributes = spanToJSON(rootSpan).data; From ceb9d2caf96e49b6de4366f47b4629a7d1df735d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 8 Jul 2024 09:42:30 +0000 Subject: [PATCH 21/21] Add changelog entry --- CHANGELOG.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90fb61d73ae2..6929bdfb48ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,27 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +### Important Changes + +- **feat(nextjs): Use spans generated by Next.js for App Router (#12729)** + +Previously, the `@sentry/nextjs` SDK automatically recorded spans in the form of transactions for each of your top-level +server components (pages, layouts, ...). This approach had a few drawbacks, the main ones being that traces didn't have +a root span, and more importantly, if you had data stream to the client, its duration was not captured because the +server component spans had finished before the data could finish streaming. + +With this release, we will capture the duration of App Router requests in their entirety as a single transaction with +server component spans being descendants of that transaction. This means you will get more data that is also more +accurate. Note that this does not apply to the Edge runtime. For the Edge runtime, the SDK will emit transactions as it +has before. + +Generally speaking, this change means that you will see less _transactions_ and more _spans_ in Sentry. Your will no +longer receive server component transactions like `Page Server Component (/path/to/route)` (unless using the Edge +runtime), and you will instead receive transactions for your App Router SSR requests that look like +`GET /path/to/route`. + +If you are on Sentry SaaS, this may have an effect on your quota consumption: Less transactions, more spans. + ## 8.15.0 - feat(core): allow unregistering callback through `on` (#11710)