feat(nextjs): Improve pageload transaction creation #5574
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,13 +1,90 @@ | ||||||||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||||||||
|
|
||||||||
| 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'; | ||||||||
|
|
||||||||
| const global = getGlobalObject<Window>(); | ||||||||
|
|
||||||||
| 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: { | ||||||||
| _sentryGetInitialPropsTraceData?: string; // trace id, if injected by server-side `getInitialProps` | ||||||||
|
There was a problem hiding this comment. it's not just trace id - let's refer to it as trace parent info since it has sampled flag and parent span id as well. |
||||||||
| _sentryGetInitialPropsBaggage?: string; // baggage, if injected by server-side `getInitialProps` | ||||||||
| pageProps?: { | ||||||||
| _sentryGetServerSidePropsTraceData?: string; // trace id, if injected by server-side `getServerSideProps` | ||||||||
| _sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps` | ||||||||
| }; | ||||||||
| }; | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * 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.`. | ||||||||
|
Comment on lines
+51
to
+53
There was a problem hiding this comment. Given that There was a problem hiding this comment. I finally read up on how ISR works and I think I fully understand it now. There is one case where it totally makes sense to trace |
||||||||
| * | ||||||||
| * 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 | undefined; | ||||||||
| traceParentData: TraceparentData | undefined; | ||||||||
| baggage: string | undefined; | ||||||||
| params: ParsedUrlQuery | undefined; | ||||||||
| } { | ||||||||
| let nextData: SentryEnhancedNextData | undefined; | ||||||||
|
There was a problem hiding this comment. low: IMO I would like this function to act as a builder, perhaps something like so: const nextDataTagInfo = {};
if (nextData) {
nextDataTagInfo.route = nextData.page;
nextDataTagInfo.params = nextData.query;
if (nextData.props) {
// ...
}
}There was a problem hiding this comment. I just tried this. Trust me, it's uglier than it is right now + you'll get problems with typing. I'm leaving it as is. Mutability is the root of all evil. Feel free to push changes to this if you want though. There was a problem hiding this comment. Done with 1bde09a, let me know what you think! (feel free to revert if you think it'll cause more bugs than less). |
||||||||
|
|
||||||||
| // 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 { | ||||||||
| 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; | ||||||||
| const params = (nextData || {}).query; | ||||||||
|
|
||||||||
| const getInitialPropsBaggage = ((nextData || {}).props || {})._sentryGetInitialPropsBaggage; | ||||||||
| const getServerSidePropsBaggage = (((nextData || {}).props || {}).pageProps || {})._sentryGetServerSidePropsBaggage; | ||||||||
|
|
||||||||
| 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, | ||||||||
| traceParentData: traceData ? extractTraceparentData(traceData) : undefined, | ||||||||
| baggage, | ||||||||
| }; | ||||||||
| } | ||||||||
|
|
||||||||
| const DEFAULT_TAGS = { | ||||||||
| 'routing.instrumentation': 'next-router', | ||||||||
| } as const; | ||||||||
|
|
@@ -30,24 +107,27 @@ export function nextRouterInstrumentation( | |||||||
| startTransactionOnLocationChange: boolean = true, | ||||||||
| ): void { | ||||||||
| startTransaction = startTransactionCb; | ||||||||
|
|
||||||||
| if (startTransactionOnPageLoad) { | ||||||||
| const { route, traceParentData, baggage, params } = extractNextDataTagInformation(); | ||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||
|
|
||||||||
| prevTransactionName = route || global.document.location.pathname; | ||||||||
|
There was a problem hiding this comment. why the change to There was a problem hiding this comment. It was just easier to test. I generally do not change stuff for the sake of changing them. -> 651e8e0 |
||||||||
| const source = route ? 'route' : 'url'; | ||||||||
|
|
||||||||
| activeTransaction = startTransactionCb({ | ||||||||
| name: prevTransactionName, | ||||||||
| op: 'pageload', | ||||||||
| tags: DEFAULT_TAGS, | ||||||||
| ...(params && { data: params }), | ||||||||
|
There was a problem hiding this comment. iirc setting data does nothing for transactions - can we throw it into some other part of the event? Like a context? Does this have PII data concerns? Perhaps we guard this with There was a problem hiding this comment. Finally found a use for that flag! Yup there are potential pii issues. Added the flag but removed tests around data because that's just not worth the effort of mocking the hub/client/options. 0ba0d6e Btw, this data shows up in the trace details when you open a transaction:
There was a problem hiding this comment.
Ah yeah - that means it'll show up on linked errors as well (since we copy over the entire trace context). I guess since we don't index this we can leave it for now. |
||||||||
| ...traceParentData, | ||||||||
| 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 +158,7 @@ type WrappedRouterChangeState = RouterChangeState; | |||||||
| * Start a navigation transaction every time the router changes state. | ||||||||
| */ | ||||||||
| function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): WrappedRouterChangeState { | ||||||||
| return function wrapper( | ||||||||
| this: any, | ||||||||
| method: string, | ||||||||
| // The parameterized url, ex. posts/[id]/[comment] | ||||||||
|
|
@@ -115,5 +195,4 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap | |||||||
| } | ||||||||
| return originalChangeStateWrapper.call(this, method, url, as, options, ...args); | ||||||||
| }; | ||||||||
| } | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other fields in NextData that we can use? https://github.com/vercel/next.js/blob/0796b6faa991cd58a517b0f160320e9978208066/packages/next/shared/lib/utils.ts#L84-L109 - perhaps create a follow up issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poked around in the type. Nothing really sticks out as very useful. I can create an issue but very low prio imo.