From f99d89b145fad50a81b101baccff4154de3f8c13 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Aug 2022 09:44:24 +0000 Subject: [PATCH 01/12] feat(nextjs): Improve pageload transaction creation --- packages/nextjs/src/performance/client.ts | 99 ++++++++--- .../nextjs/test/performance/client.test.ts | 156 ++++++++++++++++-- 2 files changed, 222 insertions(+), 33 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index 1b89d4255dd6..14e934f313a5 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -1,13 +1,71 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import { Primitive, Transaction, TransactionContext } from '@sentry/types'; -import { fill, getGlobalObject, stripUrlQueryAndFragment } from '@sentry/utils'; +import { fill, getGlobalObject, logger, parseBaggageHeader, stripUrlQueryAndFragment } from '@sentry/utils'; +import type { NEXT_DATA as NextData } from 'next/dist/next-server/lib/utils'; import { default as Router } from 'next/router'; +import type { ParsedUrlQuery } from 'querystring'; const global = getGlobalObject(); type StartTransactionCb = (context: TransactionContext) => Transaction | undefined; +/** + * Describes data located in the __NEXT_DATA__ script tag. This tag is present on every page of a Next.js app. + */ +interface SentryEnhancedNextData extends NextData { + // contains props returned by `getInitialProps` - except for `pageProps`, these are the props that got returned by `getServerSideProps` or `getStaticProps` + props: { + _sentryGetInitialPropsTraceId?: string; // trace id, if injected by server-side `getInitialProps` + _sentryGetInitialPropsBaggage?: string; // baggage, if injected by server-side `getInitialProps` + pageProps?: { + _sentryGetServerSidePropsTraceId?: string; // trace id, if injected by server-side `getServerSideProps` + _sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps` + }; + }; +} + +// Author's note: It's really not that complicated. +// eslint-disable-next-line complexity +function extractNextDataTagInformation(): { + route: string; + source: 'route' | 'url'; + traceId: string | undefined; + baggage: string | undefined; + params: ParsedUrlQuery | undefined; +} { + let nextData: SentryEnhancedNextData | undefined; + + const nextDataTag = global.document.getElementById('__NEXT_DATA__'); + if (nextDataTag && nextDataTag.innerHTML) { + try { + nextData = JSON.parse(nextDataTag.innerHTML); + } catch (e) { + __DEBUG_BUILD__ && logger.warn('Could not extract __NEXT_DATA__'); + } + } + + // `nextData.page` always contains the parameterized route + const route = (nextData || {}).page || global.document.location.pathname; + const source = nextData ? 'route' : 'url'; + + const getServerSidePropsTraceId = (((nextData || {}).props || {}).pageProps || {})._sentryGetServerSidePropsTraceId; + const getInitialPropsTraceId = ((nextData || {}).props || {})._sentryGetInitialPropsTraceId; + const getServerSidePropsBaggage = (((nextData || {}).props || {}).pageProps || {})._sentryGetServerSidePropsBaggage; + const getInitialPropsBaggage = ((nextData || {}).props || {})._sentryGetInitialPropsBaggage; + + const params = (nextData || {}).query; + + return { + route, + source, + params, + // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. + traceId: getInitialPropsTraceId || getServerSidePropsTraceId, + baggage: getInitialPropsBaggage || getServerSidePropsBaggage, + }; +} + const DEFAULT_TAGS = { 'routing.instrumentation': 'next-router', } as const; @@ -30,24 +88,26 @@ export function nextRouterInstrumentation( startTransactionOnLocationChange: boolean = true, ): void { startTransaction = startTransactionCb; - Router.ready(() => { - // We can only start the pageload transaction when we have access to the parameterized - // route name. Setting the transaction name after the transaction is started could lead - // to possible race conditions with the router, so this approach was taken. - if (startTransactionOnPageLoad) { - const pathIsRoute = Router.route !== null; - - prevTransactionName = pathIsRoute ? stripUrlQueryAndFragment(Router.route) : global.location.pathname; - activeTransaction = startTransactionCb({ - name: prevTransactionName, - op: 'pageload', - tags: DEFAULT_TAGS, - metadata: { - source: pathIsRoute ? 'route' : 'url', - }, - }); - } + if (startTransactionOnPageLoad) { + const { route, source, traceId, baggage, params } = extractNextDataTagInformation(); + + prevTransactionName = route; + + activeTransaction = startTransactionCb({ + name: prevTransactionName, + traceId, + op: 'pageload', + tags: DEFAULT_TAGS, + ...(params && { data: params }), + metadata: { + source, + ...(baggage && { baggage: parseBaggageHeader(baggage) }), + }, + }); + } + + Router.ready(() => { // Spans that aren't attached to any transaction are lost; so if transactions aren't // created (besides potentially the onpageload transaction), no need to wrap the router. if (!startTransactionOnLocationChange) return; @@ -78,7 +138,7 @@ type WrappedRouterChangeState = RouterChangeState; * Start a navigation transaction every time the router changes state. */ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): WrappedRouterChangeState { - const wrapper = function ( + return function wrapper( this: any, method: string, // The parameterized url, ex. posts/[id]/[comment] @@ -115,5 +175,4 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap } return originalChangeStateWrapper.call(this, method, url, as, options, ...args); }; - return wrapper; } diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index bf8f118cd8c4..61e2abd3f076 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -1,3 +1,5 @@ +import { getGlobalObject } from '@sentry/utils'; +import { JSDOM } from 'jsdom'; import { default as Router } from 'next/router'; import { nextRouterInstrumentation } from '../../src/performance/client'; @@ -28,28 +30,156 @@ describe('client', () => { }); describe('nextRouterInstrumentation', () => { + const originalGlobalDocument = getGlobalObject().document; + + function setUpNextPage(pageProperties: { + url: string; + route: string; + query: any; + props: any; + hasNextData: boolean; + }) { + const dom = new JSDOM( + // Just some example what a __NEXT_DATA__ tag might look like + pageProperties.hasNextData + ? ` + + ` + : '

No next data :(

', + { url: pageProperties.url }, + ); + + Object.defineProperty(global, 'document', { value: dom.window.document, writable: true, configurable: true }); + } + + afterEach(() => { + // Clean up JSDom + Object.defineProperty(global, 'document', { + value: originalGlobalDocument, + writable: true, + configurable: true, + }); + }); + it('waits for Router.ready()', () => { + setUpNextPage({ url: 'https://example.com/', route: '/', query: {}, props: {}, hasNextData: false }); const mockStartTransaction = jest.fn(); expect(readyCalled).toBe(false); nextRouterInstrumentation(mockStartTransaction); expect(readyCalled).toBe(true); }); - it('creates a pageload transaction', () => { - const mockStartTransaction = jest.fn(); - nextRouterInstrumentation(mockStartTransaction); - expect(mockStartTransaction).toHaveBeenCalledTimes(1); - expect(mockStartTransaction).toHaveBeenLastCalledWith({ - name: '/[user]/posts/[id]', - op: 'pageload', - tags: { - 'routing.instrumentation': 'next-router', + it.each([ + [ + 'https://example.com/lforst/posts/1337?q=42', + '/[user]/posts/[id]', + { user: 'lforst', id: '1337', q: '42' }, + { + _sentryGetInitialPropsTraceId: 'SOME_TRACE_ID', + _sentryGetInitialPropsBaggage: + 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', }, - metadata: { - source: 'route', + true, + { + name: '/[user]/posts/[id]', + op: 'pageload', + tags: { + 'routing.instrumentation': 'next-router', + }, + data: { + user: 'lforst', + id: '1337', + q: '42', + }, + metadata: { + source: 'route', + baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], + }, + traceId: 'SOME_TRACE_ID', }, - }); - }); + ], + [ + 'https://example.com/static', + '/static', + {}, + { + pageProps: { + _sentryGetServerSidePropsTraceId: 'SOME_TRACE_ID', + _sentryGetServerSidePropsBaggage: + 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', + }, + }, + true, + { + name: '/static', + op: 'pageload', + tags: { + 'routing.instrumentation': 'next-router', + }, + data: {}, + metadata: { + source: 'route', + baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], + }, + traceId: 'SOME_TRACE_ID', + }, + ], + [ + 'https://example.com/', + '/', + {}, + {}, + true, + { + name: '/', + op: 'pageload', + tags: { + 'routing.instrumentation': 'next-router', + }, + data: {}, + metadata: { + source: 'route', + }, + }, + ], + [ + 'https://example.com/lforst/posts/1337?q=42', + '/', + {}, + {}, + false, // no __NEXT_DATA__ tag + { + name: '/lforst/posts/1337', + op: 'pageload', + tags: { + 'routing.instrumentation': 'next-router', + }, + metadata: { + source: 'url', + }, + }, + ], + ])( + 'creates a pageload transaction (#%#)', + (url, route, query, props, hasNextData, expectedStartTransactionCall) => { + const mockStartTransaction = jest.fn(); + setUpNextPage({ url, route, query, props, hasNextData }); + nextRouterInstrumentation(mockStartTransaction); + expect(mockStartTransaction).toHaveBeenCalledTimes(1); + expect(mockStartTransaction).toHaveBeenLastCalledWith(expectedStartTransactionCall); + }, + ); it('does not create a pageload transaction if option not given', () => { const mockStartTransaction = jest.fn(); From 936b42ecacf58f344b3971cef7c09d67e845e9e0 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Aug 2022 09:53:11 +0000 Subject: [PATCH 02/12] Add explanation --- packages/nextjs/src/performance/client.ts | 27 +++++++++++++++-------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index 14e934f313a5..ae6fd99b00ab 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -25,17 +25,27 @@ interface SentryEnhancedNextData extends NextData { }; } -// Author's note: It's really not that complicated. -// eslint-disable-next-line complexity +/** + * Every Next.js page (static and dynamic ones) comes with a script tag with the id "__NEXT_DATA__". This script tag + * contains a JSON object with data that was either generated at build time for static pages (`getStaticProps`), or at + * runtime with data fetchers like `getServerSideProps.`. + * + * We can use this information to: + * - Always get the parameterized route we're in when loading a page. + * - Send trace information (trace-id, baggage) from the server to the client. + * + * This function extracts this information. + */ function extractNextDataTagInformation(): { - route: string; - source: 'route' | 'url'; + route: string | undefined; traceId: string | undefined; baggage: string | undefined; params: ParsedUrlQuery | undefined; } { let nextData: SentryEnhancedNextData | undefined; + // Let's be on the safe side and actually check first if there is really a __NEXT_DATA__ script tag on the page. + // Theoretically this should always be the case though. const nextDataTag = global.document.getElementById('__NEXT_DATA__'); if (nextDataTag && nextDataTag.innerHTML) { try { @@ -46,8 +56,7 @@ function extractNextDataTagInformation(): { } // `nextData.page` always contains the parameterized route - const route = (nextData || {}).page || global.document.location.pathname; - const source = nextData ? 'route' : 'url'; + const route = (nextData || {}).page; const getServerSidePropsTraceId = (((nextData || {}).props || {}).pageProps || {})._sentryGetServerSidePropsTraceId; const getInitialPropsTraceId = ((nextData || {}).props || {})._sentryGetInitialPropsTraceId; @@ -58,7 +67,6 @@ function extractNextDataTagInformation(): { return { route, - source, params, // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. traceId: getInitialPropsTraceId || getServerSidePropsTraceId, @@ -90,9 +98,10 @@ export function nextRouterInstrumentation( startTransaction = startTransactionCb; if (startTransactionOnPageLoad) { - const { route, source, traceId, baggage, params } = extractNextDataTagInformation(); + const { route, traceId, baggage, params } = extractNextDataTagInformation(); - prevTransactionName = route; + prevTransactionName = route || global.document.location.pathname; + const source = route ? 'route' : 'url'; activeTransaction = startTransactionCb({ name: prevTransactionName, From 227cd4d80e1dc5f8b834d393a47ec4f8caeaa2f8 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Aug 2022 10:16:13 +0000 Subject: [PATCH 03/12] Traceparent data instead of traceId only --- packages/nextjs/src/performance/client.ts | 39 ++++++++++++------- .../nextjs/test/performance/client.test.ts | 12 ++++-- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index ae6fd99b00ab..1829a31ae970 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -1,7 +1,14 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { Primitive, Transaction, TransactionContext } from '@sentry/types'; -import { fill, getGlobalObject, logger, parseBaggageHeader, stripUrlQueryAndFragment } from '@sentry/utils'; +import { Primitive, TraceparentData, Transaction, TransactionContext } from '@sentry/types'; +import { + extractTraceparentData, + fill, + getGlobalObject, + logger, + parseBaggageHeader, + stripUrlQueryAndFragment, +} from '@sentry/utils'; import type { NEXT_DATA as NextData } from 'next/dist/next-server/lib/utils'; import { default as Router } from 'next/router'; import type { ParsedUrlQuery } from 'querystring'; @@ -16,10 +23,10 @@ type StartTransactionCb = (context: TransactionContext) => Transaction | undefin interface SentryEnhancedNextData extends NextData { // contains props returned by `getInitialProps` - except for `pageProps`, these are the props that got returned by `getServerSideProps` or `getStaticProps` props: { - _sentryGetInitialPropsTraceId?: string; // trace id, if injected by server-side `getInitialProps` + _sentryGetInitialPropsTraceData?: string; // trace id, if injected by server-side `getInitialProps` _sentryGetInitialPropsBaggage?: string; // baggage, if injected by server-side `getInitialProps` pageProps?: { - _sentryGetServerSidePropsTraceId?: string; // trace id, if injected by server-side `getServerSideProps` + _sentryGetServerSidePropsTraceData?: string; // trace id, if injected by server-side `getServerSideProps` _sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps` }; }; @@ -38,7 +45,7 @@ interface SentryEnhancedNextData extends NextData { */ function extractNextDataTagInformation(): { route: string | undefined; - traceId: string | undefined; + traceParentData: TraceparentData | undefined; baggage: string | undefined; params: ParsedUrlQuery | undefined; } { @@ -57,20 +64,24 @@ function extractNextDataTagInformation(): { // `nextData.page` always contains the parameterized route const route = (nextData || {}).page; + const params = (nextData || {}).query; - const getServerSidePropsTraceId = (((nextData || {}).props || {}).pageProps || {})._sentryGetServerSidePropsTraceId; - const getInitialPropsTraceId = ((nextData || {}).props || {})._sentryGetInitialPropsTraceId; - const getServerSidePropsBaggage = (((nextData || {}).props || {}).pageProps || {})._sentryGetServerSidePropsBaggage; const getInitialPropsBaggage = ((nextData || {}).props || {})._sentryGetInitialPropsBaggage; + const getServerSidePropsBaggage = (((nextData || {}).props || {}).pageProps || {})._sentryGetServerSidePropsBaggage; - const params = (nextData || {}).query; + const getInitialPropsTraceData = ((nextData || {}).props || {})._sentryGetInitialPropsTraceData; + const getServerSidePropsTraceData = (((nextData || {}).props || {}).pageProps || {}) + ._sentryGetServerSidePropsTraceData; + + // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. + const baggage = getInitialPropsBaggage || getServerSidePropsBaggage; + const traceData = getInitialPropsTraceData || getServerSidePropsTraceData; return { route, params, - // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. - traceId: getInitialPropsTraceId || getServerSidePropsTraceId, - baggage: getInitialPropsBaggage || getServerSidePropsBaggage, + traceParentData: traceData ? extractTraceparentData(traceData) : undefined, + baggage, }; } @@ -98,17 +109,17 @@ export function nextRouterInstrumentation( startTransaction = startTransactionCb; if (startTransactionOnPageLoad) { - const { route, traceId, baggage, params } = extractNextDataTagInformation(); + const { route, traceParentData, baggage, params } = extractNextDataTagInformation(); prevTransactionName = route || global.document.location.pathname; const source = route ? 'route' : 'url'; activeTransaction = startTransactionCb({ name: prevTransactionName, - traceId, op: 'pageload', tags: DEFAULT_TAGS, ...(params && { data: params }), + ...traceParentData, metadata: { source, ...(baggage && { baggage: parseBaggageHeader(baggage) }), diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index 61e2abd3f076..b8ac7481abdc 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -86,7 +86,7 @@ describe('client', () => { '/[user]/posts/[id]', { user: 'lforst', id: '1337', q: '42' }, { - _sentryGetInitialPropsTraceId: 'SOME_TRACE_ID', + _sentryGetInitialPropsTraceData: 'c82b8554881b4d28ad977de04a4fb40a-a755953cd3394d5f-1', _sentryGetInitialPropsBaggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', }, @@ -106,7 +106,9 @@ describe('client', () => { source: 'route', baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], }, - traceId: 'SOME_TRACE_ID', + traceId: 'c82b8554881b4d28ad977de04a4fb40a', + parentSpanId: 'a755953cd3394d5f', + parentSampled: true, }, ], [ @@ -115,7 +117,7 @@ describe('client', () => { {}, { pageProps: { - _sentryGetServerSidePropsTraceId: 'SOME_TRACE_ID', + _sentryGetServerSidePropsTraceData: 'c82b8554881b4d28ad977de04a4fb40a-a755953cd3394d5f-1', _sentryGetServerSidePropsBaggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', }, @@ -132,7 +134,9 @@ describe('client', () => { source: 'route', baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], }, - traceId: 'SOME_TRACE_ID', + traceId: 'c82b8554881b4d28ad977de04a4fb40a', + parentSpanId: 'a755953cd3394d5f', + parentSampled: true, }, ], [ From 56179ed25af13bdb611c993a1ff971b208632bd5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Aug 2022 10:21:50 +0000 Subject: [PATCH 04/12] Fix tests by initializing JSDom in the right places --- packages/nextjs/test/index.client.test.ts | 13 +++++++++++++ packages/nextjs/test/performance/client.test.ts | 8 ++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/test/index.client.test.ts b/packages/nextjs/test/index.client.test.ts index a143591ad8f0..25f7d27316a6 100644 --- a/packages/nextjs/test/index.client.test.ts +++ b/packages/nextjs/test/index.client.test.ts @@ -4,6 +4,7 @@ import * as SentryReact from '@sentry/react'; import { Integrations as TracingIntegrations } from '@sentry/tracing'; import { Integration } from '@sentry/types'; import { getGlobalObject, logger, SentryError } from '@sentry/utils'; +import { JSDOM } from 'jsdom'; import { init, Integrations, nextRouterInstrumentation } from '../src/index.client'; import { NextjsOptions } from '../src/utils/nextjsOptions'; @@ -16,6 +17,18 @@ const reactInit = jest.spyOn(SentryReact, 'init'); const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent'); const logWarn = jest.spyOn(logger, 'warn'); +// Set up JSDom - needed for page load instrumentation +const dom = new JSDOM(undefined, { url: 'https://example.com/' }); +Object.defineProperty(global, 'document', { value: dom.window.document, writable: true }); + +const originalGlobalDocument = getGlobalObject().document; +afterAll(() => { + // Clean up JSDom + Object.defineProperty(global, 'document', { + value: originalGlobalDocument, + }); +}); + describe('Client init()', () => { afterEach(() => { jest.clearAllMocks(); diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index b8ac7481abdc..78b078dc5d9e 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -60,16 +60,12 @@ describe('client', () => { { url: pageProperties.url }, ); - Object.defineProperty(global, 'document', { value: dom.window.document, writable: true, configurable: true }); + Object.defineProperty(global, 'document', { value: dom.window.document, writable: true }); } afterEach(() => { // Clean up JSDom - Object.defineProperty(global, 'document', { - value: originalGlobalDocument, - writable: true, - configurable: true, - }); + Object.defineProperty(global, 'document', { value: originalGlobalDocument }); }); it('waits for Router.ready()', () => { From 22a0f83f46f2d9502701d7dd0665ac236e457952 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Aug 2022 15:44:01 +0000 Subject: [PATCH 05/12] Clarify comment about trace parent info --- packages/nextjs/src/performance/client.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index 1829a31ae970..02bd299490bc 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -23,10 +23,10 @@ type StartTransactionCb = (context: TransactionContext) => Transaction | undefin interface SentryEnhancedNextData extends NextData { // contains props returned by `getInitialProps` - except for `pageProps`, these are the props that got returned by `getServerSideProps` or `getStaticProps` props: { - _sentryGetInitialPropsTraceData?: string; // trace id, if injected by server-side `getInitialProps` + _sentryGetInitialPropsTraceData?: string; // trace parent info, if injected by server-side `getInitialProps` _sentryGetInitialPropsBaggage?: string; // baggage, if injected by server-side `getInitialProps` pageProps?: { - _sentryGetServerSidePropsTraceData?: string; // trace id, if injected by server-side `getServerSideProps` + _sentryGetServerSidePropsTraceData?: string; // trace parent info, if injected by server-side `getServerSideProps` _sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps` }; }; From 651e8e0652639c2589cbd4ff57bddb7450dc675c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Aug 2022 15:45:05 +0000 Subject: [PATCH 06/12] Undo change to `global.location` --- packages/nextjs/src/performance/client.ts | 2 +- packages/nextjs/test/index.client.test.ts | 7 ++++--- packages/nextjs/test/performance/client.test.ts | 3 +++ 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index 02bd299490bc..afbb5544ea78 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -111,7 +111,7 @@ export function nextRouterInstrumentation( if (startTransactionOnPageLoad) { const { route, traceParentData, baggage, params } = extractNextDataTagInformation(); - prevTransactionName = route || global.document.location.pathname; + prevTransactionName = route || global.location.pathname; const source = route ? 'route' : 'url'; activeTransaction = startTransactionCb({ diff --git a/packages/nextjs/test/index.client.test.ts b/packages/nextjs/test/index.client.test.ts index 25f7d27316a6..8752bc62f88c 100644 --- a/packages/nextjs/test/index.client.test.ts +++ b/packages/nextjs/test/index.client.test.ts @@ -20,13 +20,14 @@ const logWarn = jest.spyOn(logger, 'warn'); // Set up JSDom - needed for page load instrumentation const dom = new JSDOM(undefined, { url: 'https://example.com/' }); Object.defineProperty(global, 'document', { value: dom.window.document, writable: true }); +Object.defineProperty(global, 'location', { value: dom.window.document.location, writable: true }); const originalGlobalDocument = getGlobalObject().document; +const originalGlobalLocation = getGlobalObject().location; afterAll(() => { // Clean up JSDom - Object.defineProperty(global, 'document', { - value: originalGlobalDocument, - }); + Object.defineProperty(global, 'document', { value: originalGlobalDocument }); + Object.defineProperty(global, 'location', { value: originalGlobalLocation }); }); describe('Client init()', () => { diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index 78b078dc5d9e..5b94f240cfaa 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -31,6 +31,7 @@ describe('client', () => { describe('nextRouterInstrumentation', () => { const originalGlobalDocument = getGlobalObject().document; + const originalGlobalLocation = getGlobalObject().location; function setUpNextPage(pageProperties: { url: string; @@ -61,11 +62,13 @@ describe('client', () => { ); Object.defineProperty(global, 'document', { value: dom.window.document, writable: true }); + Object.defineProperty(global, 'location', { value: dom.window.document.location, writable: true }); } afterEach(() => { // Clean up JSDom Object.defineProperty(global, 'document', { value: originalGlobalDocument }); + Object.defineProperty(global, 'location', { value: originalGlobalLocation }); }); it('waits for Router.ready()', () => { From bec58e0490fbe4eff16654505173cb77f3e611a3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Aug 2022 15:46:45 +0000 Subject: [PATCH 07/12] Clean up setUpNextPage test helper --- .../nextjs/test/performance/client.test.ts | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index 5b94f240cfaa..e3e089fc550f 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -1,5 +1,6 @@ import { getGlobalObject } from '@sentry/utils'; import { JSDOM } from 'jsdom'; +import { NEXT_DATA as NextData } from 'next/dist/next-server/lib/utils'; import { default as Router } from 'next/router'; import { nextRouterInstrumentation } from '../../src/performance/client'; @@ -40,23 +41,23 @@ describe('client', () => { props: any; 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 some example what a __NEXT_DATA__ tag might look like pageProperties.hasNextData - ? ` - - ` + ? `` : '

No next data :(

', { url: pageProperties.url }, ); From 0ba0d6e6f850ab5040980d1ea7186e67687e2f1a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 12 Aug 2022 15:47:14 +0000 Subject: [PATCH 08/12] Guard Next route data behind default pii flag --- packages/nextjs/src/performance/client.ts | 5 ++++- packages/nextjs/test/performance/client.test.ts | 7 ------- packages/types/src/options.ts | 5 ----- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index afbb5544ea78..dc159c4f85ba 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -1,5 +1,6 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ +import { getCurrentHub } from '@sentry/hub'; import { Primitive, TraceparentData, Transaction, TransactionContext } from '@sentry/types'; import { extractTraceparentData, @@ -93,6 +94,8 @@ let activeTransaction: Transaction | undefined = undefined; let prevTransactionName: string | undefined = undefined; let startTransaction: StartTransactionCb | undefined = undefined; +const client = getCurrentHub().getClient(); + /** * Creates routing instrumention for Next Router. Only supported for * client side routing. Works for Next >= 10. @@ -118,7 +121,7 @@ export function nextRouterInstrumentation( name: prevTransactionName, op: 'pageload', tags: DEFAULT_TAGS, - ...(params && { data: params }), + ...(params && client && client.getOptions().sendDefaultPii && { data: params }), ...traceParentData, metadata: { source, diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index e3e089fc550f..39d61ed6f59a 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -97,11 +97,6 @@ describe('client', () => { tags: { 'routing.instrumentation': 'next-router', }, - data: { - user: 'lforst', - id: '1337', - q: '42', - }, metadata: { source: 'route', baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], @@ -129,7 +124,6 @@ describe('client', () => { tags: { 'routing.instrumentation': 'next-router', }, - data: {}, metadata: { source: 'route', baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], @@ -151,7 +145,6 @@ describe('client', () => { tags: { 'routing.instrumentation': 'next-router', }, - data: {}, metadata: { source: 'route', }, diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 077b49e4105f..78d1c7c9bc6e 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -153,16 +153,11 @@ export interface ClientOptions Date: Fri, 12 Aug 2022 13:37:05 -0400 Subject: [PATCH 09/12] Build `nextDataTagInfo` through mutation --- packages/nextjs/src/performance/client.ts | 61 ++++++++++++++--------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index dc159c4f85ba..735d8130e206 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -33,6 +33,13 @@ interface SentryEnhancedNextData extends NextData { }; } +interface NextDataTagInfo { + route?: string; + traceParentData?: TraceparentData; + baggage?: string; + params?: ParsedUrlQuery; +} + /** * Every Next.js page (static and dynamic ones) comes with a script tag with the id "__NEXT_DATA__". This script tag * contains a JSON object with data that was either generated at build time for static pages (`getStaticProps`), or at @@ -44,14 +51,8 @@ interface SentryEnhancedNextData extends NextData { * * This function extracts this information. */ -function extractNextDataTagInformation(): { - route: string | undefined; - traceParentData: TraceparentData | undefined; - baggage: string | undefined; - params: ParsedUrlQuery | undefined; -} { +function extractNextDataTagInformation(): NextDataTagInfo { let nextData: SentryEnhancedNextData | undefined; - // Let's be on the safe side and actually check first if there is really a __NEXT_DATA__ script tag on the page. // Theoretically this should always be the case though. const nextDataTag = global.document.getElementById('__NEXT_DATA__'); @@ -63,27 +64,39 @@ function extractNextDataTagInformation(): { } } - // `nextData.page` always contains the parameterized route - const route = (nextData || {}).page; - const params = (nextData || {}).query; + if (!nextData) { + return {}; + } - const getInitialPropsBaggage = ((nextData || {}).props || {})._sentryGetInitialPropsBaggage; - const getServerSidePropsBaggage = (((nextData || {}).props || {}).pageProps || {})._sentryGetServerSidePropsBaggage; + const nextDataTagInfo: NextDataTagInfo = {}; - const getInitialPropsTraceData = ((nextData || {}).props || {})._sentryGetInitialPropsTraceData; - const getServerSidePropsTraceData = (((nextData || {}).props || {}).pageProps || {}) - ._sentryGetServerSidePropsTraceData; + const { page, query, props } = nextData; - // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. - const baggage = getInitialPropsBaggage || getServerSidePropsBaggage; - const traceData = getInitialPropsTraceData || getServerSidePropsTraceData; + // `nextData.page` always contains the parameterized route + nextDataTagInfo.route = page; + nextDataTagInfo.params = query; + + if (props) { + const { pageProps } = props; + + const getInitialPropsBaggage = props._sentryGetInitialPropsBaggage; + const getServerSidePropsBaggage = pageProps && pageProps._sentryGetServerSidePropsBaggage; + // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. + const baggage = getInitialPropsBaggage || getServerSidePropsBaggage; + if (baggage) { + nextDataTagInfo.baggage = baggage; + } - return { - route, - params, - traceParentData: traceData ? extractTraceparentData(traceData) : undefined, - baggage, - }; + const getInitialPropsTraceData = props._sentryGetInitialPropsTraceData; + const getServerSidePropsTraceData = pageProps && pageProps._sentryGetServerSidePropsTraceData; + // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. + const traceData = getInitialPropsTraceData || getServerSidePropsTraceData; + if (traceData) { + nextDataTagInfo.traceParentData = extractTraceparentData(traceData); + } + } + + return nextDataTagInfo; } const DEFAULT_TAGS = { From 652d3eb5a4c7341fcf8939430e52086793200a34 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 16 Aug 2022 09:23:02 +0000 Subject: [PATCH 10/12] Add support for `getStaticProps` tracing --- packages/nextjs/src/performance/client.ts | 17 +++++++--- .../nextjs/test/performance/client.test.ts | 31 +++++++++++++++++-- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index 735d8130e206..5d2ccb3cb04f 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -29,6 +29,13 @@ interface SentryEnhancedNextData extends NextData { pageProps?: { _sentryGetServerSidePropsTraceData?: string; // trace parent info, if injected by server-side `getServerSideProps` _sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps` + + // The following two values are only injected in a very special case with the following conditions: + // 1. The page's `getStaticPaths` method must have returned `fallback: 'blocking'`. + // 2. The requested page must be a "miss" in terms of "Incremental Static Regeneration", meaning the requested page has not been generated before. + // In this case, a page is requested and only served when `getStaticProps` is done. There is not even a fallback page or similar. + _sentryGetStaticPropsTraceData?: string; // trace parent info, if injected by server-side `getStaticProps` + _sentryGetStaticPropsBaggage?: string; // baggage, if injected by server-side `getStaticProps` }; }; } @@ -81,16 +88,18 @@ function extractNextDataTagInformation(): NextDataTagInfo { const getInitialPropsBaggage = props._sentryGetInitialPropsBaggage; const getServerSidePropsBaggage = pageProps && pageProps._sentryGetServerSidePropsBaggage; - // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. - const baggage = getInitialPropsBaggage || getServerSidePropsBaggage; + const getStaticPropsBaggage = pageProps && pageProps._sentryGetStaticPropsBaggage; + // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` or `getStaticProps` so we give it priority. + const baggage = getInitialPropsBaggage || getServerSidePropsBaggage || getStaticPropsBaggage; if (baggage) { nextDataTagInfo.baggage = baggage; } const getInitialPropsTraceData = props._sentryGetInitialPropsTraceData; const getServerSidePropsTraceData = pageProps && pageProps._sentryGetServerSidePropsTraceData; - // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` so we give it priority. - const traceData = getInitialPropsTraceData || getServerSidePropsTraceData; + const getStaticPropsTraceData = pageProps && pageProps._sentryGetStaticPropsTraceData; + // Ordering of the following shouldn't matter but `getInitialProps` generally runs before `getServerSideProps` or `getStaticProps` so we give it priority. + const traceData = getInitialPropsTraceData || getServerSidePropsTraceData || getStaticPropsTraceData; if (traceData) { nextDataTagInfo.traceParentData = extractTraceparentData(traceData); } diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index 39d61ed6f59a..61b8afcab340 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -107,8 +107,8 @@ describe('client', () => { }, ], [ - 'https://example.com/static', - '/static', + 'https://example.com/dynamic', + '/dynamic', {}, { pageProps: { @@ -118,6 +118,33 @@ describe('client', () => { }, }, true, + { + name: '/dynamic', + op: 'pageload', + tags: { + 'routing.instrumentation': 'next-router', + }, + metadata: { + source: 'route', + baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], + }, + traceId: 'c82b8554881b4d28ad977de04a4fb40a', + parentSpanId: 'a755953cd3394d5f', + parentSampled: true, + }, + ], + [ + 'https://example.com/static', + '/static', + {}, + { + pageProps: { + _sentryGetStaticPropsTraceData: 'c82b8554881b4d28ad977de04a4fb40a-a755953cd3394d5f-1', + _sentryGetStaticPropsBaggage: + 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', + }, + }, + true, { name: '/static', op: 'pageload', From 49f51d786523671c35bfbfcf1fe5236467423369 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 16 Aug 2022 09:35:16 +0000 Subject: [PATCH 11/12] Clarify some stuff in tests --- packages/nextjs/test/index.client.test.ts | 4 +++- packages/nextjs/test/performance/client.test.ts | 9 ++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/test/index.client.test.ts b/packages/nextjs/test/index.client.test.ts index 8752bc62f88c..6ec25cc60381 100644 --- a/packages/nextjs/test/index.client.test.ts +++ b/packages/nextjs/test/index.client.test.ts @@ -17,7 +17,9 @@ const reactInit = jest.spyOn(SentryReact, 'init'); const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent'); const logWarn = jest.spyOn(logger, 'warn'); -// Set up JSDom - needed for page load instrumentation +// We're setting up JSDom here because 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` const dom = new JSDOM(undefined, { url: 'https://example.com/' }); Object.defineProperty(global, 'document', { value: dom.window.document, writable: true }); Object.defineProperty(global, 'location', { value: dom.window.document.location, writable: true }); diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index 61b8afcab340..2a90b7db45a1 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -53,7 +53,7 @@ describe('client', () => { }; const dom = new JSDOM( - // Just some example what a __NEXT_DATA__ tag might look like + // Just an example of what a __NEXT_DATA__ tag might look like pageProperties.hasNextData ? `