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): Trace navigation transactions #5676

Merged
merged 15 commits into from
Sep 6, 2022
Merged
161 changes: 76 additions & 85 deletions packages/nextjs/src/performance/client.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
/* eslint-disable @typescript-eslint/no-explicit-any */

import { getCurrentHub } from '@sentry/hub';
import { Primitive, TraceparentData, Transaction, TransactionContext } from '@sentry/types';
import { Primitive, TraceparentData, Transaction, TransactionContext, TransactionSource } from '@sentry/types';
import {
extractTraceparentData,
fill,
getGlobalObject,
logger,
parseBaggageHeader,
Expand All @@ -14,7 +11,13 @@ 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>();
const global = getGlobalObject<
Window & {
__BUILD_MANIFEST?: {
sortedPages?: string[];
};
}
>();

type StartTransactionCb = (context: TransactionContext) => Transaction | undefined;

Expand Down Expand Up @@ -76,6 +79,8 @@ function extractNextDataTagInformation(): NextDataTagInfo {
// `nextData.page` always contains the parameterized route - except for when an error occurs in a data fetching
// function, then it is "/_error", but that isn't a problem since users know which route threw by looking at the
// parent transaction
// TODO: Actually this is a problem (even though it is not that big), because the DSC and the transaction payload will contain
// a different transaction name. Maybe we can fix this. Idea: Also send transaction name via pageProps when available.
nextDataTagInfo.route = page;
nextDataTagInfo.params = query;

Expand All @@ -96,20 +101,12 @@ const DEFAULT_TAGS = {
'routing.instrumentation': 'next-router',
} as const;

// We keep track of the active transaction so we can finish it when we start a navigation transaction.
let activeTransaction: Transaction | undefined = undefined;
let startTransaction: StartTransactionCb | undefined = undefined;

// We keep track of the previous page location so we can avoid creating transactions when navigating to the same page.
// This variable should always contain a pathname. (without query string or fragment)
// We are making a tradeoff by not starting transactions when just the query string changes. One could argue that we
// should in fact start transactions when the query changes, however, in some cases (for example when typing in a search
// box) the query might change multiple times a second, resulting in way too many transactions.
// Because we currently don't have a real way of preventing transactions to be created in this case (except for the
// shotgun approach `startTransactionOnLocationChange: false`), we won't start transactions when *just* the query changes.
let previousLocation: string | undefined = undefined;

// We keep track of the previous transaction name so we can set the `from` field on navigation transactions.
let prevTransactionName: string | undefined = undefined;
// 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 prevLocationName: string | undefined = undefined;

const client = getCurrentHub().getClient();

Expand All @@ -126,18 +123,14 @@ export function nextRouterInstrumentation(
startTransactionOnPageLoad: boolean = true,
startTransactionOnLocationChange: boolean = true,
): void {
startTransaction = startTransactionCb;
const { route, traceParentData, baggage, params } = extractNextDataTagInformation();
prevLocationName = route || global.location.pathname;

if (startTransactionOnPageLoad) {
const { route, traceParentData, baggage, params } = extractNextDataTagInformation();

prevTransactionName = route || global.location.pathname;
previousLocation = global.location.pathname;

const source = route ? 'route' : 'url';

activeTransaction = startTransactionCb({
name: prevTransactionName,
name: prevLocationName,
op: 'pageload',
tags: DEFAULT_TAGS,
...(params && client && client.getOptions().sendDefaultPii && { data: params }),
Expand All @@ -149,78 +142,76 @@ export function nextRouterInstrumentation(
});
}

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;

// `withRouter` uses `useRouter` underneath:
// https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/with-router.tsx#L21
// Router events also use the router:
// https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/router.ts#L92
// `Router.changeState` handles the router state changes, so it may be enough to only wrap it
// (instead of wrapping all of the Router's functions).
const routerPrototype = Object.getPrototypeOf(Router.router);
fill(routerPrototype, 'changeState', changeStateWrapper);
});
}
if (startTransactionOnLocationChange) {
Router.events.on('routeChangeStart', (navigationTarget: string) => {
const matchedRoute = getNextRouteFromPathname(stripUrlQueryAndFragment(navigationTarget));

type RouterChangeState = (
method: string,
url: string,
as: string,
options: Record<string, any>,
...args: any[]
) => void;
type WrappedRouterChangeState = RouterChangeState;
let transactionName: string;
let transactionSource: TransactionSource;

/**
* Wraps Router.changeState()
* https://github.com/vercel/next.js/blob/da97a18dafc7799e63aa7985adc95f213c2bf5f3/packages/next/next-server/lib/router/router.ts#L1204
* 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]
url: string,
// The actual url, ex. posts/85/my-comment
as: string,
options: Record<string, any>,
// At the moment there are no additional arguments (meaning the rest parameter is empty).
// This is meant to protect from future additions to Next.js API, especially since this is an
// internal API.
...args: any[]
): Promise<boolean> {
const newTransactionName = stripUrlQueryAndFragment(url);

// do not start a transaction if it's from the same page
if (startTransaction !== undefined && previousLocation !== as) {
previousLocation = as;

if (activeTransaction) {
activeTransaction.finish();
if (matchedRoute) {
transactionName = matchedRoute;
transactionSource = 'route';
} else {
transactionName = navigationTarget;
transactionSource = 'url';
}

const tags: Record<string, Primitive> = {
...DEFAULT_TAGS,
method,
...options,
from: prevLocationName,
};

if (prevTransactionName) {
tags.from = prevTransactionName;
prevLocationName = transactionName;

if (activeTransaction) {
activeTransaction.finish();
}

prevTransactionName = newTransactionName;
activeTransaction = startTransaction({
name: prevTransactionName,
startTransactionCb({
name: transactionName,
op: 'navigation',
tags,
metadata: { source: 'route' },
metadata: { source: transactionSource },
});
}
return originalChangeStateWrapper.call(this, method, url, as, options, ...args);
};
});
}
}

function getNextRouteFromPathname(pathname: string): string | void {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not the biggest fan of union typing with void - can we just union type with undefined and have an early return? (see microsoft/TypeScript#42709 on some larger issues with void that I share similar opinions on)

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course! TIL 53b5021

const pageRoutes = (global.__BUILD_MANIFEST || {}).sortedPages;

// Page route should in 99.999% of the cases be defined by now but just to be sure we make a check here
if (pageRoutes) {
return pageRoutes.find(route => {
const routeRegExp = convertNextRouteToRegExp(route);
return pathname.match(routeRegExp);
});
}
}

function convertNextRouteToRegExp(route: string): RegExp {
// We can assume a route is at least "/".
const routeParts = route.split('/');

let optionalCatchallWildcardRegex = '';
if (routeParts[routeParts.length - 1].match(/^\[\[\.\.\..+\]\]$/)) {
// If last route part has pattern "[[...xyz]]"
// We pop the latest route part to get rid of the required trailing slash
routeParts.pop();
optionalCatchallWildcardRegex = '(?:/(.+?))?';
}

const rejoinedRouteParts = routeParts
.map(
routePart =>
routePart
.replace(/^\[\.\.\..+\]$/, '(.+?)') // Replace catch all wildcard with regex wildcard
.replace(/^\[.*\]$/, '([^/]+?)'), // Replace route wildcards with lazy regex wildcards
)
.join('/');

return new RegExp(
`^${rejoinedRouteParts}${optionalCatchallWildcardRegex}(?:/)?$`, // optional slash at the end
);
}
11 changes: 10 additions & 1 deletion packages/nextjs/test/integration/pages/[id]/withInitialProps.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
const WithInitialPropsPage = ({ data }: { data: string }) => <h1>WithInitialPropsPage {data}</h1>;
import Link from 'next/link';

const WithInitialPropsPage = ({ data }: { data: string }) => (
<>
<h1>WithInitialPropsPage {data}</h1>
<Link href="/1337/withServerSideProps">
<a id="server-side-props-page">Go to withServerSideProps</a>
</Link>
</>
);

WithInitialPropsPage.getInitialProps = () => {
return { data: '[some getInitialProps data]' };
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
const WithServerSidePropsPage = ({ data }: { data: string }) => <h1>WithServerSidePropsPage {data}</h1>;
import Link from 'next/link';

const WithServerSidePropsPage = ({ data }: { data: string }) => (
<>
<h1>WithServerSidePropsPage {data}</h1>
<Link href="/3c2e87573d/withInitialProps">
<a id="initial-props-page">Go to withInitialProps</a>
</Link>
</>
);

export async function getServerSideProps() {
return { props: { data: '[some getServerSideProps data]' } };
Expand Down
16 changes: 8 additions & 8 deletions packages/nextjs/test/integration/test/client/tracingNavigate.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ const { sleep } = require('../utils/common');
const { expectRequestCount, isTransactionRequest, expectTransaction } = require('../utils/client');

module.exports = async ({ page, url, requests }) => {
await page.goto(`${url}/healthy`);
await page.goto(`${url}/42/withInitialProps/`);
await page.waitForRequest(isTransactionRequest);

expectTransaction(requests.transactions[0], {
transaction: '/healthy',
transaction: '/[id]/withInitialProps',
type: 'transaction',
contexts: {
trace: {
Expand All @@ -17,35 +17,35 @@ module.exports = async ({ page, url, requests }) => {

await sleep(250);

await page.click('a#alsoHealthy');
await page.click('a#server-side-props-page');
await page.waitForRequest(isTransactionRequest);

expectTransaction(requests.transactions[1], {
transaction: '/alsoHealthy',
transaction: '/[id]/withServerSideProps',
type: 'transaction',
contexts: {
trace: {
op: 'navigation',
tags: {
from: '/healthy',
from: '/[id]/withInitialProps',
},
},
},
});

await sleep(250);

await page.click('a#healthy');
await page.click('a#initial-props-page');
await page.waitForRequest(isTransactionRequest);

expectTransaction(requests.transactions[2], {
transaction: '/healthy',
transaction: '/[id]/withInitialProps',
type: 'transaction',
contexts: {
trace: {
op: 'navigation',
tags: {
from: '/alsoHealthy',
from: '/[id]/withServerSideProps',
},
},
},
Expand Down
Loading