Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(nextjs): Improve pageload transaction creation #5574

Merged
merged 13 commits into from
Aug 16, 2022
146 changes: 125 additions & 21 deletions packages/nextjs/src/performance/client.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,113 @@
/* eslint-disable @typescript-eslint/no-explicit-any */

import { Primitive, Transaction, TransactionContext } from '@sentry/types';
import { fill, getGlobalObject, stripUrlQueryAndFragment } from '@sentry/utils';
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 {
Copy link
Member

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?

Copy link
Member Author

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.

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we're leaving getStaticProps somewhat to the side for the moment, but do you think it's worth it to add a TODO about it here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


// 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`
};
};
}

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that getStaticProps runs an arbitrary amount of time before the page is served (both when it's run at build time and when it's run at runtime as part of revalidation), it doesn't make sense to send trace data from it, only the parameterized route. Do we want to note that here somehow? Could be in the docstring here or could be below where you're dealing with the other two, to explain why it's not all three.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 getStaticProps and I went ahead and added logic and an explanation for it: 652d3eb

*
* 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
    // ...
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
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;
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);
}
}

return nextDataTagInfo;
}

const DEFAULT_TAGS = {
'routing.instrumentation': 'next-router',
} as const;
Expand All @@ -16,6 +116,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.
Expand All @@ -30,24 +132,27 @@ 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, traceParentData, baggage, params } = extractNextDataTagInformation();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { route, traceParentData, baggage, params } = extractNextDataTagInformation();
// Pull data from the `__NEXT_DATA__` script tag automatically injected serverside by nextjs
const { route, traceParentData, baggage, params } = extractNextDataTagInformation();


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;
Expand Down Expand Up @@ -78,7 +183,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]
Expand Down Expand Up @@ -115,5 +220,4 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap
}
return originalChangeStateWrapper.call(this, method, url, as, options, ...args);
};
return wrapper;
}
16 changes: 16 additions & 0 deletions packages/nextjs/test/index.client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } from '@sentry/utils';
import { JSDOM } from 'jsdom';

import { init, Integrations, nextRouterInstrumentation } from '../src/index.client';
import { NextjsOptions } from '../src/utils/nextjsOptions';
Expand All @@ -16,6 +17,21 @@ const reactInit = jest.spyOn(SentryReact, 'init');
const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent');
const loggerLogSpy = jest.spyOn(logger, 'log');

// 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 });

const originalGlobalDocument = getGlobalObject<Window>().document;
const originalGlobalLocation = getGlobalObject<Window>().location;
afterAll(() => {
// Clean up JSDom
Object.defineProperty(global, 'document', { value: originalGlobalDocument });
Object.defineProperty(global, 'location', { value: originalGlobalLocation });
});

describe('Client init()', () => {
afterEach(() => {
jest.clearAllMocks();
Expand Down
Loading