From 7b2af0f5299eeed133a5d5ff9beb1acc1cb2c192 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 08:46:03 +0000 Subject: [PATCH 01/12] Drop any spans emitted by Next.js for pages router API routes --- .../wrapApiHandlerWithSentry.ts | 12 ++++++++++++ .../common/span-attributes-with-logic-attached.ts | 4 ++++ packages/nextjs/src/server/index.ts | 6 ++++++ 3 files changed, 22 insertions(+) create mode 100644 packages/nextjs/src/common/span-attributes-with-logic-attached.ts diff --git a/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts index 51afa404c113..c9a1446b50ee 100644 --- a/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts @@ -2,6 +2,8 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, captureException, continueTrace, + getActiveSpan, + getRootSpan, setHttpStatus, startSpanManual, withIsolationScope, @@ -9,6 +11,7 @@ import { import { consoleSandbox, isString, logger, objectify } from '@sentry/utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; +import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; import type { AugmentedNextApiRequest, AugmentedNextApiResponse, NextApiHandler } from '../types'; import { flushSafelyWithTimeout } from '../utils/responseEnd'; import { escapeNextjsTracing } from '../utils/tracingUtils'; @@ -24,6 +27,15 @@ import { vercelWaitUntil } from '../utils/vercelWaitUntil'; * @returns The wrapped handler */ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameterizedRoute: string): NextApiHandler { + // Since the API route handler spans emitted by Next.js are super buggy with completely wrong timestamps + // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally + // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think + // about keeping them. + const nextJsOwnedSpan = getActiveSpan(); + if (nextJsOwnedSpan) { + getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); + } + return new Proxy(apiHandler, { apply: ( wrappingTarget, diff --git a/packages/nextjs/src/common/span-attributes-with-logic-attached.ts b/packages/nextjs/src/common/span-attributes-with-logic-attached.ts new file mode 100644 index 000000000000..f6b5b46fdcac --- /dev/null +++ b/packages/nextjs/src/common/span-attributes-with-logic-attached.ts @@ -0,0 +1,4 @@ +/** + * If this attribute is attached to a transaction, the Next.js SDK will drop that transaction. + */ +export const TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION = 'sentry.drop_transaction'; diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 1bfc57b44418..5ac96c0eac77 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -21,6 +21,7 @@ import type { EventProcessor } from '@sentry/types'; import { DEBUG_BUILD } from '../common/debug-build'; import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolicationEventProcessor'; import { getVercelEnv } from '../common/getVercelEnv'; +import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../common/span-attributes-with-logic-attached'; import { isBuild } from '../common/utils/isBuild'; import { distDirRewriteFramesIntegration } from './distDirRewriteFramesIntegration'; @@ -244,6 +245,11 @@ export function init(options: NodeOptions): NodeClient | undefined { return null; } + // Filter transactions that we explicitly want to drop. + if (event.contexts?.trace?.data?.[TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION]) { + return null; + } + return event; } else { return event; From 8ed3a47067f6ace8ca5d29c25e4e5310a5a7d8ed Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 13:56:39 +0000 Subject: [PATCH 02/12] ref(nextjs): Make individual features opt into OTEL tracing --- .../wrapPageComponentWithSentry.ts | 21 +++++++++- .../src/common/utils/edgeWrapperUtils.ts | 12 ++++++ .../nextjs/src/common/utils/wrapperUtils.ts | 12 ++++++ .../common/withServerActionInstrumentation.ts | 12 ++++++ .../wrapGenerationFunctionWithSentry.ts | 3 -- .../common/wrapServerComponentWithSentry.ts | 3 -- packages/nextjs/src/server/index.ts | 39 ++++++------------- packages/opentelemetry/src/spanExporter.ts | 1 - .../src/utils/parseSpanDescription.ts | 13 ------- .../test/utils/parseSpanDescription.test.ts | 21 ---------- 10 files changed, 68 insertions(+), 69 deletions(-) diff --git a/packages/nextjs/src/common/pages-router-instrumentation/wrapPageComponentWithSentry.ts b/packages/nextjs/src/common/pages-router-instrumentation/wrapPageComponentWithSentry.ts index 75c176de8fac..535c88b82f88 100644 --- a/packages/nextjs/src/common/pages-router-instrumentation/wrapPageComponentWithSentry.ts +++ b/packages/nextjs/src/common/pages-router-instrumentation/wrapPageComponentWithSentry.ts @@ -1,5 +1,6 @@ -import { captureException, getCurrentScope, withIsolationScope } from '@sentry/core'; +import { captureException, getActiveSpan, getCurrentScope, getRootSpan, withIsolationScope } from '@sentry/core'; import { extractTraceparentData } from '@sentry/utils'; +import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; import { escapeNextjsTracing } from '../utils/tracingUtils'; interface FunctionComponent { @@ -25,6 +26,15 @@ export function wrapPageComponentWithSentry(pageComponent: FunctionComponent | C if (isReactClassComponent(pageComponent)) { return class SentryWrappedPageComponent extends pageComponent { public render(...args: unknown[]): unknown { + // Since the spans emitted by Next.js are super buggy with completely wrong timestamps + // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally + // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think + // about keeping them. + const nextJsOwnedSpan = getActiveSpan(); + if (nextJsOwnedSpan) { + getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); + } + return escapeNextjsTracing(() => { return withIsolationScope(() => { const scope = getCurrentScope(); @@ -62,6 +72,15 @@ export function wrapPageComponentWithSentry(pageComponent: FunctionComponent | C } else if (typeof pageComponent === 'function') { return new Proxy(pageComponent, { apply(target, thisArg, argArray: [{ _sentryTraceData?: string } | undefined]) { + // Since the spans emitted by Next.js are super buggy with completely wrong timestamps + // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally + // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think + // about keeping them. + const nextJsOwnedSpan = getActiveSpan(); + if (nextJsOwnedSpan) { + getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); + } + return escapeNextjsTracing(() => { return withIsolationScope(() => { const scope = getCurrentScope(); diff --git a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts index 65bdabc93dda..7919a980673c 100644 --- a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts +++ b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts @@ -4,6 +4,8 @@ import { SPAN_STATUS_OK, captureException, continueTrace, + getActiveSpan, + getRootSpan, handleCallbackErrors, setHttpStatus, startSpan, @@ -12,6 +14,7 @@ import { import { winterCGRequestToRequestData } from '@sentry/utils'; import type { EdgeRouteHandler } from '../../edge/types'; +import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; import { flushSafelyWithTimeout } from './responseEnd'; import { commonObjectToIsolationScope, escapeNextjsTracing } from './tracingUtils'; import { vercelWaitUntil } from './vercelWaitUntil'; @@ -24,6 +27,15 @@ export function withEdgeWrapping( options: { spanDescription: string; spanOp: string; mechanismFunctionName: string }, ): (...params: Parameters) => Promise> { return async function (this: unknown, ...args) { + // Since the spans emitted by Next.js are super buggy with completely wrong timestamps + // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally + // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think + // about keeping them. + const nextJsOwnedSpan = getActiveSpan(); + if (nextJsOwnedSpan) { + getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); + } + return escapeNextjsTracing(() => { const req: unknown = args[0]; return withIsolationScope(commonObjectToIsolationScope(req), isolationScope => { diff --git a/packages/nextjs/src/common/utils/wrapperUtils.ts b/packages/nextjs/src/common/utils/wrapperUtils.ts index f07970e4db3b..6c071fae05c3 100644 --- a/packages/nextjs/src/common/utils/wrapperUtils.ts +++ b/packages/nextjs/src/common/utils/wrapperUtils.ts @@ -6,6 +6,8 @@ import { SPAN_STATUS_OK, captureException, continueTrace, + getActiveSpan, + getRootSpan, getTraceData, startInactiveSpan, startSpan, @@ -16,6 +18,7 @@ import { import type { Span } from '@sentry/types'; import { isString } from '@sentry/utils'; +import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; import { autoEndSpanOnResponseEnd, flushSafelyWithTimeout } from './responseEnd'; import { commonObjectToIsolationScope, escapeNextjsTracing } from './tracingUtils'; import { vercelWaitUntil } from './vercelWaitUntil'; @@ -94,6 +97,15 @@ export function withTracedServerSideDataFetcher Pr this: unknown, ...args: Parameters ): Promise<{ data: ReturnType; sentryTrace?: string; baggage?: string }> { + // Since the spans emitted by Next.js are super buggy with completely wrong timestamps + // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally + // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think + // about keeping them. + const nextJsOwnedSpan = getActiveSpan(); + if (nextJsOwnedSpan) { + getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); + } + return escapeNextjsTracing(() => { const isolationScope = commonObjectToIsolationScope(req); return withIsolationScope(isolationScope, () => { diff --git a/packages/nextjs/src/common/withServerActionInstrumentation.ts b/packages/nextjs/src/common/withServerActionInstrumentation.ts index c1633d8fab1b..0fb0b592fcc9 100644 --- a/packages/nextjs/src/common/withServerActionInstrumentation.ts +++ b/packages/nextjs/src/common/withServerActionInstrumentation.ts @@ -1,7 +1,9 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, SPAN_STATUS_ERROR, + getActiveSpan, getIsolationScope, + getRootSpan, withIsolationScope, } from '@sentry/core'; import { captureException, continueTrace, getClient, handleCallbackErrors, startSpan } from '@sentry/core'; @@ -9,6 +11,7 @@ import { logger } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils'; +import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from './span-attributes-with-logic-attached'; import { flushSafelyWithTimeout } from './utils/responseEnd'; import { escapeNextjsTracing } from './utils/tracingUtils'; import { vercelWaitUntil } from './utils/vercelWaitUntil'; @@ -65,6 +68,15 @@ async function withServerActionInstrumentationImplementation> { + // Since the spans emitted by Next.js are super buggy with completely wrong timestamps + // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally + // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think + // about keeping them. + const nextJsOwnedSpan = getActiveSpan(); + if (nextJsOwnedSpan) { + getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); + } + return escapeNextjsTracing(() => { return withIsolationScope(async isolationScope => { const sendDefaultPii = getClient()?.getOptions().sendDefaultPii; diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index 5944b520f6ea..d165f639930a 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -49,9 +49,6 @@ export function wrapGenerationFunctionWithSentry a 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); } let data: Record | undefined = undefined; diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index e8d734a90ff3..f17c082d5e1b 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -44,9 +44,6 @@ export function wrapServerComponentWithSentry any> 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); } const headersDict = context.headers ? winterCGHeadersToDict(context.headers) : undefined; diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 5ac96c0eac77..1366148745e2 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -1,5 +1,4 @@ import { - SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, applySdkMetadata, getClient, @@ -190,15 +189,8 @@ export function init(options: NodeOptions): NodeClient | undefined { // 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); span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto'); } - - // 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( @@ -212,15 +204,6 @@ export function init(options: NodeOptions): NodeClient | undefined { return null; } - // 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?.[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; - } - // Filter out transactions for requests to the tunnel route if ( globalWithInjectedValues.__sentryRewritesTunnelPath__ && @@ -299,16 +282,18 @@ export function init(options: NodeOptions): NodeClient | undefined { (event => { // Sometimes, the HTTP integration will not work, causing us not to properly set an op for spans generated by // Next.js that are actually more or less correct server HTTP spans, so we are backfilling the op here. - if ( - event.type === 'transaction' && - event.transaction?.match(/^(RSC )?GET /) && - event.contexts?.trace?.data?.['sentry.rsc'] === true && - !event.contexts.trace.op - ) { - event.contexts.trace.data = event.contexts.trace.data || {}; - event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_OP] = 'http.server'; - event.contexts.trace.op = 'http.server'; - } + // if ( + // event.type === 'transaction' && + // event.transaction?.match(/^(RSC )?GET /) && + // event.contexts?.trace?.data?.['sentry.rsc'] === true && + // !event.contexts.trace.op + // ) { + // event.contexts.trace.data = event.contexts.trace.data || {}; + // event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_OP] = 'http.server'; + // event.contexts.trace.op = 'http.server'; + // } + + // TODO: Add logic here to backfill things like, parameterized transaction name, op, request interface, etc. return event; }) satisfies EventProcessor, diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index 18c935863b75..d00319ec2c98 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -345,7 +345,6 @@ function removeSentryAttributes(data: Record): Record { source: 'route', }, ], - [ - "should not do any data parsing when the 'sentry.skip_span_data_inference' attribute is set", - { - 'sentry.skip_span_data_inference': true, - - // All of these should be ignored - [SEMATTRS_HTTP_METHOD]: 'GET', - [SEMATTRS_DB_SYSTEM]: 'mysql', - [SEMATTRS_DB_STATEMENT]: 'SELECT * from users', - }, - 'test name', - undefined, - { - op: undefined, - description: 'test name', - source: 'custom', - data: { - 'sentry.skip_span_data_inference': undefined, - }, - }, - ], ])('%s', (_, attributes, name, kind, expected) => { const actual = parseSpanDescription({ attributes, kind, name } as unknown as Span); expect(actual).toEqual(expected); From 8fb46dbb988c0d41be538589e49f69b4dfb1f439 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 14:53:57 +0000 Subject: [PATCH 03/12] Update route handler logic --- .../src/common/wrapRouteHandlerWithSentry.ts | 1 - packages/nextjs/src/server/index.ts | 34 +++++++------------ 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts index ad70c865dedf..a789cb80aadf 100644 --- a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts @@ -51,7 +51,6 @@ export function wrapRouteHandlerWithSentry any>( const activeSpan = getActiveSpan(); if (activeSpan) { const rootSpan = getRootSpan(activeSpan); - rootSpan.setAttribute('sentry.route_handler', true); const { scope } = getCapturedScopesOnSpan(rootSpan); setCapturedScopesOnSpan(rootSpan, scope ?? new Scope(), isolationScope); } diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index a47aed3c67df..a9a2cebbe5eb 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -1,6 +1,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, applySdkMetadata, getClient, getGlobalScope, @@ -284,32 +285,21 @@ export function init(options: NodeOptions): NodeClient | undefined { getGlobalScope().addEventProcessor( Object.assign( (event => { - // Sometimes, the HTTP integration will not work, causing us not to properly set an op for spans generated by - // Next.js that are actually more or less correct server HTTP spans, so we are backfilling the op here. - - // if ( - // event.type === 'transaction' && - // event.transaction?.match(/^(RSC )?GET /) && - // event.contexts?.trace?.data?.['sentry.rsc'] === true && - // !event.contexts.trace.op - // ) { - // event.contexts.trace.data = event.contexts.trace.data || {}; - // event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_OP] = 'http.server'; - // event.contexts.trace.op = 'http.server'; - // } - - // TODO: Add logic here to backfill things like, parameterized transaction name, op, request interface, etc. - // Enhance route handler transactions - if (event.type === 'transaction' && event.contexts?.trace?.data?.['sentry.route_handler'] === true) { + if ( + event.type === 'transaction' && + event.contexts?.trace?.data?.['next.span_type'] === 'BaseServer.handleRequest' + ) { event.contexts.trace.data = event.contexts.trace.data || {}; event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_OP] = 'http.server'; event.contexts.trace.op = 'http.server'; - if (typeof event.contexts.trace.data[ATTR_HTTP_ROUTE] === 'string') { - // eslint-disable-next-line deprecation/deprecation - event.transaction = `${event.contexts.trace.data[SEMATTRS_HTTP_METHOD]} ${event.contexts.trace.data[ - ATTR_HTTP_ROUTE - ].replace(/\/route$/, '')}`; + + // eslint-disable-next-line deprecation/deprecation + const method = event.contexts.trace.data[SEMATTRS_HTTP_METHOD]; + const route = event.contexts.trace.data[ATTR_HTTP_ROUTE]; + if (typeof method === 'string' && typeof route === 'string') { + event.transaction = `${method} ${route.replace(/\/route$/, '')}`; + event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] = 'route'; } } From 4fc432064d465c7d27ee3bc8fb27ed1eaab49294 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 9 Oct 2024 09:43:20 +0000 Subject: [PATCH 04/12] Make E2E test script less verbose --- dev-packages/e2e-tests/publish-packages.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/dev-packages/e2e-tests/publish-packages.ts b/dev-packages/e2e-tests/publish-packages.ts index 408d046977a2..4f2cc4056826 100644 --- a/dev-packages/e2e-tests/publish-packages.ts +++ b/dev-packages/e2e-tests/publish-packages.ts @@ -12,6 +12,8 @@ const packageTarballPaths = glob.sync('packages/*/sentry-*.tgz', { // Publish built packages to the fake registry packageTarballPaths.forEach(tarballPath => { + // eslint-disable-next-line no-console + console.log(`Publishing tarball ${tarballPath} ...`); // `--userconfig` flag needs to be before `publish` childProcess.exec( `npm --userconfig ${__dirname}/test-registry.npmrc publish ${tarballPath}`, @@ -19,14 +21,10 @@ packageTarballPaths.forEach(tarballPath => { cwd: repositoryRoot, // Can't use __dirname here because npm would try to publish `@sentry-internal/e2e-tests` encoding: 'utf8', }, - (err, stdout, stderr) => { - // eslint-disable-next-line no-console - console.log(stdout); - // eslint-disable-next-line no-console - console.log(stderr); + err => { if (err) { // eslint-disable-next-line no-console - console.error(err); + console.error(`Error publishing tarball ${tarballPath}`, err); process.exit(1); } }, From e13abef6498a047c9483ac4c629ebfdb9bd9dfd4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 9 Oct 2024 09:45:03 +0000 Subject: [PATCH 05/12] Extract dropping utility into separate function --- .../wrapApiHandlerWithSentry.ts | 15 ++--------- .../wrapApiHandlerWithSentryVercelCrons.ts | 2 +- .../wrapPageComponentWithSentry.ts | 25 +++---------------- .../src/common/utils/edgeWrapperUtils.ts | 15 ++--------- .../nextjs/src/common/utils/tracingUtils.ts | 19 +++++++++++++- .../nextjs/src/common/utils/wrapperUtils.ts | 15 ++--------- .../common/withServerActionInstrumentation.ts | 21 ++++++---------- 7 files changed, 36 insertions(+), 76 deletions(-) diff --git a/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts index 5f1b6061d592..ed920eee58ba 100644 --- a/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts @@ -3,8 +3,6 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, captureException, continueTrace, - getActiveSpan, - getRootSpan, setHttpStatus, startSpanManual, withIsolationScope, @@ -13,10 +11,9 @@ import { isString, logger, objectify } from '@sentry/utils'; import { vercelWaitUntil } from '@sentry/utils'; import type { NextApiRequest } from 'next'; -import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; import type { AugmentedNextApiResponse, NextApiHandler } from '../types'; import { flushSafelyWithTimeout } from '../utils/responseEnd'; -import { escapeNextjsTracing } from '../utils/tracingUtils'; +import { dropNextjsRootContext, escapeNextjsTracing } from '../utils/tracingUtils'; export type AugmentedNextApiRequest = NextApiRequest & { __withSentry_applied__?: boolean; @@ -31,21 +28,13 @@ export type AugmentedNextApiRequest = NextApiRequest & { * @returns The wrapped handler which will always return a Promise. */ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameterizedRoute: string): NextApiHandler { - // Since the API route handler spans emitted by Next.js are super buggy with completely wrong timestamps - // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally - // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think - // about keeping them. - const nextJsOwnedSpan = getActiveSpan(); - if (nextJsOwnedSpan) { - getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); - } - return new Proxy(apiHandler, { apply: ( wrappingTarget, thisArg, args: [AugmentedNextApiRequest | undefined, AugmentedNextApiResponse | undefined], ) => { + dropNextjsRootContext(); return escapeNextjsTracing(() => { const [req, res] = args; diff --git a/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentryVercelCrons.ts b/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentryVercelCrons.ts index 20ed01d51e94..5ca29b338cda 100644 --- a/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentryVercelCrons.ts +++ b/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentryVercelCrons.ts @@ -9,7 +9,7 @@ type EdgeRequest = { }; /** - * Wraps a function with Sentry crons instrumentation by automaticaly sending check-ins for the given Vercel crons config. + * Wraps a function with Sentry crons instrumentation by automatically sending check-ins for the given Vercel crons config. */ // eslint-disable-next-line @typescript-eslint/no-explicit-any export function wrapApiHandlerWithSentryVercelCrons any>( diff --git a/packages/nextjs/src/common/pages-router-instrumentation/wrapPageComponentWithSentry.ts b/packages/nextjs/src/common/pages-router-instrumentation/wrapPageComponentWithSentry.ts index 535c88b82f88..2914366f9a6b 100644 --- a/packages/nextjs/src/common/pages-router-instrumentation/wrapPageComponentWithSentry.ts +++ b/packages/nextjs/src/common/pages-router-instrumentation/wrapPageComponentWithSentry.ts @@ -1,7 +1,6 @@ -import { captureException, getActiveSpan, getCurrentScope, getRootSpan, withIsolationScope } from '@sentry/core'; +import { captureException, getCurrentScope, withIsolationScope } from '@sentry/core'; import { extractTraceparentData } from '@sentry/utils'; -import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; -import { escapeNextjsTracing } from '../utils/tracingUtils'; +import { dropNextjsRootContext, escapeNextjsTracing } from '../utils/tracingUtils'; interface FunctionComponent { (...args: unknown[]): unknown; @@ -26,15 +25,7 @@ export function wrapPageComponentWithSentry(pageComponent: FunctionComponent | C if (isReactClassComponent(pageComponent)) { return class SentryWrappedPageComponent extends pageComponent { public render(...args: unknown[]): unknown { - // Since the spans emitted by Next.js are super buggy with completely wrong timestamps - // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally - // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think - // about keeping them. - const nextJsOwnedSpan = getActiveSpan(); - if (nextJsOwnedSpan) { - getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); - } - + dropNextjsRootContext(); return escapeNextjsTracing(() => { return withIsolationScope(() => { const scope = getCurrentScope(); @@ -72,15 +63,7 @@ export function wrapPageComponentWithSentry(pageComponent: FunctionComponent | C } else if (typeof pageComponent === 'function') { return new Proxy(pageComponent, { apply(target, thisArg, argArray: [{ _sentryTraceData?: string } | undefined]) { - // Since the spans emitted by Next.js are super buggy with completely wrong timestamps - // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally - // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think - // about keeping them. - const nextJsOwnedSpan = getActiveSpan(); - if (nextJsOwnedSpan) { - getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); - } - + dropNextjsRootContext(); return escapeNextjsTracing(() => { return withIsolationScope(() => { const scope = getCurrentScope(); diff --git a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts index d2d350d1d242..9cfe5a66ac89 100644 --- a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts +++ b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts @@ -4,8 +4,6 @@ import { SPAN_STATUS_OK, captureException, continueTrace, - getActiveSpan, - getRootSpan, handleCallbackErrors, setHttpStatus, startSpan, @@ -14,9 +12,8 @@ import { import { vercelWaitUntil, winterCGRequestToRequestData } from '@sentry/utils'; import type { EdgeRouteHandler } from '../../edge/types'; -import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; import { flushSafelyWithTimeout } from './responseEnd'; -import { commonObjectToIsolationScope, escapeNextjsTracing } from './tracingUtils'; +import { commonObjectToIsolationScope, dropNextjsRootContext, escapeNextjsTracing } from './tracingUtils'; /** * Wraps a function on the edge runtime with error and performance monitoring. @@ -26,15 +23,7 @@ export function withEdgeWrapping( options: { spanDescription: string; spanOp: string; mechanismFunctionName: string }, ): (...params: Parameters) => Promise> { return async function (this: unknown, ...args) { - // Since the spans emitted by Next.js are super buggy with completely wrong timestamps - // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally - // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think - // about keeping them. - const nextJsOwnedSpan = getActiveSpan(); - if (nextJsOwnedSpan) { - getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); - } - + dropNextjsRootContext(); return escapeNextjsTracing(() => { const req: unknown = args[0]; return withIsolationScope(commonObjectToIsolationScope(req), isolationScope => { diff --git a/packages/nextjs/src/common/utils/tracingUtils.ts b/packages/nextjs/src/common/utils/tracingUtils.ts index b996b6af1877..ff57fcae3acc 100644 --- a/packages/nextjs/src/common/utils/tracingUtils.ts +++ b/packages/nextjs/src/common/utils/tracingUtils.ts @@ -1,7 +1,8 @@ -import { Scope, startNewTrace } from '@sentry/core'; +import { Scope, getActiveSpan, getRootSpan, spanToJSON, startNewTrace } from '@sentry/core'; import type { PropagationContext } from '@sentry/types'; import { GLOBAL_OBJ, logger } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; +import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; const commonPropagationContextMap = new WeakMap(); @@ -92,3 +93,19 @@ export function escapeNextjsTracing(cb: () => T): T { }); } } + +/** + * Ideally this function never lands in the develop branch. + * + * Drops the entire span tree this function was called in, if it was a span tree created by Next.js. + */ +export function dropNextjsRootContext(): void { + const nextJsOwnedSpan = getActiveSpan(); + if (nextJsOwnedSpan) { + const rootSpan = getRootSpan(nextJsOwnedSpan); + const rootSpanAttributes = spanToJSON(rootSpan).data; + if (rootSpanAttributes?.['next.span_type']) { + getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); + } + } +} diff --git a/packages/nextjs/src/common/utils/wrapperUtils.ts b/packages/nextjs/src/common/utils/wrapperUtils.ts index 403157a75f72..ddde0be63a18 100644 --- a/packages/nextjs/src/common/utils/wrapperUtils.ts +++ b/packages/nextjs/src/common/utils/wrapperUtils.ts @@ -6,8 +6,6 @@ import { SPAN_STATUS_OK, captureException, continueTrace, - getActiveSpan, - getRootSpan, getTraceData, startInactiveSpan, startSpan, @@ -18,9 +16,8 @@ import { import type { Span } from '@sentry/types'; import { isString, vercelWaitUntil } from '@sentry/utils'; -import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached'; import { autoEndSpanOnResponseEnd, flushSafelyWithTimeout } from './responseEnd'; -import { commonObjectToIsolationScope, escapeNextjsTracing } from './tracingUtils'; +import { commonObjectToIsolationScope, dropNextjsRootContext, escapeNextjsTracing } from './tracingUtils'; declare module 'http' { interface IncomingMessage { @@ -96,15 +93,7 @@ export function withTracedServerSideDataFetcher Pr this: unknown, ...args: Parameters ): Promise<{ data: ReturnType; sentryTrace?: string; baggage?: string }> { - // Since the spans emitted by Next.js are super buggy with completely wrong timestamps - // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally - // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think - // about keeping them. - const nextJsOwnedSpan = getActiveSpan(); - if (nextJsOwnedSpan) { - getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); - } - + dropNextjsRootContext(); return escapeNextjsTracing(() => { const isolationScope = commonObjectToIsolationScope(req); return withIsolationScope(isolationScope, () => { diff --git a/packages/nextjs/src/common/withServerActionInstrumentation.ts b/packages/nextjs/src/common/withServerActionInstrumentation.ts index a15de7683cf8..d1a37a7cab76 100644 --- a/packages/nextjs/src/common/withServerActionInstrumentation.ts +++ b/packages/nextjs/src/common/withServerActionInstrumentation.ts @@ -1,19 +1,20 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, SPAN_STATUS_ERROR, - getActiveSpan, + captureException, + continueTrace, + getClient, getIsolationScope, - getRootSpan, + handleCallbackErrors, + startSpan, withIsolationScope, } from '@sentry/core'; -import { captureException, continueTrace, getClient, handleCallbackErrors, startSpan } from '@sentry/core'; import { logger, vercelWaitUntil } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils'; -import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from './span-attributes-with-logic-attached'; import { flushSafelyWithTimeout } from './utils/responseEnd'; -import { escapeNextjsTracing } from './utils/tracingUtils'; +import { dropNextjsRootContext, escapeNextjsTracing } from './utils/tracingUtils'; interface Options { formData?: FormData; @@ -67,15 +68,7 @@ async function withServerActionInstrumentationImplementation> { - // Since the spans emitted by Next.js are super buggy with completely wrong timestamps - // (fix pending at the time of writing this: https://github.com/vercel/next.js/pull/70908) we want to intentionally - // drop them. In the future, when Next.js' OTEL instrumentation is in a high-quality place we can potentially think - // about keeping them. - const nextJsOwnedSpan = getActiveSpan(); - if (nextJsOwnedSpan) { - getRootSpan(nextJsOwnedSpan)?.setAttribute(TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION, true); - } - + dropNextjsRootContext(); return escapeNextjsTracing(() => { return withIsolationScope(async isolationScope => { const sendDefaultPii = getClient()?.getOptions().sendDefaultPii; From 80745fcd10472e0f29d549f84ea3d8485ef2751a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 9 Oct 2024 11:31:00 +0000 Subject: [PATCH 06/12] Don't drop for edge? --- packages/nextjs/src/common/utils/edgeWrapperUtils.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts index 9cfe5a66ac89..5eed59aca0a3 100644 --- a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts +++ b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts @@ -13,7 +13,7 @@ import { vercelWaitUntil, winterCGRequestToRequestData } from '@sentry/utils'; import type { EdgeRouteHandler } from '../../edge/types'; import { flushSafelyWithTimeout } from './responseEnd'; -import { commonObjectToIsolationScope, dropNextjsRootContext, escapeNextjsTracing } from './tracingUtils'; +import { commonObjectToIsolationScope, escapeNextjsTracing } from './tracingUtils'; /** * Wraps a function on the edge runtime with error and performance monitoring. @@ -23,7 +23,6 @@ export function withEdgeWrapping( options: { spanDescription: string; spanOp: string; mechanismFunctionName: string }, ): (...params: Parameters) => Promise> { return async function (this: unknown, ...args) { - dropNextjsRootContext(); return escapeNextjsTracing(() => { const req: unknown = args[0]; return withIsolationScope(commonObjectToIsolationScope(req), isolationScope => { From 10829d60ef7eb06d84ca8007711bd1bebe343153 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 9 Oct 2024 11:32:30 +0000 Subject: [PATCH 07/12] Actually factually don't drop Next.js OTEL spans anymore --- packages/nextjs/src/server/index.ts | 42 ++++------------------------- 1 file changed, 5 insertions(+), 37 deletions(-) diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index a9a2cebbe5eb..dba187628cb4 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -36,22 +36,6 @@ const globalWithInjectedValues = GLOBAL_OBJ as typeof GLOBAL_OBJ & { __sentryRewritesTunnelPath__?: string; }; -// https://github.com/lforst/nextjs-fork/blob/9051bc44d969a6e0ab65a955a2fc0af522a83911/packages/next/src/server/lib/trace/constants.ts#L11 -const NEXTJS_SPAN_NAME_PREFIXES = [ - 'BaseServer.', - 'LoadComponents.', - 'NextServer.', - 'createServer.', - 'startServer.', - 'NextNodeServer.', - 'Render.', - 'AppRender.', - 'Router.', - 'Node.', - 'AppRouteRouteHandlers.', - 'ResolveMetadata.', -]; - /** * A passthrough error boundary for the server that doesn't depend on any react. Error boundaries don't catch SSR errors * so they should simply be a passthrough. @@ -135,25 +119,7 @@ export function init(options: NodeOptions): NodeClient | undefined { applySdkMetadata(opts, 'nextjs', ['nextjs', 'node']); const client = nodeInit(opts); - client?.on('beforeSampling', ({ spanAttributes, spanName, parentSampled, parentContext }, samplingDecision) => { - // 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') { - 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; - } - + client?.on('beforeSampling', ({ spanAttributes }, samplingDecision) => { // 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. @@ -178,7 +144,7 @@ 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` attribute, up to the transaction by setting the http.route attribute. - if (spanAttributes?.['next.route']) { + if (typeof spanAttributes?.['next.route'] === 'string') { const rootSpan = getRootSpan(span); const rootSpanAttributes = spanToJSON(rootSpan).data; @@ -188,7 +154,9 @@ export function init(options: NodeOptions): NodeClient | undefined { (rootSpanAttributes?.[ATTR_HTTP_REQUEST_METHOD] || rootSpanAttributes?.[SEMATTRS_HTTP_METHOD]) && !rootSpanAttributes?.[ATTR_HTTP_ROUTE] ) { - rootSpan.setAttribute(ATTR_HTTP_ROUTE, spanAttributes['next.route']); + const route = spanAttributes['next.route'].replace(/\/route$/, ''); + rootSpan.updateName(route); + rootSpan.setAttribute(ATTR_HTTP_ROUTE, route); } } From f8cf09123921b72b87c0dadd99ec911d0365ad2c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 9 Oct 2024 11:47:51 +0000 Subject: [PATCH 08/12] Drop edge nextjs context --- packages/nextjs/src/common/utils/edgeWrapperUtils.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts index 5eed59aca0a3..9cfe5a66ac89 100644 --- a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts +++ b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts @@ -13,7 +13,7 @@ import { vercelWaitUntil, winterCGRequestToRequestData } from '@sentry/utils'; import type { EdgeRouteHandler } from '../../edge/types'; import { flushSafelyWithTimeout } from './responseEnd'; -import { commonObjectToIsolationScope, escapeNextjsTracing } from './tracingUtils'; +import { commonObjectToIsolationScope, dropNextjsRootContext, escapeNextjsTracing } from './tracingUtils'; /** * Wraps a function on the edge runtime with error and performance monitoring. @@ -23,6 +23,7 @@ export function withEdgeWrapping( options: { spanDescription: string; spanOp: string; mechanismFunctionName: string }, ): (...params: Parameters) => Promise> { return async function (this: unknown, ...args) { + dropNextjsRootContext(); return escapeNextjsTracing(() => { const req: unknown = args[0]; return withIsolationScope(commonObjectToIsolationScope(req), isolationScope => { From c62392fb251a6f1cd24bc7acef632080ac1d9c28 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 9 Oct 2024 12:32:04 +0000 Subject: [PATCH 09/12] ref: Disable incoming HTTP request instrumentation --- .../tests/route-handlers.test.ts | 2 +- .../tests/server-components.test.ts | 4 +-- packages/nextjs/src/server/index.ts | 30 ++++++++++++++++--- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts index 3e8006ae21f8..90d71865dd3b 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts @@ -56,7 +56,7 @@ test('Should record exceptions and transactions for faulty route handlers', asyn expect(routehandlerTransaction.contexts?.trace?.status).toBe('internal_error'); expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server'); - expect(routehandlerTransaction.contexts?.trace?.origin).toContain('auto.http.otel.http'); + expect(routehandlerTransaction.contexts?.trace?.origin).toContain('auto'); expect(routehandlerError.exception?.values?.[0].value).toBe('route-handler-error'); 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 49afe791328f..75f30075a47f 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 @@ -16,7 +16,7 @@ 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': 'auto.http.otel.http', + 'sentry.origin': 'auto', 'sentry.sample_rate': 1, 'sentry.source': 'route', 'http.method': 'GET', @@ -27,7 +27,7 @@ test('Sends a transaction for a request to app router', async ({ page }) => { 'otel.kind': 'SERVER', }), op: 'http.server', - origin: 'auto.http.otel.http', + origin: 'auto', span_id: expect.any(String), status: 'ok', trace_id: expect.any(String), diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 8e00a325f5f5..d113cc83b815 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -1,15 +1,16 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, applySdkMetadata, getClient, getGlobalScope, getRootSpan, spanToJSON, } from '@sentry/core'; -import { getDefaultIntegrations, init as nodeInit } from '@sentry/node'; +import { getDefaultIntegrations, httpIntegration, init as nodeInit } from '@sentry/node'; import type { NodeClient, NodeOptions } from '@sentry/node'; -import { GLOBAL_OBJ, logger } from '@sentry/utils'; +import { GLOBAL_OBJ, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import { ATTR_HTTP_REQUEST_METHOD, @@ -99,7 +100,18 @@ export function init(options: NodeOptions): NodeClient | undefined { return; } - const customDefaultIntegrations = getDefaultIntegrations(options); + const customDefaultIntegrations = getDefaultIntegrations(options) + .filter(integration => integration.name !== 'Http') + .concat( + // We are using the HTTP integration without instrumenting incoming HTTP requests because Next.js does that by itself. + httpIntegration({ + instrumentation: { + _experimentalConfig: { + disableIncomingRequestInstrumentation: true, + }, + }, + }), + ); // Turn off Next.js' own fetch instrumentation // https://github.com/lforst/nextjs-fork/blob/1994fd186defda77ad971c36dc3163db263c993f/packages/next/src/server/lib/patch-fetch.ts#L245 @@ -319,15 +331,25 @@ export function init(options: NodeOptions): NodeClient | undefined { } // Enhance route handler transactions - if (event.type === 'transaction' && event.contexts?.trace?.data?.['sentry.route_handler'] === true) { + if ( + event.type === 'transaction' && + (event.contexts?.trace?.data?.['sentry.route_handler'] === true || + event.contexts?.trace?.data?.['sentry.rsc'] === true) + ) { event.contexts.trace.data = event.contexts.trace.data || {}; event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_OP] = 'http.server'; event.contexts.trace.op = 'http.server'; + + if (event.transaction) { + event.transaction = stripUrlQueryAndFragment(event.transaction); + } + if (typeof event.contexts.trace.data[ATTR_HTTP_ROUTE] === 'string') { // eslint-disable-next-line deprecation/deprecation event.transaction = `${event.contexts.trace.data[SEMATTRS_HTTP_METHOD]} ${event.contexts.trace.data[ ATTR_HTTP_ROUTE ].replace(/\/route$/, '')}`; + event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] = 'route'; } } From 89f278e89d0e034c703e4cf534290ec3e107e640 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 9 Oct 2024 15:20:34 +0000 Subject: [PATCH 10/12] Don't drop for edge --- packages/nextjs/src/common/utils/edgeWrapperUtils.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts index 9cfe5a66ac89..5eed59aca0a3 100644 --- a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts +++ b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts @@ -13,7 +13,7 @@ import { vercelWaitUntil, winterCGRequestToRequestData } from '@sentry/utils'; import type { EdgeRouteHandler } from '../../edge/types'; import { flushSafelyWithTimeout } from './responseEnd'; -import { commonObjectToIsolationScope, dropNextjsRootContext, escapeNextjsTracing } from './tracingUtils'; +import { commonObjectToIsolationScope, escapeNextjsTracing } from './tracingUtils'; /** * Wraps a function on the edge runtime with error and performance monitoring. @@ -23,7 +23,6 @@ export function withEdgeWrapping( options: { spanDescription: string; spanOp: string; mechanismFunctionName: string }, ): (...params: Parameters) => Promise> { return async function (this: unknown, ...args) { - dropNextjsRootContext(); return escapeNextjsTracing(() => { const req: unknown = args[0]; return withIsolationScope(commonObjectToIsolationScope(req), isolationScope => { From 38fc9913614c2c485226fbb634927e54392b13a4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 10 Oct 2024 09:25:30 +0000 Subject: [PATCH 11/12] Don't prefetch route for navigation link in tests --- .../e2e-tests/test-applications/nextjs-app-dir/app/layout.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/layout.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/layout.tsx index d2aae8c9cd8d..15c3b9789ee3 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/layout.tsx +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/layout.tsx @@ -27,7 +27,9 @@ export default function Layout({ children }: { children: React.ReactNode }) { /server-component/parameter/42
  • - /server-component/parameter/foo/bar/baz + + /server-component/parameter/foo/bar/baz +
  • /not-found From 7e9a40622f525dc309173fff7c92e3ef9712ea71 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 10 Oct 2024 11:29:32 +0000 Subject: [PATCH 12/12] Skip tests that do no longer make sense --- .../nextjs-13/tests/server/excluded-api-endpoints.test.ts | 8 ++++++-- packages/nextjs/src/config/types.ts | 7 +++++-- packages/nextjs/src/server/index.ts | 5 +++++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts index 63082fee6e07..b328f35d0721 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts @@ -1,7 +1,9 @@ import { expect, test } from '@playwright/test'; import { waitForTransaction } from '@sentry-internal/test-utils'; -test('should not automatically create transactions for routes that were excluded from auto wrapping (string)', async ({ +// TODO(lforst): This cannot make it into production - Make sure to fix this test +// The problem is that if we are not applying build time instrumentation Next.js will still emit spans (which is fine, but we need to find a different way of testing that build time instrumentation is successfully disabled - maybe with an attribute or something if build-time instrumentation is applied) +test.skip('should not automatically create transactions for routes that were excluded from auto wrapping (string)', async ({ request, }) => { const transactionPromise = waitForTransaction('nextjs-13', async transactionEvent => { @@ -23,7 +25,9 @@ test('should not automatically create transactions for routes that were excluded expect(transactionPromiseReceived).toBe(false); }); -test('should not automatically create transactions for routes that were excluded from auto wrapping (regex)', async ({ +// TODO(lforst): This cannot make it into production - Make sure to fix this test +// The problem is that if we are not applying build time instrumentation Next.js will still emit spans (which is fine, but we need to find a different way of testing that build time instrumentation is successfully disabled - maybe with an attribute or something if build-time instrumentation is applied) +test.skip('should not automatically create transactions for routes that were excluded from auto wrapping (regex)', async ({ request, }) => { const transactionPromise = waitForTransaction('nextjs-13', async transactionEvent => { diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index d2e78d87f4ae..63f678cdbc66 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -409,12 +409,15 @@ export type SentryBuildOptions = { autoInstrumentAppDirectory?: boolean; /** - * Exclude certain serverside API routes or pages from being instrumented with Sentry. This option takes an array of - * strings or regular expressions. This options also affects pages in the `app` directory. + * Exclude certain serverside API routes or pages from being instrumented with Sentry during build-time. This option + * takes an array of strings or regular expressions. This options also affects pages in the `app` directory. * * NOTE: Pages should be specified as routes (`/animals` or `/api/animals/[animalType]/habitat`), not filepaths * (`pages/animals/index.js` or `.\src\pages\api\animals\[animalType]\habitat.tsx`), and strings must be be a full, * exact match. + * + * Notice: If you build Next.js with turbopack, the Sentry SDK will no longer apply build-time instrumentation and + * purely rely on Next.js telemetry features, meaning that this option will effectively no-op. */ excludeServerRoutes?: Array; diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 2961df2fae1b..168288e081fe 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -332,6 +332,11 @@ export function init(options: NodeOptions): NodeClient | undefined { } } + // Next.js 13 sometimes names the root transactions like this containing useless tracing. + if (event.type === 'transaction' && event.transaction === 'NextServer.getRequestHandler') { + return null; + } + return event; }) satisfies EventProcessor, { id: 'NextjsTransactionEnhancer' },