From 3fc99d961ef7693294faaab5057dbeef384e516e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 11 Mar 2024 13:52:35 +0000 Subject: [PATCH 1/4] ref(nextjs): Clean up browser tracing integration --- .../src/client/browserTracingIntegration.ts | 38 +- .../appRouterRoutingInstrumentation.ts | 96 ++--- .../routing/nextRoutingInstrumentation.ts | 41 +-- .../pagesRouterRoutingInstrumentation.ts | 131 ++++--- .../appRouterInstrumentation.test.ts | 75 ++-- .../pagesRouterInstrumentation.test.ts | 348 ++++++++++-------- 6 files changed, 373 insertions(+), 356 deletions(-) diff --git a/packages/nextjs/src/client/browserTracingIntegration.ts b/packages/nextjs/src/client/browserTracingIntegration.ts index d70eb3da0746..c50dbbf1686e 100644 --- a/packages/nextjs/src/client/browserTracingIntegration.ts +++ b/packages/nextjs/src/client/browserTracingIntegration.ts @@ -1,10 +1,6 @@ -import { - browserTracingIntegration as originalBrowserTracingIntegration, - startBrowserTracingNavigationSpan, - startBrowserTracingPageLoadSpan, -} from '@sentry/react'; -import type { Integration, StartSpanOptions } from '@sentry/types'; -import { nextRouterInstrumentation } from './routing/nextRoutingInstrumentation'; +import { browserTracingIntegration as originalBrowserTracingIntegration } from '@sentry/react'; +import type { Integration } from '@sentry/types'; +import { nextRouterInstrumentNavigation, nextRouterInstrumentPageLoad } from './routing/nextRoutingInstrumentation'; /** * A custom browser tracing integration for Next.js. @@ -18,36 +14,24 @@ export function browserTracingIntegration( instrumentPageLoad: false, }); + const { instrumentPageLoad = true, instrumentNavigation = true } = options; + return { ...browserTracingIntegrationInstance, afterAllSetup(client) { - const startPageloadCallback = (startSpanOptions: StartSpanOptions): void => { - startBrowserTracingPageLoadSpan(client, startSpanOptions); - }; - - const startNavigationCallback = (startSpanOptions: StartSpanOptions): void => { - 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. - nextRouterInstrumentation( - false, - options.instrumentNavigation === undefined ? true : options.instrumentNavigation, - startPageloadCallback, - startNavigationCallback, - ); + if (instrumentNavigation) { + nextRouterInstrumentNavigation(client); + } browserTracingIntegrationInstance.afterAllSetup(client); - nextRouterInstrumentation( - options.instrumentPageLoad === undefined ? true : options.instrumentPageLoad, - false, - startPageloadCallback, - startNavigationCallback, - ); + if (instrumentPageLoad) { + nextRouterInstrumentPageLoad(client); + } }, }; } diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 8459f88454a3..25c1496d25b4 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -3,71 +3,55 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, } from '@sentry/core'; -import { WINDOW } from '@sentry/react'; -import type { StartSpanOptions } from '@sentry/types'; +import { WINDOW, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan } from '@sentry/react'; +import type { Client } from '@sentry/types'; import { addFetchInstrumentationHandler, browserPerformanceTimeOrigin } from '@sentry/utils'; -type StartSpanCb = (context: StartSpanOptions) => void; - -/** - * Instruments the Next.js Client App Router. - */ -export function appRouterInstrumentation( - shouldInstrumentPageload: boolean, - shouldInstrumentNavigation: boolean, - startPageloadSpanCallback: StartSpanCb, - startNavigationSpanCallback: StartSpanCb, -): void { - // 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 currPathname = WINDOW.location.pathname; - - if (shouldInstrumentPageload) { - startPageloadSpanCallback({ - name: currPathname, - // pageload should always start at timeOrigin (and needs to be in s, not ms) - startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.nextjs.app_router_instrumentation', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - }, - }); - } +/** Instruments the Next.js app router for pageloads. */ +export function appRouterInstrumentPageLoad(client: Client): void { + startBrowserTracingPageLoadSpan(client, { + name: WINDOW.location.pathname, + // pageload should always start at timeOrigin (and needs to be in s, not ms) + startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.nextjs.app_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, + }); +} - if (shouldInstrumentNavigation) { - addFetchInstrumentationHandler(handlerData => { - // The instrumentation handler is invoked twice - once for starting a request and once when the req finishes - // We can use the existence of the end-timestamp to filter out "finishing"-events. - if (handlerData.endTimestamp !== undefined) { - return; - } +/** Instruments the Next.js app router for navigation. */ +export function appRouterInstrumentNavigation(client: Client): void { + addFetchInstrumentationHandler(handlerData => { + // The instrumentation handler is invoked twice - once for starting a request and once when the req finishes + // We can use the existence of the end-timestamp to filter out "finishing"-events. + if (handlerData.endTimestamp !== undefined) { + return; + } - // Only GET requests can be navigating RSC requests - if (handlerData.fetchData.method !== 'GET') { - return; - } + // Only GET requests can be navigating RSC requests + if (handlerData.fetchData.method !== 'GET') { + return; + } - const parsedNavigatingRscFetchArgs = parseNavigatingRscFetchArgs(handlerData.args); + const parsedNavigatingRscFetchArgs = parseNavigatingRscFetchArgs(handlerData.args); - if (parsedNavigatingRscFetchArgs === null) { - return; - } + if (parsedNavigatingRscFetchArgs === null) { + return; + } - const newPathname = parsedNavigatingRscFetchArgs.targetPathname; - currPathname = newPathname; + const newPathname = parsedNavigatingRscFetchArgs.targetPathname; - startNavigationSpanCallback({ - name: newPathname, - attributes: { - from: currPathname, - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - }, - }); + startBrowserTracingNavigationSpan(client, { + name: newPathname, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, }); - } + }); } function parseNavigatingRscFetchArgs(fetchArgs: unknown[]): null | { diff --git a/packages/nextjs/src/client/routing/nextRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/nextRoutingInstrumentation.ts index f092b2c0b3db..a25c765e0143 100644 --- a/packages/nextjs/src/client/routing/nextRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/nextRoutingInstrumentation.ts @@ -1,34 +1,29 @@ import { WINDOW } from '@sentry/react'; -import type { StartSpanOptions } from '@sentry/types'; +import type { Client } from '@sentry/types'; -import { appRouterInstrumentation } from './appRouterRoutingInstrumentation'; -import { pagesRouterInstrumentation } from './pagesRouterRoutingInstrumentation'; +import { appRouterInstrumentNavigation, appRouterInstrumentPageLoad } from './appRouterRoutingInstrumentation'; +import { pagesRouterInstrumentNavigation, pagesRouterInstrumentPageLoad } from './pagesRouterRoutingInstrumentation'; -type StartSpanCb = (context: StartSpanOptions) => void; +/** + * Instruments the Next.js Client Router for page loads. + */ +export function nextRouterInstrumentPageLoad(client: Client): void { + const isAppRouter = !WINDOW.document.getElementById('__NEXT_DATA__'); + if (isAppRouter) { + appRouterInstrumentPageLoad(client); + } else { + pagesRouterInstrumentPageLoad(client); + } +} /** - * Instruments the Next.js Client Router. + * Instruments the Next.js Client Router for navigation. */ -export function nextRouterInstrumentation( - shouldInstrumentPageload: boolean, - shouldInstrumentNavigation: boolean, - startPageloadSpanCallback: StartSpanCb, - startNavigationSpanCallback: StartSpanCb, -): void { +export function nextRouterInstrumentNavigation(client: Client): void { const isAppRouter = !WINDOW.document.getElementById('__NEXT_DATA__'); if (isAppRouter) { - appRouterInstrumentation( - shouldInstrumentPageload, - shouldInstrumentNavigation, - startPageloadSpanCallback, - startNavigationSpanCallback, - ); + appRouterInstrumentNavigation(client); } else { - pagesRouterInstrumentation( - shouldInstrumentPageload, - shouldInstrumentNavigation, - startPageloadSpanCallback, - startNavigationSpanCallback, - ); + pagesRouterInstrumentNavigation(client); } } diff --git a/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts index e13c51827c6e..c1fd0a6c1f63 100644 --- a/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts @@ -3,16 +3,13 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - getClient, + continueTrace, + getCurrentScope, + withScope, } from '@sentry/core'; -import { WINDOW } from '@sentry/react'; -import type { StartSpanOptions, TransactionSource } from '@sentry/types'; -import { - browserPerformanceTimeOrigin, - logger, - propagationContextFromHeaders, - stripUrlQueryAndFragment, -} from '@sentry/utils'; +import { WINDOW, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan } from '@sentry/react'; +import type { Client, TransactionSource } from '@sentry/types'; +import { browserPerformanceTimeOrigin, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import type { NEXT_DATA as NextData } from 'next/dist/next-server/lib/utils'; import RouterImport from 'next/router'; @@ -30,8 +27,6 @@ const globalObject = WINDOW as typeof WINDOW & { }; }; -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. */ @@ -104,73 +99,77 @@ function extractNextDataTagInformation(): NextDataTagInfo { } /** - * Instruments the Next.js pages router. Only supported for - * client side routing. Works for Next >= 10. + * Instruments the Next.js pages router for pageloads. + * Only supported for client side routing. Works for Next >= 10. * * Leverages the SingletonRouter from the `next/router` to * generate pageload/navigation transactions and parameterize * transaction names. */ -export function pagesRouterInstrumentation( - shouldInstrumentPageload: boolean, - shouldInstrumentNavigation: boolean, - startPageloadSpanCallback: StartSpanCb, - startNavigationSpanCallback: StartSpanCb, -): void { +export function pagesRouterInstrumentPageLoad(client: Client): void { const { route, params, sentryTrace, baggage } = extractNextDataTagInformation(); - const { traceId, dsc, parentSpanId, sampled } = propagationContextFromHeaders(sentryTrace, baggage); - let prevLocationName = route || globalObject.location.pathname; - - if (shouldInstrumentPageload) { - const client = getClient(); - startPageloadSpanCallback({ - name: prevLocationName, - // pageload should always start at timeOrigin (and needs to be in s, not ms) - startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined, - traceId, - parentSpanId, - parentSampled: sampled, - ...(params && client && client.getOptions().sendDefaultPii && { data: params }), - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.nextjs.pages_router_instrumentation', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: route ? 'route' : 'url', - }, - metadata: { - dynamicSamplingContext: dsc, - }, - }); - } + const name = route || globalObject.location.pathname; + + // Continue trace updates the _current_ scope, but we want to break out of it again... + // This is a bit hacky, because we want to get the span to use both the correct scope _and_ the correct propagation context + // but wards, we want to reset it to avoid this also applying to other spans + const scope = getCurrentScope(); + const propagationContextBefore = scope.getPropagationContext(); + + continueTrace({ sentryTrace, baggage }, () => { + // Ensure we are on the original current scope again, so the span is set as active on it + return withScope(scope, () => { + startBrowserTracingPageLoadSpan(client, { + name, + // pageload should always start at timeOrigin (and needs to be in s, not ms) + startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined, - if (shouldInstrumentNavigation) { - Router.events.on('routeChangeStart', (navigationTarget: string) => { - const strippedNavigationTarget = stripUrlQueryAndFragment(navigationTarget); - const matchedRoute = getNextRouteFromPathname(strippedNavigationTarget); - - let newLocation: string; - let spanSource: TransactionSource; - - if (matchedRoute) { - newLocation = matchedRoute; - spanSource = 'route'; - } else { - newLocation = strippedNavigationTarget; - spanSource = 'url'; - } - - startNavigationSpanCallback({ - name: newLocation, attributes: { - from: prevLocationName, - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.pages_router_instrumentation', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: spanSource, + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.nextjs.pages_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: route ? 'route' : 'url', + ...(params && client.getOptions().sendDefaultPii && { ...params }), }, }); + }); + }); + + scope.setPropagationContext(propagationContextBefore); +} - prevLocationName = newLocation; +/** + * Instruments the Next.js pages router for navigation. + * Only supported for client side routing. Works for Next >= 10. + * + * Leverages the SingletonRouter from the `next/router` to + * generate pageload/navigation transactions and parameterize + * transaction names. + */ +export function pagesRouterInstrumentNavigation(client: Client): void { + Router.events.on('routeChangeStart', (navigationTarget: string) => { + const strippedNavigationTarget = stripUrlQueryAndFragment(navigationTarget); + const matchedRoute = getNextRouteFromPathname(strippedNavigationTarget); + + let newLocation: string; + let spanSource: TransactionSource; + + if (matchedRoute) { + newLocation = matchedRoute; + spanSource = 'route'; + } else { + newLocation = strippedNavigationTarget; + spanSource = 'url'; + } + + startBrowserTracingNavigationSpan(client, { + name: newLocation, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.pages_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: spanSource, + }, }); - } + }); } function getNextRouteFromPathname(pathname: string): string | undefined { diff --git a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts b/packages/nextjs/test/performance/appRouterInstrumentation.test.ts index 077b68769b38..1d48b86f73ab 100644 --- a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts +++ b/packages/nextjs/test/performance/appRouterInstrumentation.test.ts @@ -1,9 +1,12 @@ import { WINDOW } from '@sentry/react'; -import type { HandlerDataFetch } from '@sentry/types'; +import type { Client, HandlerDataFetch } from '@sentry/types'; import * as sentryUtils from '@sentry/utils'; import { JSDOM } from 'jsdom'; -import { appRouterInstrumentation } from '../../src/client/routing/appRouterRoutingInstrumentation'; +import { + appRouterInstrumentNavigation, + appRouterInstrumentPageLoad, +} from '../../src/client/routing/appRouterRoutingInstrumentation'; const addFetchInstrumentationHandlerSpy = jest.spyOn(sentryUtils, 'addFetchInstrumentationHandler'); @@ -17,7 +20,7 @@ function setUpPage(url: string) { Object.defineProperty(WINDOW, 'location', { value: dom.window.document.location, writable: true }); } -describe('appRouterInstrumentation', () => { +describe('appRouterInstrumentPageLoad', () => { const originalGlobalDocument = WINDOW.document; const originalGlobalLocation = WINDOW.location; @@ -29,11 +32,17 @@ describe('appRouterInstrumentation', () => { it('should create a pageload transactions with the current location name', () => { setUpPage('https://example.com/some/page?someParam=foobar'); - const mockStartPageloadSpan = jest.fn(); - const mockStartNavigationSpan = jest.fn(); - appRouterInstrumentation(true, false, mockStartPageloadSpan, mockStartNavigationSpan); - expect(mockStartPageloadSpan).toHaveBeenCalledWith( + const emit = jest.fn(); + const client = { + emit, + } as unknown as Client; + + appRouterInstrumentPageLoad(client); + + expect(emit).toHaveBeenCalledTimes(1); + expect(emit).toHaveBeenCalledWith( + 'startPageLoadSpan', expect.objectContaining({ name: '/some/page', attributes: { @@ -44,15 +53,16 @@ describe('appRouterInstrumentation', () => { }), ); }); +}); - it('should not create a pageload transaction when `startTransactionOnPageLoad` is false', () => { - setUpPage('https://example.com/some/page?someParam=foobar'); - const startTransactionCallbackFn = jest.fn(); - const mockStartPageloadSpan = jest.fn(); - const mockStartNavigationSpan = jest.fn(); +describe('appRouterInstrumentNavigation', () => { + const originalGlobalDocument = WINDOW.document; + const originalGlobalLocation = WINDOW.location; - appRouterInstrumentation(false, false, mockStartPageloadSpan, mockStartNavigationSpan); - expect(startTransactionCallbackFn).not.toHaveBeenCalled(); + afterEach(() => { + // Clean up JSDom + Object.defineProperty(WINDOW, 'document', { value: originalGlobalDocument }); + Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); }); it('should create a navigation transactions when a navigation RSC request is sent', () => { @@ -63,10 +73,12 @@ describe('appRouterInstrumentation', () => { fetchInstrumentationHandlerCallback = callback; }); - const mockStartPageloadSpan = jest.fn(); - const mockStartNavigationSpan = jest.fn(); + const emit = jest.fn(); + const client = { + emit, + } as unknown as Client; - appRouterInstrumentation(false, true, mockStartPageloadSpan, mockStartNavigationSpan); + appRouterInstrumentNavigation(client); fetchInstrumentationHandlerCallback!({ args: [ @@ -81,10 +93,10 @@ describe('appRouterInstrumentation', () => { startTimestamp: 1337, }); - expect(mockStartNavigationSpan).toHaveBeenCalledWith({ + expect(emit).toHaveBeenCalledTimes(1); + expect(emit).toHaveBeenCalledWith('startNavigationSpan', { name: '/some/server/component/page', attributes: { - from: '/some/server/component/page', 'sentry.op': 'navigation', 'sentry.origin': 'auto.navigation.nextjs.app_router_instrumentation', 'sentry.source': 'url', @@ -147,26 +159,15 @@ describe('appRouterInstrumentation', () => { fetchInstrumentationHandlerCallback = callback; }); - const startTransactionCallbackFn = jest.fn(); - const mockStartPageloadSpan = jest.fn(); - const mockStartNavigationSpan = jest.fn(); + const emit = jest.fn(); + const client = { + emit, + } as unknown as Client; - appRouterInstrumentation(false, true, mockStartPageloadSpan, mockStartNavigationSpan); + appRouterInstrumentNavigation(client); fetchInstrumentationHandlerCallback!(fetchCallbackData); - expect(startTransactionCallbackFn).not.toHaveBeenCalled(); - expect(mockStartNavigationSpan).not.toHaveBeenCalled(); + + expect(emit).toHaveBeenCalledTimes(0); }, ); - - it('should not create navigation transactions when `startTransactionOnLocationChange` is false', () => { - setUpPage('https://example.com/some/page?someParam=foobar'); - const addFetchInstrumentationHandlerImpl = jest.fn(); - const mockStartPageloadSpan = jest.fn(); - const mockStartNavigationSpan = jest.fn(); - - addFetchInstrumentationHandlerSpy.mockImplementationOnce(addFetchInstrumentationHandlerImpl); - appRouterInstrumentation(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 76cfdec677b6..d32e2ad37f14 100644 --- a/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts +++ b/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts @@ -1,9 +1,13 @@ import { WINDOW } from '@sentry/react'; +import type { Client } from '@sentry/types'; import { JSDOM } from 'jsdom'; import type { NEXT_DATA as NextData } from 'next/dist/next-server/lib/utils'; import Router from 'next/router'; -import { pagesRouterInstrumentation } from '../../src/client/routing/pagesRouterRoutingInstrumentation'; +import { + pagesRouterInstrumentNavigation, + pagesRouterInstrumentPageLoad, +} from '../../src/client/routing/pagesRouterRoutingInstrumentation'; const globalObject = WINDOW as typeof WINDOW & { __BUILD_MANIFEST?: { @@ -44,7 +48,7 @@ jest.mock('next/router', () => { }; }); -describe('pagesRouterInstrumentation', () => { +describe('pagesRouterInstrumentPageLoad', () => { const originalGlobalDocument = WINDOW.document; const originalGlobalLocation = WINDOW.location; @@ -106,169 +110,219 @@ describe('pagesRouterInstrumentation', () => { jest.clearAllMocks(); }); - describe('pageload transactions', () => { - it.each([ - [ - 'https://example.com/lforst/posts/1337?q=42', - '/[user]/posts/[id]', - { user: 'lforst', id: '1337', q: '42' }, - { - pageProps: { - _sentryTraceData: 'c82b8554881b4d28ad977de04a4fb40a-a755953cd3394d5f-1', - _sentryBaggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', - }, + it.each([ + [ + 'https://example.com/lforst/posts/1337?q=42', + '/[user]/posts/[id]', + { user: 'lforst', id: '1337', q: '42' }, + { + pageProps: { + _sentryTraceData: 'c82b8554881b4d28ad977de04a4fb40a-a755953cd3394d5f-1', + _sentryBaggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', }, - true, - { - name: '/[user]/posts/[id]', - attributes: { - 'sentry.op': 'pageload', - 'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation', - 'sentry.source': 'route', - }, + }, + true, + { + name: '/[user]/posts/[id]', + attributes: { + 'sentry.op': 'pageload', + 'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation', + 'sentry.source': 'route', }, - ], - [ - 'https://example.com/some-page', - '/some-page', - {}, - { - pageProps: { - _sentryTraceData: 'c82b8554881b4d28ad977de04a4fb40a-a755953cd3394d5f-1', - _sentryBaggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', - }, + }, + ], + [ + 'https://example.com/some-page', + '/some-page', + {}, + { + pageProps: { + _sentryTraceData: 'c82b8554881b4d28ad977de04a4fb40a-a755953cd3394d5f-1', + _sentryBaggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', }, - true, - { - name: '/some-page', - attributes: { - 'sentry.op': 'pageload', - 'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation', - 'sentry.source': 'route', - }, + }, + true, + { + name: '/some-page', + attributes: { + 'sentry.op': 'pageload', + 'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation', + 'sentry.source': 'route', }, - ], - [ - 'https://example.com/', - '/', - {}, - {}, - true, - { - name: '/', - attributes: { - 'sentry.op': 'pageload', - 'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation', - 'sentry.source': 'route', - }, + }, + ], + [ + 'https://example.com/', + '/', + {}, + {}, + true, + { + name: '/', + attributes: { + 'sentry.op': 'pageload', + 'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation', + 'sentry.source': 'route', }, - ], - [ - 'https://example.com/lforst/posts/1337?q=42', - '/', - {}, - {}, - false, // no __NEXT_DATA__ tag - { - name: '/lforst/posts/1337', - attributes: { - 'sentry.op': 'pageload', - 'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation', - 'sentry.source': 'url', - }, + }, + ], + [ + 'https://example.com/lforst/posts/1337?q=42', + '/', + {}, + {}, + false, // no __NEXT_DATA__ tag + { + name: '/lforst/posts/1337', + attributes: { + 'sentry.op': 'pageload', + 'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation', + 'sentry.source': 'url', }, - ], - ])( - 'creates a pageload transaction (#%#)', - (url, route, query, props, hasNextData, expectedStartTransactionArgument) => { - const mockStartPageloadSpan = jest.fn(); - const mockStartNavigationSpan = jest.fn(); + }, + ], + ])( + 'creates a pageload transaction (#%#)', + (url, route, query, props, hasNextData, expectedStartTransactionArgument) => { + setUpNextPage({ url, route, query, props, hasNextData }); - setUpNextPage({ url, route, query, props, hasNextData }); - pagesRouterInstrumentation(true, true, mockStartPageloadSpan, mockStartNavigationSpan); + const emit = jest.fn(); + const client = { + emit, + getOptions: () => ({}), + } as unknown as Client; - expect(mockStartPageloadSpan).toHaveBeenCalledWith(expect.objectContaining(expectedStartTransactionArgument)); - }, - ); - }); + pagesRouterInstrumentPageLoad(client); - describe('new navigation transactions', () => { - it.each([ - ['/news', '/news', 'route'], - ['/news/', '/news', 'route'], - ['/some-route-that-is-not-defined-12332', '/some-route-that-is-not-defined-12332', 'url'], // unknown route - ['/some-route-that-is-not-defined-12332?q=42', '/some-route-that-is-not-defined-12332', 'url'], // unknown route w/ query param - ['/posts/42', '/posts/[id]', 'route'], - ['/posts/42/', '/posts/[id]', 'route'], - ['/posts/42?someParam=1', '/posts/[id]', 'route'], // query params are ignored - ['/posts/42/details', '/posts/[id]/details', 'route'], - ['/users/1337/friends/closeby/good', '/users/[id]/friends/[...filters]', 'route'], - ['/users/1337/friends', '/users/1337/friends', 'url'], - ['/statistics/page-visits', '/statistics/[[...parameters]]', 'route'], - ['/statistics', '/statistics/[[...parameters]]', 'route'], - ['/a/b/c/d', '/[a]/b/[c]/[...d]', 'route'], - ['/a/b/c/d/e', '/[a]/b/[c]/[...d]', 'route'], - ['/a/b/c', '/a/b/c', 'url'], - ['/e/f/g/h', '/e/[f]/[g]/[[...h]]', 'route'], - ['/e/f/g/h/i', '/e/[f]/[g]/[[...h]]', 'route'], - ['/e/f/g', '/e/[f]/[g]/[[...h]]', 'route'], - ])( - 'should create a parameterized transaction on route change (%s)', - (targetLocation, expectedTransactionName, expectedTransactionSource) => { - const mockStartPageloadSpan = jest.fn(); - const mockStartNavigationSpan = jest.fn(); - - setUpNextPage({ - url: 'https://example.com/home', - route: '/home', - hasNextData: true, - navigatableRoutes: [ - '/home', - '/news', - '/posts/[id]', - '/posts/[id]/details', - '/users/[id]/friends/[...filters]', - '/statistics/[[...parameters]]', - // just some complicated routes to see if we get the matching right - '/[a]/b/[c]/[...d]', - '/e/[f]/[g]/[[...h]]', - ], - }); - - pagesRouterInstrumentation(false, true, mockStartPageloadSpan, mockStartNavigationSpan); - - Router.events.emit('routeChangeStart', targetLocation); - - expect(mockStartNavigationSpan).toHaveBeenCalledWith( - expect.objectContaining({ - name: expectedTransactionName, - attributes: { - from: '/home', - 'sentry.op': 'navigation', - 'sentry.origin': 'auto.navigation.nextjs.pages_router_instrumentation', - 'sentry.source': expectedTransactionSource, - }, - }), - ); - }, + expect(emit).toHaveBeenCalledTimes(1); + expect(emit).toHaveBeenCalledWith('startPageLoadSpan', expect.objectContaining(expectedStartTransactionArgument)); + }, + ); +}); + +describe('pagesRouterInstrumentNavigation', () => { + const originalGlobalDocument = WINDOW.document; + const originalGlobalLocation = WINDOW.location; + + function setUpNextPage(pageProperties: { + url: string; + route: string; + query?: any; + props?: any; + navigatableRoutes?: string[]; + hasNextData: boolean; + }) { + const nextDataContent: NextData = { + props: pageProperties.props, + page: pageProperties.route, + query: pageProperties.query, + buildId: 'y76hvndNJBAithejdVGLW', + isFallback: false, + gssp: true, + appGip: true, + scriptLoader: [], + }; + + const dom = new JSDOM( + // Just an example of what a __NEXT_DATA__ tag might look like + pageProperties.hasNextData + ? `` + : '

No next data :(

', + { url: pageProperties.url }, ); - it('should not create transaction when navigation transactions are disabled', () => { - const mockStartPageloadSpan = jest.fn(); - const mockStartNavigationSpan = jest.fn(); + // The Next.js routing instrumentations requires a few things to be present on pageload: + // 1. Access to window.document API for `window.document.getElementById` + // 2. Access to window.location API for `window.location.pathname` + Object.defineProperty(WINDOW, 'document', { value: dom.window.document, writable: true }); + Object.defineProperty(WINDOW, 'location', { value: dom.window.document.location, writable: true }); + + // Define Next.js clientside build manifest with navigatable routes + globalObject.__BUILD_MANIFEST = { + ...globalObject.__BUILD_MANIFEST, + sortedPages: pageProperties.navigatableRoutes as string[], + }; + } + afterEach(() => { + // Clean up JSDom + Object.defineProperty(WINDOW, 'document', { value: originalGlobalDocument }); + Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); + + // Reset Next.js' __BUILD_MANIFEST + globalObject.__BUILD_MANIFEST = originalBuildManifest; + if (globalObject.__BUILD_MANIFEST) { + globalObject.__BUILD_MANIFEST.sortedPages = originalBuildManifestRoutes as string[]; + } + + // Clear all event handlers + eventHandlers = {}; + + // Necessary to clear all Router.events.off() mock call numbers + jest.clearAllMocks(); + }); + + it.each([ + ['/news', '/news', 'route'], + ['/news/', '/news', 'route'], + ['/some-route-that-is-not-defined-12332', '/some-route-that-is-not-defined-12332', 'url'], // unknown route + ['/some-route-that-is-not-defined-12332?q=42', '/some-route-that-is-not-defined-12332', 'url'], // unknown route w/ query param + ['/posts/42', '/posts/[id]', 'route'], + ['/posts/42/', '/posts/[id]', 'route'], + ['/posts/42?someParam=1', '/posts/[id]', 'route'], // query params are ignored + ['/posts/42/details', '/posts/[id]/details', 'route'], + ['/users/1337/friends/closeby/good', '/users/[id]/friends/[...filters]', 'route'], + ['/users/1337/friends', '/users/1337/friends', 'url'], + ['/statistics/page-visits', '/statistics/[[...parameters]]', 'route'], + ['/statistics', '/statistics/[[...parameters]]', 'route'], + ['/a/b/c/d', '/[a]/b/[c]/[...d]', 'route'], + ['/a/b/c/d/e', '/[a]/b/[c]/[...d]', 'route'], + ['/a/b/c', '/a/b/c', 'url'], + ['/e/f/g/h', '/e/[f]/[g]/[[...h]]', 'route'], + ['/e/f/g/h/i', '/e/[f]/[g]/[[...h]]', 'route'], + ['/e/f/g', '/e/[f]/[g]/[[...h]]', 'route'], + ])( + 'should create a parameterized transaction on route change (%s)', + (targetLocation, expectedTransactionName, expectedTransactionSource) => { setUpNextPage({ url: 'https://example.com/home', route: '/home', hasNextData: true, - navigatableRoutes: ['/home', '/posts/[id]'], + navigatableRoutes: [ + '/home', + '/news', + '/posts/[id]', + '/posts/[id]/details', + '/users/[id]/friends/[...filters]', + '/statistics/[[...parameters]]', + // just some complicated routes to see if we get the matching right + '/[a]/b/[c]/[...d]', + '/e/[f]/[g]/[[...h]]', + ], }); - pagesRouterInstrumentation(false, false, mockStartPageloadSpan, mockStartNavigationSpan); + const emit = jest.fn(); + const client = { + emit, + getOptions: () => ({}), + } as unknown as Client; - Router.events.emit('routeChangeStart', '/posts/42'); + pagesRouterInstrumentNavigation(client); - expect(mockStartNavigationSpan).not.toHaveBeenCalled(); - }); - }); + Router.events.emit('routeChangeStart', targetLocation); + + expect(emit).toHaveBeenCalledTimes(1); + expect(emit).toHaveBeenCalledWith( + 'startNavigationSpan', + expect.objectContaining({ + name: expectedTransactionName, + attributes: { + 'sentry.op': 'navigation', + 'sentry.origin': 'auto.navigation.nextjs.pages_router_instrumentation', + 'sentry.source': expectedTransactionSource, + }, + }), + ); + }, + ); }); From 5f15f962b09e296f88e6baf5d630a674fe4db198 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 11 Mar 2024 15:11:00 +0000 Subject: [PATCH 2/4] fix test --- .../test/integration/test/client/tracingNavigate.test.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/test/integration/test/client/tracingNavigate.test.ts b/packages/nextjs/test/integration/test/client/tracingNavigate.test.ts index 6a10c502ed83..434f12ecca22 100644 --- a/packages/nextjs/test/integration/test/client/tracingNavigate.test.ts +++ b/packages/nextjs/test/integration/test/client/tracingNavigate.test.ts @@ -31,9 +31,7 @@ test('should report navigation transactions', async ({ page }) => { contexts: { trace: { op: 'navigation', - data: { - from: '/[id]/withInitialProps', - }, + data: {}, }, }, }); @@ -51,9 +49,7 @@ test('should report navigation transactions', async ({ page }) => { contexts: { trace: { op: 'navigation', - data: { - from: '/[id]/withServerSideProps', - }, + data: {}, }, }, }); From 3370b86d1ab6ab3130963d9a82032baf0786e490 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 11 Mar 2024 15:50:44 +0000 Subject: [PATCH 3/4] improve span code & test?? --- .../nextjs/src/common/utils/wrapperUtils.ts | 54 +++++++++---------- .../tracingClientGetInitialProps.test.ts | 7 ++- 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/packages/nextjs/src/common/utils/wrapperUtils.ts b/packages/nextjs/src/common/utils/wrapperUtils.ts index f958089f6cad..84aa0df2a642 100644 --- a/packages/nextjs/src/common/utils/wrapperUtils.ts +++ b/packages/nextjs/src/common/utils/wrapperUtils.ts @@ -35,8 +35,8 @@ export function getSpanFromRequest(req: IncomingMessage): Span | undefined { return req._sentrySpan; } -function setSpanOnRequest(transaction: Span, req: IncomingMessage): void { - req._sentrySpan = transaction; +function setSpanOnRequest(span: Span, req: IncomingMessage): void { + req._sentrySpan = span; } /** @@ -99,25 +99,9 @@ export function withTracedServerSideDataFetcher Pr const baggage = req.headers?.baggage; return continueTrace({ sentryTrace, baggage }, () => { - let requestSpan: Span | undefined = getSpanFromRequest(req); - if (!requestSpan) { - // TODO(v8): Simplify these checks when startInactiveSpan always returns a span - requestSpan = startInactiveSpan({ - name: options.requestedRouteName, - op: 'http.server', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - }, - }); - if (requestSpan) { - requestSpan.setStatus({ code: SPAN_STATUS_OK }); - setSpanOnRequest(requestSpan, req); - autoEndSpanOnResponseEnd(requestSpan, res); - } - } + const requestSpan = getOrStartRequestSpan(req, res, options.requestedRouteName); - const withActiveSpanCallback = (): Promise> => { + return withActiveSpan(requestSpan, () => { return startSpanManual( { op: 'function.nextjs', @@ -143,18 +127,34 @@ export function withTracedServerSideDataFetcher Pr } }, ); - }; - - if (requestSpan) { - return withActiveSpan(requestSpan, withActiveSpanCallback); - } else { - return withActiveSpanCallback(); - } + }); }); }); }; } +function getOrStartRequestSpan(req: IncomingMessage, res: ServerResponse, name: string): Span { + const existingSpan = getSpanFromRequest(req); + if (existingSpan) { + return existingSpan; + } + + const requestSpan = startInactiveSpan({ + name, + op: 'http.server', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + }); + + requestSpan.setStatus({ code: SPAN_STATUS_OK }); + setSpanOnRequest(requestSpan, req); + autoEndSpanOnResponseEnd(requestSpan, res); + + return requestSpan; +} + /** * Call a data fetcher and trace it. Only traces the function if there is an active transaction on the scope. * diff --git a/packages/nextjs/test/integration/test/client/tracingClientGetInitialProps.test.ts b/packages/nextjs/test/integration/test/client/tracingClientGetInitialProps.test.ts index ddacc8bbca91..3f6cad97fe00 100644 --- a/packages/nextjs/test/integration/test/client/tracingClientGetInitialProps.test.ts +++ b/packages/nextjs/test/integration/test/client/tracingClientGetInitialProps.test.ts @@ -1,9 +1,9 @@ import { expect, test } from '@playwright/test'; -import { Transaction } from '@sentry/types'; +import { TransactionEvent } from '@sentry/types'; import { countEnvelopes, getMultipleSentryEnvelopeRequests } from './utils/helpers'; test('should instrument `getInitialProps` for performance tracing', async ({ page }) => { - const transaction = await getMultipleSentryEnvelopeRequests(page, 1, { + const transaction = await getMultipleSentryEnvelopeRequests(page, 1, { url: '/42/withInitialProps', envelopeType: 'transaction', }); @@ -19,8 +19,7 @@ test('should instrument `getInitialProps` for performance tracing', async ({ pag const nextDataTag = await page.waitForSelector('#__NEXT_DATA__', { state: 'attached' }); const nextDataTagValue = JSON.parse(await nextDataTag.evaluate(tag => (tag as HTMLElement).innerText)); - // @ts-expect-error - We know `contexts` is defined in the Transaction envelope - const traceId = transaction[0].contexts.trace.trace_id; + const traceId = transaction[0].contexts?.trace?.trace_id; expect(traceId).toBeDefined(); From 644da33b2050ca8f224a6d0b196f743a343b56fd Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 12 Mar 2024 13:28:11 +0000 Subject: [PATCH 4/4] fix continue trace --- packages/core/src/baseclient.ts | 14 +++++- .../pagesRouterRoutingInstrumentation.ts | 43 ++++++----------- .../appRouterInstrumentation.test.ts | 1 + .../pagesRouterInstrumentation.test.ts | 12 ++++- .../src/browser/browserTracingIntegration.ts | 21 +++++--- .../browser/browserTracingIntegration.test.ts | 48 +++++++++++++++++++ packages/types/src/client.ts | 14 +++++- 7 files changed, 113 insertions(+), 40 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index c27ecfa5c341..178a37aff5c0 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -450,7 +450,13 @@ export abstract class BaseClient implements Client { ): void; /** @inheritdoc */ - public on(hook: 'startPageLoadSpan', callback: (options: StartSpanOptions) => void): void; + public on( + hook: 'startPageLoadSpan', + callback: ( + options: StartSpanOptions, + traceOptions?: { sentryTrace?: string | undefined; baggage?: string | undefined }, + ) => void, + ): void; /** @inheritdoc */ public on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): void; @@ -500,7 +506,11 @@ export abstract class BaseClient implements Client { public emit(hook: 'beforeSendFeedback', feedback: FeedbackEvent, options?: { includeReplay: boolean }): void; /** @inheritdoc */ - public emit(hook: 'startPageLoadSpan', options: StartSpanOptions): void; + public emit( + hook: 'startPageLoadSpan', + options: StartSpanOptions, + traceOptions?: { sentryTrace?: string | undefined; baggage?: string | undefined }, + ): void; /** @inheritdoc */ public emit(hook: 'startNavigationSpan', options: StartSpanOptions): void; diff --git a/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts index c1fd0a6c1f63..720921ddb1e9 100644 --- a/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts @@ -3,9 +3,6 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - continueTrace, - getCurrentScope, - withScope, } from '@sentry/core'; import { WINDOW, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan } from '@sentry/react'; import type { Client, TransactionSource } from '@sentry/types'; @@ -110,31 +107,21 @@ export function pagesRouterInstrumentPageLoad(client: Client): void { const { route, params, sentryTrace, baggage } = extractNextDataTagInformation(); const name = route || globalObject.location.pathname; - // Continue trace updates the _current_ scope, but we want to break out of it again... - // This is a bit hacky, because we want to get the span to use both the correct scope _and_ the correct propagation context - // but wards, we want to reset it to avoid this also applying to other spans - const scope = getCurrentScope(); - const propagationContextBefore = scope.getPropagationContext(); - - continueTrace({ sentryTrace, baggage }, () => { - // Ensure we are on the original current scope again, so the span is set as active on it - return withScope(scope, () => { - startBrowserTracingPageLoadSpan(client, { - name, - // pageload should always start at timeOrigin (and needs to be in s, not ms) - startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined, - - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.nextjs.pages_router_instrumentation', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: route ? 'route' : 'url', - ...(params && client.getOptions().sendDefaultPii && { ...params }), - }, - }); - }); - }); - - scope.setPropagationContext(propagationContextBefore); + startBrowserTracingPageLoadSpan( + client, + { + name, + // pageload should always start at timeOrigin (and needs to be in s, not ms) + startTime: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.nextjs.pages_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: route ? 'route' : 'url', + ...(params && client.getOptions().sendDefaultPii && { ...params }), + }, + }, + { sentryTrace, baggage }, + ); } /** diff --git a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts b/packages/nextjs/test/performance/appRouterInstrumentation.test.ts index 1d48b86f73ab..16992a498f83 100644 --- a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts +++ b/packages/nextjs/test/performance/appRouterInstrumentation.test.ts @@ -51,6 +51,7 @@ describe('appRouterInstrumentPageLoad', () => { 'sentry.source': 'url', }, }), + undefined, ); }); }); diff --git a/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts b/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts index d32e2ad37f14..7ca1f01ff8d6 100644 --- a/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts +++ b/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts @@ -194,8 +194,18 @@ describe('pagesRouterInstrumentPageLoad', () => { pagesRouterInstrumentPageLoad(client); + const sentryTrace = (props as any).pageProps?._sentryTraceData; + const baggage = (props as any).pageProps?._sentryBaggage; + expect(emit).toHaveBeenCalledTimes(1); - expect(emit).toHaveBeenCalledWith('startPageLoadSpan', expect.objectContaining(expectedStartTransactionArgument)); + expect(emit).toHaveBeenCalledWith( + 'startPageLoadSpan', + expect.objectContaining(expectedStartTransactionArgument), + { + sentryTrace, + baggage, + }, + ); }, ); }); diff --git a/packages/tracing-internal/src/browser/browserTracingIntegration.ts b/packages/tracing-internal/src/browser/browserTracingIntegration.ts index 16ffe8467f34..e6d2c5e0720e 100644 --- a/packages/tracing-internal/src/browser/browserTracingIntegration.ts +++ b/packages/tracing-internal/src/browser/browserTracingIntegration.ts @@ -245,7 +245,7 @@ export const browserTracingIntegration = ((_options: Partial { + client.on('startNavigationSpan', startSpanOptions => { if (getClient() !== client) { return; } @@ -261,7 +261,7 @@ export const browserTracingIntegration = ((_options: Partial { + client.on('startPageLoadSpan', (startSpanOptions, traceOptions) => { if (getClient() !== client) { return; } @@ -272,10 +272,10 @@ export const browserTracingIntegration = ((_options: Partial tags. */ -export function startBrowserTracingPageLoadSpan(client: Client, spanOptions: StartSpanOptions): Span | undefined { - client.emit('startPageLoadSpan', spanOptions); +export function startBrowserTracingPageLoadSpan( + client: Client, + spanOptions: StartSpanOptions, + traceOptions?: { sentryTrace?: string | undefined; baggage?: string | undefined }, +): Span | undefined { + client.emit('startPageLoadSpan', spanOptions, traceOptions); const span = getActiveSpan(); const op = span && spanToJSON(span).op; diff --git a/packages/tracing-internal/test/browser/browserTracingIntegration.test.ts b/packages/tracing-internal/test/browser/browserTracingIntegration.test.ts index 9df7e23d797b..c304d323a8da 100644 --- a/packages/tracing-internal/test/browser/browserTracingIntegration.test.ts +++ b/packages/tracing-internal/test/browser/browserTracingIntegration.test.ts @@ -708,6 +708,54 @@ describe('browserTracingIntegration', () => { expect(propagationContext.traceId).not.toEqual('12312012123120121231201212312012'); expect(propagationContext.parentSpanId).not.toEqual('1121201211212012'); }); + + it('uses passed in tracing data for pageload span over meta tags', () => { + // make sampled false here, so we can see that it's being used rather than the tracesSampleRate-dictated one + document.head.innerHTML = + '' + + ''; + + const client = new TestClient( + getDefaultClientOptions({ + tracesSampleRate: 1, + integrations: [browserTracingIntegration({ instrumentPageLoad: false })], + }), + ); + setCurrentClient(client); + + client.init(); + + // manually create a pageload span with tracing data + startBrowserTracingPageLoadSpan( + client, + { + name: 'test span', + }, + { + sentryTrace: '12312012123120121231201212312011-1121201211212011-1', + baggage: 'sentry-release=2.2.14,foo=bar', + }, + ); + + const idleSpan = getActiveSpan()!; + expect(idleSpan).toBeDefined(); + + const dynamicSamplingContext = getDynamicSamplingContextFromSpan(idleSpan!); + const propagationContext = getCurrentScope().getPropagationContext(); + + // Span is correct + expect(spanToJSON(idleSpan).op).toBe('pageload'); + expect(spanToJSON(idleSpan).trace_id).toEqual('12312012123120121231201212312011'); + expect(spanToJSON(idleSpan).parent_span_id).toEqual('1121201211212011'); + expect(spanIsSampled(idleSpan)).toBe(true); + + expect(dynamicSamplingContext).toBeDefined(); + expect(dynamicSamplingContext).toStrictEqual({ release: '2.2.14' }); + + // Propagation context is reset and does not contain the meta tag data + expect(propagationContext.traceId).not.toEqual('12312012123120121231201212312012'); + expect(propagationContext.parentSpanId).not.toEqual('1121201211212012'); + }); }); describe('idleTimeout', () => { diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index dad1911c72a7..a764023a9b0e 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -248,7 +248,13 @@ export interface Client { /** * A hook for the browser tracing integrations to trigger a span start for a page load. */ - on(hook: 'startPageLoadSpan', callback: (options: StartSpanOptions) => void): void; + on( + hook: 'startPageLoadSpan', + callback: ( + options: StartSpanOptions, + traceOptions?: { sentryTrace?: string | undefined; baggage?: string | undefined }, + ) => void, + ): void; /** * A hook for browser tracing integrations to trigger a span for a navigation. @@ -321,7 +327,11 @@ export interface Client { /** * Emit a hook event for browser tracing integrations to trigger a span start for a page load. */ - emit(hook: 'startPageLoadSpan', options: StartSpanOptions): void; + emit( + hook: 'startPageLoadSpan', + options: StartSpanOptions, + traceOptions?: { sentryTrace?: string | undefined; baggage?: string | undefined }, + ): void; /** * Emit a hook event for browser tracing integrations to trigger a span for a navigation.