feat(nextjs): Improve pageload transaction creation #5574
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,13 +1,104 @@ | ||||||||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||||||||
|
|
||||||||
| import { getCurrentHub } from '@sentry/hub'; | ||||||||
| 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 parent info, if injected by server-side `getInitialProps` | ||||||||
| _sentryGetInitialPropsBaggage?: string; // baggage, if injected by server-side `getInitialProps` | ||||||||
| pageProps?: { | ||||||||
| _sentryGetServerSidePropsTraceData?: string; // trace parent info, if injected by server-side `getServerSideProps` | ||||||||
| _sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps` | ||||||||
|
Comment on lines
+30
to
+31
There was a problem hiding this comment. I know we're leaving |
||||||||
| }; | ||||||||
| }; | ||||||||
| } | ||||||||
|
|
||||||||
| 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 | ||||||||
| * 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(): NextDataTagInfo { | ||||||||
| 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__'); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| if (!nextData) { | ||||||||
| return {}; | ||||||||
| } | ||||||||
|
|
||||||||
| const nextDataTagInfo: NextDataTagInfo = {}; | ||||||||
|
|
||||||||
| const { page, query, props } = nextData; | ||||||||
|
|
||||||||
| // `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; | ||||||||
| } | ||||||||
|
|
||||||||
| 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 = { | ||||||||
| 'routing.instrumentation': 'next-router', | ||||||||
| } as const; | ||||||||
|
|
@@ -16,6 +107,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. | ||||||||
|
|
@@ -30,24 +123,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.location.pathname; | ||||||||
| const source = route ? 'route' : 'url'; | ||||||||
|
|
||||||||
| activeTransaction = startTransactionCb({ | ||||||||
| name: prevTransactionName, | ||||||||
| op: 'pageload', | ||||||||
| tags: DEFAULT_TAGS, | ||||||||
| ...(params && client && client.getOptions().sendDefaultPii && { data: params }), | ||||||||
| ...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 +174,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 +211,4 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap | |||||||
| } | ||||||||
| return originalChangeStateWrapper.call(this, method, url, as, options, ...args); | ||||||||
| }; | ||||||||
| } | ||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,16 +153,11 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp | |
| tunnel?: string; | ||
|
|
||
| /** | ||
|
There was a problem hiding this comment. Rather than just deleting this line, I think we should say that it works client-side in the nextjs SDK but not elsewhere. |
||
| * Controls if potentially sensitive data should be sent to Sentry by default. | ||
| * Note that this only applies to data that the SDK is sending by default | ||
| * but not data that was explicitly set (e.g. by calling `Sentry.setUser()`). | ||
| * | ||
| * Defaults to `false`. | ||
| */ | ||
| sendDefaultPii?: boolean; | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.