From 7d3a193cd73b30a8a746de09931709bf4e4e7b38 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 29 Jan 2024 17:00:41 +0100 Subject: [PATCH 1/8] feat(nextjs): Add `browserTracingIntegration` --- .../src/client/browserTracingIntegration.ts | 45 ++++++++++++++++++- packages/nextjs/src/client/index.ts | 6 ++- .../appRouterRoutingInstrumentation.ts | 22 ++++++--- .../routing/nextRoutingInstrumentation.ts | 25 +++++++++-- .../pagesRouterRoutingInstrumentation.ts | 17 ++++--- 5 files changed, 97 insertions(+), 18 deletions(-) diff --git a/packages/nextjs/src/client/browserTracingIntegration.ts b/packages/nextjs/src/client/browserTracingIntegration.ts index c3eb18887301..c0f85a17b671 100644 --- a/packages/nextjs/src/client/browserTracingIntegration.ts +++ b/packages/nextjs/src/client/browserTracingIntegration.ts @@ -1,4 +1,9 @@ -import { BrowserTracing as OriginalBrowserTracing, defaultRequestInstrumentationOptions } from '@sentry/react'; +import { + BrowserTracing as OriginalBrowserTracing, + browserTracingIntegration as originalBrowserTracingIntegration, + defaultRequestInstrumentationOptions, +} from '@sentry/react'; +import type { Integration, StartSpanOptions } from '@sentry/types'; import { nextRouterInstrumentation } from '../index.client'; /** @@ -19,8 +24,46 @@ export class BrowserTracing extends OriginalBrowserTracing { ] : // eslint-disable-next-line deprecation/deprecation [...defaultRequestInstrumentationOptions.tracingOrigins, /^(api\/)/], + // eslint-disable-next-line deprecation/deprecation routingInstrumentation: nextRouterInstrumentation, ...options, }); } } + +/** + * TODO + */ +export function browserTracingIntegration( + options?: Parameters[0], +): Integration { + const browserTracingIntegrationInstance = originalBrowserTracingIntegration({ + ...options, + instrumentNavigation: false, + instrumentPageLoad: false, + }); + + return { + ...browserTracingIntegrationInstance, + afterAllSetup(client) { + const startPageloadCallback = (startSpanOptions: StartSpanOptions): void => { + if (!client.emit) { + return; + } + client.emit('startPageLoadSpan', startSpanOptions); + return undefined; + }; + + const startNavigationCallback = (startSpanOptions: StartSpanOptions): void => { + if (!client.emit) { + return; + } + client.emit('startNavigationSpan', startSpanOptions); + return undefined; + }; + + // eslint-disable-next-line deprecation/deprecation + nextRouterInstrumentation(() => undefined, true, true, startPageloadCallback, startNavigationCallback); + }, + }; +} diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index a1c20937f578..ddea9dee3505 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -1,5 +1,5 @@ import { applySdkMetadata, hasTracingEnabled } from '@sentry/core'; -import type { BrowserOptions, browserTracingIntegration } from '@sentry/react'; +import type { BrowserOptions } from '@sentry/react'; import { Integrations as OriginalIntegrations, getCurrentScope, @@ -10,11 +10,13 @@ import type { EventProcessor, Integration } from '@sentry/types'; import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolicationEventProcessor'; import { getVercelEnv } from '../common/getVercelEnv'; +import type { browserTracingIntegration } from './browserTracingIntegration'; import { BrowserTracing } from './browserTracingIntegration'; import { rewriteFramesIntegration } from './rewriteFramesIntegration'; import { applyTunnelRouteOption } from './tunnelRoute'; export * from '@sentry/react'; +// eslint-disable-next-line deprecation/deprecation export { nextRouterInstrumentation } from './routing/nextRoutingInstrumentation'; export { captureUnderscoreErrorException } from '../common/_error'; @@ -140,4 +142,6 @@ export function withSentryConfig(exportedUserNextConfig: T): T { return exportedUserNextConfig; } +export { browserTracingIntegration }; + export * from '../common'; diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 3083013e084a..78b2612b293f 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -1,30 +1,33 @@ import { WINDOW } from '@sentry/react'; -import type { Primitive, Transaction, TransactionContext } from '@sentry/types'; +import type { Primitive, Span, StartSpanOptions, Transaction, TransactionContext } from '@sentry/types'; import { addFetchInstrumentationHandler, browserPerformanceTimeOrigin } from '@sentry/utils'; type StartTransactionCb = (context: TransactionContext) => Transaction | undefined; +type StartSpanCb = (context: StartSpanOptions) => void; const DEFAULT_TAGS = { 'routing.instrumentation': 'next-app-router', } as const; /** - * Instruments the Next.js Clientside App Router. + * Instruments the Next.js Client App Router. */ export function appRouterInstrumentation( startTransactionCb: StartTransactionCb, startTransactionOnPageLoad: boolean = true, startTransactionOnLocationChange: boolean = true, + startPageloadSpanCallback: StartSpanCb, + startNavigationSpanCallback: StartSpanCb, ): void { // We keep track of the active transaction so we can finish it when we start a navigation transaction. - let activeTransaction: Transaction | undefined = undefined; + let activeTransaction: Span | undefined = undefined; // We keep track of the previous location name so we can set the `from` field on navigation transactions. // This is either a route or a pathname. let prevLocationName = WINDOW.location.pathname; if (startTransactionOnPageLoad) { - activeTransaction = startTransactionCb({ + const transactionContext = { name: prevLocationName, op: 'pageload', origin: 'auto.pageload.nextjs.app_router_instrumentation', @@ -32,7 +35,9 @@ export function appRouterInstrumentation( // pageload should always start at timeOrigin (and needs to be in s, not ms) startTimestamp: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined, metadata: { source: 'url' }, - }); + } as const; + activeTransaction = startTransactionCb(transactionContext); + startPageloadSpanCallback(transactionContext); } if (startTransactionOnLocationChange) { @@ -66,13 +71,16 @@ export function appRouterInstrumentation( activeTransaction.end(); } - startTransactionCb({ + const transactionContext = { name: transactionName, op: 'navigation', origin: 'auto.navigation.nextjs.app_router_instrumentation', tags, metadata: { source: 'url' }, - }); + } as const; + + startTransactionCb(transactionContext); + startNavigationSpanCallback(transactionContext); }); } } diff --git a/packages/nextjs/src/client/routing/nextRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/nextRoutingInstrumentation.ts index 3010faad4183..4706fb8a32f2 100644 --- a/packages/nextjs/src/client/routing/nextRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/nextRoutingInstrumentation.ts @@ -1,23 +1,40 @@ import { WINDOW } from '@sentry/react'; -import type { Transaction, TransactionContext } from '@sentry/types'; +import type { StartSpanOptions, Transaction, TransactionContext } from '@sentry/types'; import { appRouterInstrumentation } from './appRouterRoutingInstrumentation'; import { pagesRouterInstrumentation } from './pagesRouterRoutingInstrumentation'; type StartTransactionCb = (context: TransactionContext) => Transaction | undefined; +type StartSpanCb = (context: StartSpanOptions) => void; /** - * Instruments the Next.js Clientside Router. + * Instruments the Next.js Client Router. + * + * @deprecated Use `browserTracingIntegration()` as exported from `@sentry/nextjs` instead. */ export function nextRouterInstrumentation( startTransactionCb: StartTransactionCb, startTransactionOnPageLoad: boolean = true, startTransactionOnLocationChange: boolean = true, + startPageloadSpanCallback?: StartSpanCb, + startNavigationSpanCallback?: StartSpanCb, ): void { const isAppRouter = !WINDOW.document.getElementById('__NEXT_DATA__'); if (isAppRouter) { - appRouterInstrumentation(startTransactionCb, startTransactionOnPageLoad, startTransactionOnLocationChange); + appRouterInstrumentation( + startTransactionCb, + startTransactionOnPageLoad, + startTransactionOnLocationChange, + startPageloadSpanCallback || (() => undefined), + startNavigationSpanCallback || (() => undefined), + ); } else { - pagesRouterInstrumentation(startTransactionCb, startTransactionOnPageLoad, startTransactionOnLocationChange); + pagesRouterInstrumentation( + startTransactionCb, + startTransactionOnPageLoad, + startTransactionOnLocationChange, + startPageloadSpanCallback || (() => undefined), + startNavigationSpanCallback || (() => undefined), + ); } } diff --git a/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts index 5f2064c690e4..90ef8416679b 100644 --- a/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts @@ -1,7 +1,7 @@ import type { ParsedUrlQuery } from 'querystring'; import { getClient, getCurrentScope } from '@sentry/core'; import { WINDOW } from '@sentry/react'; -import type { Primitive, Transaction, TransactionContext, TransactionSource } from '@sentry/types'; +import type { Primitive, StartSpanOptions, Transaction, TransactionContext, TransactionSource } from '@sentry/types'; import { browserPerformanceTimeOrigin, logger, @@ -20,6 +20,7 @@ const globalObject = WINDOW as typeof WINDOW & { }; type StartTransactionCb = (context: TransactionContext) => Transaction | undefined; +type StartSpanCb = (context: StartSpanOptions) => void; /** * Describes data located in the __NEXT_DATA__ script tag. This tag is present on every page of a Next.js app. @@ -117,6 +118,8 @@ export function pagesRouterInstrumentation( startTransactionCb: StartTransactionCb, startTransactionOnPageLoad: boolean = true, startTransactionOnLocationChange: boolean = true, + startPageloadSpanCallback: StartSpanCb, + startNavigationSpanCallback: StartSpanCb, ): void { const { route, params, sentryTrace, baggage } = extractNextDataTagInformation(); const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders( @@ -129,7 +132,7 @@ export function pagesRouterInstrumentation( if (startTransactionOnPageLoad) { const source = route ? 'route' : 'url'; - activeTransaction = startTransactionCb({ + const transactionContext = { name: prevLocationName, op: 'pageload', origin: 'auto.pageload.nextjs.pages_router_instrumentation', @@ -142,7 +145,9 @@ export function pagesRouterInstrumentation( source, dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, }, - }); + } as const; + activeTransaction = startTransactionCb(transactionContext); + startPageloadSpanCallback(transactionContext); } if (startTransactionOnLocationChange) { @@ -172,13 +177,15 @@ export function pagesRouterInstrumentation( activeTransaction.end(); } - const navigationTransaction = startTransactionCb({ + const transactionContext = { name: transactionName, op: 'navigation', origin: 'auto.navigation.nextjs.pages_router_instrumentation', tags, metadata: { source: transactionSource }, - }); + } as const; + const navigationTransaction = startTransactionCb(transactionContext); + startNavigationSpanCallback(transactionContext); if (navigationTransaction) { // In addition to the navigation transaction we're also starting a span to mark Next.js's `routeChangeStart` From 7393eed47708254d8c449eafe499409192588522 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 30 Jan 2024 17:31:57 +0100 Subject: [PATCH 2/8] Hmm --- .../src/client/browserTracingIntegration.ts | 22 ++++++++++++++----- packages/nextjs/src/client/index.ts | 14 ++++++++---- packages/nextjs/test/clientSdk.test.ts | 13 +++++++++++ 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/client/browserTracingIntegration.ts b/packages/nextjs/src/client/browserTracingIntegration.ts index c0f85a17b671..07e63e03abcf 100644 --- a/packages/nextjs/src/client/browserTracingIntegration.ts +++ b/packages/nextjs/src/client/browserTracingIntegration.ts @@ -2,12 +2,16 @@ import { BrowserTracing as OriginalBrowserTracing, browserTracingIntegration as originalBrowserTracingIntegration, defaultRequestInstrumentationOptions, + startBrowserTracingNavigationSpan, + startBrowserTracingPageLoadSpan, } from '@sentry/react'; import type { Integration, StartSpanOptions } from '@sentry/types'; import { nextRouterInstrumentation } from '../index.client'; /** * A custom BrowserTracing integration for Next.js. + * + * @deprecated Use `browserTracingIntegration` instead. */ export class BrowserTracing extends OriginalBrowserTracing { public constructor(options?: ConstructorParameters[0]) { @@ -32,7 +36,7 @@ export class BrowserTracing extends OriginalBrowserTracing { } /** - * TODO + * A custom BrowserTracing integration for Next.js. */ export function browserTracingIntegration( options?: Parameters[0], @@ -50,20 +54,26 @@ export function browserTracingIntegration( if (!client.emit) { return; } - client.emit('startPageLoadSpan', startSpanOptions); - return undefined; + + startBrowserTracingPageLoadSpan(client, startSpanOptions); }; const startNavigationCallback = (startSpanOptions: StartSpanOptions): void => { if (!client.emit) { return; } - client.emit('startNavigationSpan', startSpanOptions); - return undefined; + + startBrowserTracingNavigationSpan(client, startSpanOptions); }; // eslint-disable-next-line deprecation/deprecation - nextRouterInstrumentation(() => undefined, true, true, startPageloadCallback, startNavigationCallback); + nextRouterInstrumentation( + () => undefined, + options?.instrumentPageLoad, + options?.instrumentNavigation, + startPageloadCallback, + startNavigationCallback, + ); }, }; } diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index ddea9dee3505..e0d22445a3a1 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -10,7 +10,7 @@ import type { EventProcessor, Integration } from '@sentry/types'; import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolicationEventProcessor'; import { getVercelEnv } from '../common/getVercelEnv'; -import type { browserTracingIntegration } from './browserTracingIntegration'; +import { browserTracingIntegration } from './browserTracingIntegration'; import { BrowserTracing } from './browserTracingIntegration'; import { rewriteFramesIntegration } from './rewriteFramesIntegration'; import { applyTunnelRouteOption } from './tunnelRoute'; @@ -37,6 +37,7 @@ export const Integrations = { // // import { BrowserTracing } from '@sentry/nextjs'; // const instance = new BrowserTracing(); +// eslint-disable-next-line deprecation/deprecation export { BrowserTracing, rewriteFramesIntegration }; // Treeshakable guard to remove all code related to tracing @@ -70,7 +71,7 @@ export function init(options: BrowserOptions): void { } // TODO v8: Remove this again -// We need to handle BrowserTracing passed to `integrations` that comes from `@sentry/tracing`, not `@sentry/sveltekit` :( +// We need to handle BrowserTracing passed to `integrations` that comes from `@sentry/tracing`, not `@sentry/nextjs` :( function fixBrowserTracingIntegration(options: BrowserOptions): void { const { integrations } = options; if (!integrations) { @@ -91,6 +92,7 @@ function fixBrowserTracingIntegration(options: BrowserOptions): void { function isNewBrowserTracingIntegration( integration: Integration, ): integration is Integration & { options?: Parameters[0] } { + // eslint-disable-next-line deprecation/deprecation return !!integration.afterAllSetup && !!(integration as BrowserTracing).options; } @@ -104,17 +106,21 @@ function maybeUpdateBrowserTracingIntegration(integrations: Integration[]): Inte // If `browserTracingIntegration()` was added, we need to force-convert it to our custom one if (isNewBrowserTracingIntegration(browserTracing)) { const { options } = browserTracing; + // eslint-disable-next-line deprecation/deprecation integrations[integrations.indexOf(browserTracing)] = new BrowserTracing(options); } // If BrowserTracing was added, but it is not our forked version, // replace it with our forked version with the same options + // eslint-disable-next-line deprecation/deprecation if (!(browserTracing instanceof BrowserTracing)) { + // eslint-disable-next-line deprecation/deprecation const options: ConstructorParameters[0] = (browserTracing as BrowserTracing).options; // This option is overwritten by the custom integration delete options.routingInstrumentation; // eslint-disable-next-line deprecation/deprecation delete options.tracingOrigins; + // eslint-disable-next-line deprecation/deprecation integrations[integrations.indexOf(browserTracing)] = new BrowserTracing(options); } @@ -128,7 +134,7 @@ function getDefaultIntegrations(options: BrowserOptions): Integration[] { // will get treeshaken away if (typeof __SENTRY_TRACING__ === 'undefined' || __SENTRY_TRACING__) { if (hasTracingEnabled(options)) { - customDefaultIntegrations.push(new BrowserTracing()); + customDefaultIntegrations.push(browserTracingIntegration()); } } @@ -142,6 +148,6 @@ export function withSentryConfig(exportedUserNextConfig: T): T { return exportedUserNextConfig; } -export { browserTracingIntegration }; +export { browserTracingIntegration } from './browserTracingIntegration'; export * from '../common'; diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index f4ec99c3cc71..ffe8206a095e 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -125,11 +125,13 @@ describe('Client init()', () => { }); const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); expect(browserTracingIntegration).toBeDefined(); expect(browserTracingIntegration?.options).toEqual( expect.objectContaining({ + // eslint-disable-next-line deprecation/deprecation routingInstrumentation: nextRouterInstrumentation, }), ); @@ -142,11 +144,13 @@ describe('Client init()', () => { }); const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); expect(browserTracingIntegration).toBeDefined(); expect(browserTracingIntegration?.options).toEqual( expect.objectContaining({ + // eslint-disable-next-line deprecation/deprecation routingInstrumentation: nextRouterInstrumentation, }), ); @@ -158,6 +162,7 @@ describe('Client init()', () => { }); const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); expect(browserTracingIntegration).toBeUndefined(); @@ -167,15 +172,18 @@ describe('Client init()', () => { init({ dsn: TEST_DSN, tracesSampleRate: 1.0, + // eslint-disable-next-line deprecation/deprecation integrations: [new BrowserTracing({ finalTimeout: 10 })], }); const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); expect(browserTracingIntegration).toBeDefined(); expect(browserTracingIntegration?.options).toEqual( expect.objectContaining({ + // eslint-disable-next-line deprecation/deprecation routingInstrumentation: nextRouterInstrumentation, // This proves it's still the user's copy finalTimeout: 10, @@ -191,11 +199,13 @@ describe('Client init()', () => { }); const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation const integration = client.getIntegrationByName('BrowserTracing'); expect(integration).toBeDefined(); expect(integration?.options).toEqual( expect.objectContaining({ + // eslint-disable-next-line deprecation/deprecation routingInstrumentation: nextRouterInstrumentation, // This proves it's still the user's copy finalTimeout: 10, @@ -207,16 +217,19 @@ describe('Client init()', () => { init({ dsn: TEST_DSN, tracesSampleRate: 1.0, + // eslint-disable-next-line deprecation/deprecation integrations: defaults => [...defaults, new BrowserTracing({ startTransactionOnLocationChange: false })], }); const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); expect(browserTracingIntegration).toBeDefined(); expect(browserTracingIntegration?.options).toEqual( expect.objectContaining({ + // eslint-disable-next-line deprecation/deprecation routingInstrumentation: nextRouterInstrumentation, // This proves it's still the user's copy startTransactionOnLocationChange: false, From f2315b9aa59f2a91e6aee9ec9ece948a03c78d19 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 31 Jan 2024 12:42:20 +0100 Subject: [PATCH 3/8] Fix and improve tests --- packages/nextjs/test/clientSdk.test.ts | 27 ++-------- .../appRouterInstrumentation.test.ts | 49 ++++++++++++++++--- .../pagesRouterInstrumentation.test.ts | 48 ++++++++++++++++-- 3 files changed, 91 insertions(+), 33 deletions(-) diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index ffe8206a095e..cdcf0cc0352d 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -125,16 +125,8 @@ describe('Client init()', () => { }); const client = getClient()!; - // eslint-disable-next-line deprecation/deprecation - const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); - - expect(browserTracingIntegration).toBeDefined(); - expect(browserTracingIntegration?.options).toEqual( - expect.objectContaining({ - // eslint-disable-next-line deprecation/deprecation - routingInstrumentation: nextRouterInstrumentation, - }), - ); + const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); + expect(browserTracingIntegration?.name).toBe('BrowserTracing'); }); it('adds `BrowserTracing` integration if `tracesSampler` is set', () => { @@ -144,16 +136,8 @@ describe('Client init()', () => { }); const client = getClient()!; - // eslint-disable-next-line deprecation/deprecation - const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); - - expect(browserTracingIntegration).toBeDefined(); - expect(browserTracingIntegration?.options).toEqual( - expect.objectContaining({ - // eslint-disable-next-line deprecation/deprecation - routingInstrumentation: nextRouterInstrumentation, - }), - ); + const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); + expect(browserTracingIntegration?.name).toBe('BrowserTracing'); }); it('does not add `BrowserTracing` integration if tracing not enabled in SDK', () => { @@ -162,9 +146,8 @@ describe('Client init()', () => { }); const client = getClient()!; - // eslint-disable-next-line deprecation/deprecation - const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); + const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); expect(browserTracingIntegration).toBeUndefined(); }); diff --git a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts b/packages/nextjs/test/performance/appRouterInstrumentation.test.ts index 3337b99ab9a9..34a6b31fc60f 100644 --- a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts +++ b/packages/nextjs/test/performance/appRouterInstrumentation.test.ts @@ -30,7 +30,10 @@ describe('appRouterInstrumentation', () => { it('should create a pageload transactions with the current location name', () => { setUpPage('https://example.com/some/page?someParam=foobar'); const startTransactionCallbackFn = jest.fn(); - appRouterInstrumentation(startTransactionCallbackFn, true, false); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); + + appRouterInstrumentation(startTransactionCallbackFn, true, false, mockStartPageloadSpan, mockStartNavigationSpan); expect(startTransactionCallbackFn).toHaveBeenCalledWith( expect.objectContaining({ name: '/some/page', @@ -42,12 +45,26 @@ describe('appRouterInstrumentation', () => { metadata: { source: 'url' }, }), ); + expect(mockStartPageloadSpan).toHaveBeenCalledWith( + expect.objectContaining({ + name: '/some/page', + op: 'pageload', + origin: 'auto.pageload.nextjs.app_router_instrumentation', + tags: { + 'routing.instrumentation': 'next-app-router', + }, + metadata: { source: 'url' }, + }), + ); }); it('should not create a pageload transaction when `startTransactionOnPageLoad` is false', () => { setUpPage('https://example.com/some/page?someParam=foobar'); const startTransactionCallbackFn = jest.fn(); - appRouterInstrumentation(startTransactionCallbackFn, false, false); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); + + appRouterInstrumentation(startTransactionCallbackFn, false, false, mockStartPageloadSpan, mockStartNavigationSpan); expect(startTransactionCallbackFn).not.toHaveBeenCalled(); }); @@ -60,7 +77,10 @@ describe('appRouterInstrumentation', () => { }); const startTransactionCallbackFn = jest.fn(); - appRouterInstrumentation(startTransactionCallbackFn, false, true); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); + + appRouterInstrumentation(startTransactionCallbackFn, false, true, mockStartPageloadSpan, mockStartNavigationSpan); fetchInstrumentationHandlerCallback!({ args: [ @@ -85,6 +105,16 @@ describe('appRouterInstrumentation', () => { 'routing.instrumentation': 'next-app-router', }, }); + expect(mockStartNavigationSpan).toHaveBeenCalledWith({ + name: '/some/server/component/page', + op: 'navigation', + origin: 'auto.navigation.nextjs.app_router_instrumentation', + metadata: { source: 'url' }, + tags: { + from: '/some/page', + 'routing.instrumentation': 'next-app-router', + }, + }); }); it.each([ @@ -133,7 +163,7 @@ describe('appRouterInstrumentation', () => { }, ], ])( - 'should not create naviagtion transactions for fetch requests that are not navigating RSC requests (%s)', + 'should not create navigation transactions for fetch requests that are not navigating RSC requests (%s)', (_, fetchCallbackData) => { setUpPage('https://example.com/some/page?someParam=foobar'); let fetchInstrumentationHandlerCallback: (arg: HandlerDataFetch) => void; @@ -143,9 +173,13 @@ describe('appRouterInstrumentation', () => { }); const startTransactionCallbackFn = jest.fn(); - appRouterInstrumentation(startTransactionCallbackFn, false, true); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); + + appRouterInstrumentation(startTransactionCallbackFn, false, true, mockStartPageloadSpan, mockStartNavigationSpan); fetchInstrumentationHandlerCallback!(fetchCallbackData); expect(startTransactionCallbackFn).not.toHaveBeenCalled(); + expect(mockStartNavigationSpan).not.toHaveBeenCalled(); }, ); @@ -153,9 +187,12 @@ describe('appRouterInstrumentation', () => { setUpPage('https://example.com/some/page?someParam=foobar'); const addFetchInstrumentationHandlerImpl = jest.fn(); const startTransactionCallbackFn = jest.fn(); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); addFetchInstrumentationHandlerSpy.mockImplementationOnce(addFetchInstrumentationHandlerImpl); - appRouterInstrumentation(startTransactionCallbackFn, false, false); + appRouterInstrumentation(startTransactionCallbackFn, false, false, mockStartPageloadSpan, mockStartNavigationSpan); expect(addFetchInstrumentationHandlerImpl).not.toHaveBeenCalled(); + expect(mockStartNavigationSpan).not.toHaveBeenCalled(); }); }); diff --git a/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts b/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts index 592df911bde2..3e032c1f01d1 100644 --- a/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts +++ b/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts @@ -211,20 +211,41 @@ describe('pagesRouterInstrumentation', () => { 'creates a pageload transaction (#%#)', (url, route, query, props, hasNextData, expectedStartTransactionArgument) => { const mockStartTransaction = createMockStartTransaction(); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); + setUpNextPage({ url, route, query, props, hasNextData }); - pagesRouterInstrumentation(mockStartTransaction); + pagesRouterInstrumentation( + mockStartTransaction, + undefined, + undefined, + mockStartPageloadSpan, + mockStartNavigationSpan, + ); + expect(mockStartTransaction).toHaveBeenCalledTimes(1); expect(mockStartTransaction).toHaveBeenLastCalledWith( expect.objectContaining(expectedStartTransactionArgument), ); + expect(mockStartPageloadSpan).toHaveBeenCalledWith(expect.objectContaining(expectedStartTransactionArgument)); }, ); it('does not create a pageload transaction if option not given', () => { const mockStartTransaction = createMockStartTransaction(); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); + setUpNextPage({ url: 'https://example.com/', route: '/', hasNextData: false }); - pagesRouterInstrumentation(mockStartTransaction, false); - expect(mockStartTransaction).toHaveBeenCalledTimes(0); + pagesRouterInstrumentation( + mockStartTransaction, + false, + undefined, + mockStartPageloadSpan, + mockStartNavigationSpan, + ); + expect(mockStartTransaction).not.toHaveBeenCalled(); + expect(mockStartPageloadSpan).not.toHaveBeenCalled(); }); }); @@ -252,6 +273,8 @@ describe('pagesRouterInstrumentation', () => { 'should create a parameterized transaction on route change (%s)', (targetLocation, expectedTransactionName, expectedTransactionSource) => { const mockStartTransaction = createMockStartTransaction(); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); setUpNextPage({ url: 'https://example.com/home', @@ -270,7 +293,7 @@ describe('pagesRouterInstrumentation', () => { ], }); - pagesRouterInstrumentation(mockStartTransaction, false, true); + pagesRouterInstrumentation(mockStartTransaction, false, true, mockStartPageloadSpan, mockStartNavigationSpan); Router.events.emit('routeChangeStart', targetLocation); @@ -287,6 +310,18 @@ describe('pagesRouterInstrumentation', () => { }), }), ); + expect(mockStartNavigationSpan).toHaveBeenCalledWith( + expect.objectContaining({ + name: expectedTransactionName, + op: 'navigation', + tags: expect.objectContaining({ + 'routing.instrumentation': 'next-pages-router', + }), + metadata: expect.objectContaining({ + source: expectedTransactionSource, + }), + }), + ); Router.events.emit('routeChangeComplete', targetLocation); // eslint-disable-next-line @typescript-eslint/unbound-method @@ -298,6 +333,8 @@ describe('pagesRouterInstrumentation', () => { it('should not create transaction when navigation transactions are disabled', () => { const mockStartTransaction = createMockStartTransaction(); + const mockStartPageloadSpan = jest.fn(); + const mockStartNavigationSpan = jest.fn(); setUpNextPage({ url: 'https://example.com/home', @@ -306,11 +343,12 @@ describe('pagesRouterInstrumentation', () => { navigatableRoutes: ['/home', '/posts/[id]'], }); - pagesRouterInstrumentation(mockStartTransaction, false, false); + pagesRouterInstrumentation(mockStartTransaction, false, false, mockStartPageloadSpan, mockStartNavigationSpan); Router.events.emit('routeChangeStart', '/posts/42'); expect(mockStartTransaction).not.toHaveBeenCalled(); + expect(mockStartNavigationSpan).not.toHaveBeenCalled(); }); }); }); From 10c6ca4f0dc96f241beb97861e3e94f291db49df Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 31 Jan 2024 13:02:39 +0100 Subject: [PATCH 4/8] Ci faster --- .github/workflows/build.yml | 2 +- .../e2e-tests/test-applications/nextjs-14/playwright.config.ts | 3 +++ .../test-applications/nextjs-app-dir/playwright.config.ts | 3 +++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b1f974dad511..6906cc768f71 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -931,7 +931,7 @@ jobs: github.actor != 'dependabot[bot]' needs: [job_get_metadata, job_build, job_e2e_prepare] runs-on: ubuntu-20.04 - timeout-minutes: 10 + timeout-minutes: 20 env: E2E_TEST_AUTH_TOKEN: ${{ secrets.E2E_TEST_AUTH_TOKEN }} E2E_TEST_DSN: ${{ secrets.E2E_TEST_DSN }} 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 ab3c40a21471..d855e4918ce5 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 @@ -1,3 +1,4 @@ +import os from 'os'; import type { PlaywrightTestConfig } from '@playwright/test'; import { devices } from '@playwright/test'; @@ -31,6 +32,8 @@ const config: PlaywrightTestConfig = { }, /* Run tests in files in parallel */ fullyParallel: true, + /* Defaults to half the number of CPUs. The tests are not really CPU-bound but rather I/O-bound with all the polling we do so we increase the concurrency to the CPU count. */ + workers: os.cpus().length, /* Fail the build on CI if you accidentally left test.only in the source code. */ forbidOnly: !!process.env.CI, /* `next dev` is incredibly buggy with the app dir */ diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/playwright.config.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/playwright.config.ts index ab3c40a21471..599afc629b87 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/playwright.config.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/playwright.config.ts @@ -1,3 +1,4 @@ +import os from 'os'; import type { PlaywrightTestConfig } from '@playwright/test'; import { devices } from '@playwright/test'; @@ -29,6 +30,8 @@ const config: PlaywrightTestConfig = { */ timeout: 10000, }, + /* Defaults to half the number of CPUs. The tests are not really CPU-bound but rather I/O-bound with all the polling we do so we increase the concurrency to the CPU count. */ + workers: os.cpus().length, /* Run tests in files in parallel */ fullyParallel: true, /* Fail the build on CI if you accidentally left test.only in the source code. */ From f2c57e216045e4b75d5ed18e4f361c5472c1602f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 31 Jan 2024 12:58:57 +0000 Subject: [PATCH 5/8] Tests --- packages/nextjs/test/clientSdk.test.ts | 142 ++++++++++++------------- 1 file changed, 71 insertions(+), 71 deletions(-) diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index cdcf0cc0352d..0ce7733dc137 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -117,107 +117,107 @@ describe('Client init()', () => { expect(installedBreadcrumbsIntegration).toBeDefined(); }); - describe('`BrowserTracing` integration', () => { - it('adds `BrowserTracing` integration if `tracesSampleRate` is set', () => { - init({ - dsn: TEST_DSN, - tracesSampleRate: 1.0, - }); - - const client = getClient()!; - const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); - expect(browserTracingIntegration?.name).toBe('BrowserTracing'); + it('forces correct router instrumentation if user provides `BrowserTracing` in an array', () => { + init({ + dsn: TEST_DSN, + tracesSampleRate: 1.0, + // eslint-disable-next-line deprecation/deprecation + integrations: [new BrowserTracing({ finalTimeout: 10 })], }); - it('adds `BrowserTracing` integration if `tracesSampler` is set', () => { - init({ - dsn: TEST_DSN, - tracesSampler: () => true, - }); + const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation + const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); - const client = getClient()!; - const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); - expect(browserTracingIntegration?.name).toBe('BrowserTracing'); + expect(browserTracingIntegration).toBeDefined(); + expect(browserTracingIntegration?.options).toEqual( + expect.objectContaining({ + // eslint-disable-next-line deprecation/deprecation + routingInstrumentation: nextRouterInstrumentation, + // This proves it's still the user's copy + finalTimeout: 10, + }), + ); + }); + + it('forces correct router instrumentation if user provides `browserTracingIntegration`', () => { + init({ + dsn: TEST_DSN, + integrations: [browserTracingIntegration({ finalTimeout: 10 })], + enableTracing: true, }); - it('does not add `BrowserTracing` integration if tracing not enabled in SDK', () => { - init({ - dsn: TEST_DSN, - }); + const client = getClient()!; + // eslint-disable-next-line deprecation/deprecation + const integration = client.getIntegrationByName('BrowserTracing'); - const client = getClient()!; + expect(integration).toBeDefined(); + expect(integration?.options).toEqual( + expect.objectContaining({ + // eslint-disable-next-line deprecation/deprecation + routingInstrumentation: nextRouterInstrumentation, + // This proves it's still the user's copy + finalTimeout: 10, + }), + ); + }); - const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); - expect(browserTracingIntegration).toBeUndefined(); + it('forces correct router instrumentation if user provides `BrowserTracing` in a function', () => { + init({ + dsn: TEST_DSN, + tracesSampleRate: 1.0, + // eslint-disable-next-line deprecation/deprecation + integrations: defaults => [...defaults, new BrowserTracing({ startTransactionOnLocationChange: false })], }); - it('forces correct router instrumentation if user provides `BrowserTracing` in an array', () => { + const client = getClient()!; + + // eslint-disable-next-line deprecation/deprecation + const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); + + expect(browserTracingIntegration).toBeDefined(); + expect(browserTracingIntegration?.options).toEqual( + expect.objectContaining({ + // eslint-disable-next-line deprecation/deprecation + routingInstrumentation: nextRouterInstrumentation, + // This proves it's still the user's copy + startTransactionOnLocationChange: false, + }), + ); + }); + + describe('browserTracingIntegration()', () => { + it('adds `browserTracingIntegration()` integration if `tracesSampleRate` is set', () => { init({ dsn: TEST_DSN, tracesSampleRate: 1.0, - // eslint-disable-next-line deprecation/deprecation - integrations: [new BrowserTracing({ finalTimeout: 10 })], }); const client = getClient()!; - // eslint-disable-next-line deprecation/deprecation - const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); - - expect(browserTracingIntegration).toBeDefined(); - expect(browserTracingIntegration?.options).toEqual( - expect.objectContaining({ - // eslint-disable-next-line deprecation/deprecation - routingInstrumentation: nextRouterInstrumentation, - // This proves it's still the user's copy - finalTimeout: 10, - }), - ); + const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); + expect(browserTracingIntegration?.name).toBe('BrowserTracing'); }); - it('forces correct router instrumentation if user provides `browserTracingIntegration`', () => { + it('adds `browserTracingIntegration()` integration if `tracesSampler` is set', () => { init({ dsn: TEST_DSN, - integrations: [browserTracingIntegration({ finalTimeout: 10 })], - enableTracing: true, + tracesSampler: () => true, }); const client = getClient()!; - // eslint-disable-next-line deprecation/deprecation - const integration = client.getIntegrationByName('BrowserTracing'); - - expect(integration).toBeDefined(); - expect(integration?.options).toEqual( - expect.objectContaining({ - // eslint-disable-next-line deprecation/deprecation - routingInstrumentation: nextRouterInstrumentation, - // This proves it's still the user's copy - finalTimeout: 10, - }), - ); + const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); + expect(browserTracingIntegration?.name).toBe('BrowserTracing'); }); - it('forces correct router instrumentation if user provides `BrowserTracing` in a function', () => { + it('does not add `browserTracingIntegration()` integration if tracing not enabled in SDK', () => { init({ dsn: TEST_DSN, - tracesSampleRate: 1.0, - // eslint-disable-next-line deprecation/deprecation - integrations: defaults => [...defaults, new BrowserTracing({ startTransactionOnLocationChange: false })], }); const client = getClient()!; - // eslint-disable-next-line deprecation/deprecation - const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); - - expect(browserTracingIntegration).toBeDefined(); - expect(browserTracingIntegration?.options).toEqual( - expect.objectContaining({ - // eslint-disable-next-line deprecation/deprecation - routingInstrumentation: nextRouterInstrumentation, - // This proves it's still the user's copy - startTransactionOnLocationChange: false, - }), - ); + const browserTracingIntegration = client.getIntegrationByName('BrowserTracing'); + expect(browserTracingIntegration).toBeUndefined(); }); }); }); From ea5836d2da72b44f8c0666acf639af518eba5b9f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 31 Jan 2024 13:17:43 +0000 Subject: [PATCH 6/8] Call original afterAllSetup --- .../nextjs/src/client/browserTracingIntegration.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/packages/nextjs/src/client/browserTracingIntegration.ts b/packages/nextjs/src/client/browserTracingIntegration.ts index 07e63e03abcf..700c69950507 100644 --- a/packages/nextjs/src/client/browserTracingIntegration.ts +++ b/packages/nextjs/src/client/browserTracingIntegration.ts @@ -50,19 +50,13 @@ export function browserTracingIntegration( return { ...browserTracingIntegrationInstance, afterAllSetup(client) { - const startPageloadCallback = (startSpanOptions: StartSpanOptions): void => { - if (!client.emit) { - return; - } + browserTracingIntegrationInstance.afterAllSetup(client); + const startPageloadCallback = (startSpanOptions: StartSpanOptions): void => { startBrowserTracingPageLoadSpan(client, startSpanOptions); }; const startNavigationCallback = (startSpanOptions: StartSpanOptions): void => { - if (!client.emit) { - return; - } - startBrowserTracingNavigationSpan(client, startSpanOptions); }; From 9963ef0ad0f393ca043eea61fec9995f54ff5d04 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 31 Jan 2024 15:04:24 +0000 Subject: [PATCH 7/8] big sad --- .../src/client/browserTracingIntegration.ts | 31 +++++++++++++++++-- .../appRouterRoutingInstrumentation.ts | 1 + 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/client/browserTracingIntegration.ts b/packages/nextjs/src/client/browserTracingIntegration.ts index 700c69950507..bf62725e105b 100644 --- a/packages/nextjs/src/client/browserTracingIntegration.ts +++ b/packages/nextjs/src/client/browserTracingIntegration.ts @@ -42,6 +42,18 @@ export function browserTracingIntegration( options?: Parameters[0], ): Integration { const browserTracingIntegrationInstance = originalBrowserTracingIntegration({ + // eslint-disable-next-line deprecation/deprecation + tracingOrigins: + process.env.NODE_ENV === 'development' + ? [ + // Will match any URL that contains "localhost" but not "webpack.hot-update.json" - The webpack dev-server + // has cors and it doesn't like extra headers when it's accessed from a different URL. + // TODO(v8): Ideally we rework our tracePropagationTargets logic so this hack won't be necessary anymore (see issue #9764) + /^(?=.*localhost)(?!.*webpack\.hot-update\.json).*/, + /^\/(?!\/)/, + ] + : // eslint-disable-next-line deprecation/deprecation + [...defaultRequestInstrumentationOptions.tracingOrigins, /^(api\/)/], ...options, instrumentNavigation: false, instrumentPageLoad: false, @@ -50,8 +62,6 @@ export function browserTracingIntegration( return { ...browserTracingIntegrationInstance, afterAllSetup(client) { - browserTracingIntegrationInstance.afterAllSetup(client); - const startPageloadCallback = (startSpanOptions: StartSpanOptions): void => { startBrowserTracingPageLoadSpan(client, startSpanOptions); }; @@ -60,14 +70,29 @@ export function browserTracingIntegration( startBrowserTracingNavigationSpan(client, startSpanOptions); }; + // We need to run the navigation span instrumentation before the `afterAllSetup` hook on the normal browser + // tracing integration because we need to ensure the order of execution is as follows: + // Instrumentation to start span on RSC fetch request runs -> Instrumentation to put tracing headers from active span on fetch runs + // If it were the other way around, the RSC fetch request would not receive the tracing headers from the navigation transaction. // eslint-disable-next-line deprecation/deprecation nextRouterInstrumentation( () => undefined, - options?.instrumentPageLoad, + false, options?.instrumentNavigation, startPageloadCallback, startNavigationCallback, ); + + browserTracingIntegrationInstance.afterAllSetup(client); + + // eslint-disable-next-line deprecation/deprecation + nextRouterInstrumentation( + () => undefined, + options?.instrumentPageLoad, + false, + startPageloadCallback, + startNavigationCallback, + ); }, }; } diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 78b2612b293f..25ec697a2161 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -12,6 +12,7 @@ const DEFAULT_TAGS = { /** * Instruments the Next.js Client App Router. */ +// TODO(v8): Clean this function up by splitting into pageload and navigation instrumentation respectively. Also remove startTransactionCb in the process. export function appRouterInstrumentation( startTransactionCb: StartTransactionCb, startTransactionOnPageLoad: boolean = true, From 52c4535f67f8cfbc3efa7f24687c46933192ae59 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 2 Feb 2024 14:26:20 +0100 Subject: [PATCH 8/8] timeout --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b4803a011823..74ccf23dc669 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -995,7 +995,7 @@ jobs: github.actor != 'dependabot[bot]' needs: [job_get_metadata, job_build, job_e2e_prepare] runs-on: ubuntu-20.04 - timeout-minutes: 20 + timeout-minutes: 10 env: E2E_TEST_AUTH_TOKEN: ${{ secrets.E2E_TEST_AUTH_TOKEN }} E2E_TEST_DSN: ${{ secrets.E2E_TEST_DSN }}