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): Create transactions in getInitialProps and getServerSideProps #5593

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const origGetStaticProps = userPageModule.getStaticProps;
const origGetServerSideProps = userPageModule.getServerSideProps;

if (typeof origGetInitialProps === 'function') {
pageComponent.getInitialProps = Sentry.withSentryGetInitialProps(
pageComponent.getInitialProps = Sentry.withSentryServerSideGetInitialProps(
origGetInitialProps,
'__ROUTE__',
) as NextPageComponent['getInitialProps'];
Expand Down
2 changes: 1 addition & 1 deletion packages/nextjs/src/config/wrappers/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export { withSentryGetStaticProps } from './withSentryGetStaticProps';
export { withSentryGetInitialProps } from './withSentryGetInitialProps';
export { withSentryGetServerSideProps } from './withSentryGetServerSideProps';
export { withSentryServerSideGetInitialProps } from './withSentryServerSideGetInitialProps';
26 changes: 0 additions & 26 deletions packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { hasTracingEnabled } from '@sentry/tracing';
import { GetServerSideProps } from 'next';

import { callDataFetcherTraced } from './wrapperUtils';
import { isBuild } from '../../utils/isBuild';
import { callTracedServerSideDataFetcher, withErrorInstrumentation } from './wrapperUtils';

/**
* Create a wrapped version of the user's exported `getServerSideProps` function
Expand All @@ -16,9 +18,22 @@ export function withSentryGetServerSideProps(
return async function (
...getServerSidePropsArguments: Parameters<GetServerSideProps>
): ReturnType<GetServerSideProps> {
return callDataFetcherTraced(origGetServerSideProps, getServerSidePropsArguments, {
parameterizedRoute,
dataFetchingMethodName: 'getServerSideProps',
});
if (isBuild()) {
return origGetServerSideProps(...getServerSidePropsArguments);
}

const [context] = getServerSidePropsArguments;
const { req, res } = context;

const errorWrappedGetServerSideProps = withErrorInstrumentation(origGetServerSideProps);

if (hasTracingEnabled()) {
return callTracedServerSideDataFetcher(errorWrappedGetServerSideProps, getServerSidePropsArguments, req, res, {
parameterizedRoute,
functionName: 'getServerSideProps',
});
} else {
return errorWrappedGetServerSideProps(...getServerSidePropsArguments);
}
};
}
11 changes: 9 additions & 2 deletions packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { GetStaticProps } from 'next';

import { callDataFetcherTraced } from './wrapperUtils';
import { isBuild } from '../../utils/isBuild';
import { callDataFetcherTraced, withErrorInstrumentation } from './wrapperUtils';

type Props = { [key: string]: unknown };

Expand All @@ -18,7 +19,13 @@ export function withSentryGetStaticProps(
return async function (
...getStaticPropsArguments: Parameters<GetStaticProps<Props>>
): ReturnType<GetStaticProps<Props>> {
return callDataFetcherTraced(origGetStaticProps, getStaticPropsArguments, {
if (isBuild()) {
return origGetStaticProps(...getStaticPropsArguments);
}

const errorWrappedGetStaticProps = withErrorInstrumentation(origGetStaticProps);

return callDataFetcherTraced(errorWrappedGetStaticProps, getStaticPropsArguments, {
parameterizedRoute,
dataFetchingMethodName: 'getStaticProps',
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { hasTracingEnabled } from '@sentry/tracing';
import { NextPage } from 'next';

import { isBuild } from '../../utils/isBuild';
import { callTracedServerSideDataFetcher, withErrorInstrumentation } from './wrapperUtils';

type GetInitialProps = Required<NextPage>['getInitialProps'];

/**
* Create a wrapped version of the user's exported `getInitialProps` function
*
* @param origGetInitialProps The user's `getInitialProps` function
* @param parameterizedRoute The page's parameterized route
* @returns A wrapped version of the function
*/
export function withSentryServerSideGetInitialProps(
origGetInitialProps: GetInitialProps,
parameterizedRoute: string,
): GetInitialProps {
return async function (
...getInitialPropsArguments: Parameters<GetInitialProps>
): Promise<ReturnType<GetInitialProps>> {
if (isBuild()) {
return origGetInitialProps(...getInitialPropsArguments);
}

const [context] = getInitialPropsArguments;
const { req, res } = context;

const errorWrappedGetInitialProps = withErrorInstrumentation(origGetInitialProps);

if (hasTracingEnabled()) {
// Since this wrapper is only applied to `getInitialProps` running on the server, we can assert that `req` and
// `res` are always defined: https://nextjs.org/docs/api-reference/data-fetching/get-initial-props#context-object
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return callTracedServerSideDataFetcher(errorWrappedGetInitialProps, getInitialPropsArguments, req!, res!, {
parameterizedRoute,
functionName: 'getInitialProps',
});
} else {
return errorWrappedGetInitialProps(...getInitialPropsArguments);
}
};
}
126 changes: 125 additions & 1 deletion packages/nextjs/src/config/wrappers/wrapperUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,129 @@
import { captureException } from '@sentry/core';
import { captureException, getCurrentHub, startTransaction } from '@sentry/core';
import { addRequestDataToEvent } from '@sentry/node';
import { getActiveTransaction } from '@sentry/tracing';
import { Transaction } from '@sentry/types';
import { fill } from '@sentry/utils';
import * as domain from 'domain';
import { IncomingMessage, ServerResponse } from 'http';

declare module 'http' {
interface IncomingMessage {
_sentryTransaction?: Transaction;
}
}

function getTransactionFromRequest(req: IncomingMessage): Transaction | undefined {
return req._sentryTransaction;
}

function setTransactionOnRequest(transaction: Transaction, req: IncomingMessage): void {
req._sentryTransaction = transaction;
}

function autoEndTransactionOnResponseEnd(transaction: Transaction, res: ServerResponse): void {
fill(res, 'end', (originalEnd: ServerResponse['end']) => {
return function (this: unknown, ...endArguments: Parameters<ServerResponse['end']>) {
transaction.finish();
return originalEnd.call(this, ...endArguments);
};
});
}

/**
* Wraps a function that potentially throws. If it does, the error is passed to `captureException` and rethrown.
*/
export function withErrorInstrumentation<F extends (...args: any[]) => any>(
origFunction: F,
): (...params: Parameters<F>) => ReturnType<F> {
lforst marked this conversation as resolved.
Show resolved Hide resolved
return function (this: unknown, ...origFunctionArguments: Parameters<F>): ReturnType<F> {
try {
const potentialPromiseResult = origFunction.call(this, ...origFunctionArguments);
// First of all, we need to capture promise rejections so we have the following check, as well as the try-catch block.
// Additionally, we do the following instead of `await`-ing so we do not change the method signature of the passed function from `() => unknown` to `() => Promise<unknown>.
Promise.resolve(potentialPromiseResult).catch(err => {
// TODO: Extract error logic from `withSentry` in here or create a new wrapper with said logic or something like that.
captureException(err);
});
return potentialPromiseResult;
} catch (e) {
// TODO: Extract error logic from `withSentry` in here or create a new wrapper with said logic or something like that.
captureException(e);
throw e;
}
};
}

/**
* Calls a server-side data fetching function (that takes a `req` and `res` object in its context) with tracing
* instrumentation. A transaction will be created for the incoming request (if it doesn't already exist) in addition to
* a span for the wrapped data fetching function.
*
* All of the above happens in an isolated domain, meaning all thrown errors will be associated with the correct span.
*
* @param origFunction The data fetching method to call.
* @param origFunctionArguments The arguments to call the data fetching method with.
* @param req The data fetching function's request object.
* @param res The data fetching function's response object.
* @param options Options providing details for the created transaction and span.
* @returns what the data fetching method call returned.
*/
export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Promise<any> | any>(
origFunction: F,
origFunctionArguments: Parameters<F>,
req: IncomingMessage,
res: ServerResponse,
options: {
parameterizedRoute: string;
functionName: string;
},
): Promise<ReturnType<F>> {
return domain.create().bind(async () => {
let requestTransaction: Transaction | undefined = getTransactionFromRequest(req);

if (requestTransaction === undefined) {
// TODO: Extract trace data from `req` object (trace and baggage headers) and attach it to transaction

const newTransaction = startTransaction({
op: 'nextjs.data',
name: options.parameterizedRoute,
metadata: {
source: 'route',
},
});

requestTransaction = newTransaction;
autoEndTransactionOnResponseEnd(newTransaction, res);
setTransactionOnRequest(newTransaction, req);
}

const dataFetcherSpan = requestTransaction.startChild({
op: 'nextjs.data',
description: `${options.functionName} (${options.parameterizedRoute})`,
});

const currentScope = getCurrentHub().getScope();
if (currentScope) {
currentScope.setSpan(dataFetcherSpan);
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... What happens if multiple data-fetching functions for the same request are running at the same time? (Not a current concern, but it will be one eventually, and I figure we should future-proof ourselves as much as we can.)

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 think we're good in this situation. Since we have an individual domain for each of the data fetchers, they also have their own hub/cloned scope, so setting the span should "just work". Let me know if I misunderstand something.

Copy link
Member

Choose a reason for hiding this comment

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

Since we have an individual domain for each of the data fetchers

Yeah, okay, I see what you're saying. I think I'm stuck in the mindset which applies in our other framework integrations, where it's one domain/request. I'm trying to think if there's any reason we'd need to have all of a request's data fetchers live in the same domain. I guess the question is, is there anything which happens to the scope in one which we'd want to be persisted to another?

Thoughts:

  • We don't know the order in which the data fetchers will run, so even if they were all running in the same domain, we can't count on one changing the scope in a way that effects the others, because it might not run first.
  • In a way, we're treating the request like a persisted scope, in that we're attaching the transaction to it (the one thing we do need to be the same for all data fetchers).
  • I don't think (?) there's anything else we automatically do to the scope which we wouldn't want to lose.

So I think the answer is no, because we don't know the order in which they'll run, so even if they were in the same domain, we couldn't count on one setting data that the next one could use. The one exception here is the event processor to add request data. That's gotta be attached to whatever scope is active when we call transaction.finish. Off-the-cuff idea: In addition to attaching the transaction to the request, we should also store a reference to the request in the transaction metadata. Then we can add an event processor at SDK startup which grabs the request out of sdkProcessingMetadata and uses it to populate request data into the event.

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 spent the night thinking about if there is a really good reason for only having one domain across all data fetchers, since having only one domain has its drawbacks with error association. So far I've not come up with anything. I also believe users in any case already have the mental model that data fetchers are more or less isolated and do not share data between them. If bad comes to worst we can explain this behavior in the docs.

As for the request data processor, it turned out to be even simpler. transaction.finish() is still called within the individual data fetcher's scope, so adding the event processor inside callTracedServerSideDataFetcher just works: 3141759 👍

currentScope.addEventProcessor(event =>
addRequestDataToEvent(event, req, {
include: {
// When the `transaction` option is set to true, it tries to extract a transaction name from the request
// object. We don't want this since we already have a high-quality transaction name with a parameterized
// route. Setting `transaction` to `true` will clobber that transaction name.
transaction: false,
},
}),
);
}

try {
// TODO: Inject trace data into returned props
return await origFunction(...origFunctionArguments);
} finally {
dataFetcherSpan.finish();
}
})();
}

/**
* Call a data fetcher and trace it. Only traces the function if there is an active transaction on the scope.
Expand Down
2 changes: 0 additions & 2 deletions packages/nextjs/src/index.client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ export * from '@sentry/react';
export { nextRouterInstrumentation } from './performance/client';
export { captureUnderscoreErrorException } from './utils/_error';

export { withSentryGetInitialProps } from './config/wrappers';

export { Integrations };

// Previously we expected users to import `BrowserTracing` like this:
Expand Down
6 changes: 5 additions & 1 deletion packages/nextjs/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,11 @@ function addServerIntegrations(options: NextjsOptions): void {
export type { SentryWebpackPluginOptions } from './config/types';
export { withSentryConfig } from './config';
export { isBuild } from './utils/isBuild';
export { withSentryGetServerSideProps, withSentryGetStaticProps, withSentryGetInitialProps } from './config/wrappers';
export {
withSentryGetServerSideProps,
withSentryGetStaticProps,
withSentryServerSideGetInitialProps,
} from './config/wrappers';
export { withSentry } from './utils/withSentry';

// Wrap various server methods to enable error monitoring and tracing. (Note: This only happens for non-Vercel
Expand Down