-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 spans and route parameterization in server-side getInitialProps
#5587
Changes from all commits
927746c
88b8d18
7c93961
02c1d43
d996be7
515a1b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,26 @@ | ||
import { GIProps } from './types'; | ||
import { NextPage } from 'next'; | ||
|
||
import { callDataFetcherTraced } from './wrapperUtils'; | ||
|
||
type GetInitialProps = Required<NextPage<unknown>>['getInitialProps']; | ||
|
||
/** | ||
* Create a wrapped version of the user's exported `getInitialProps` function | ||
* | ||
* @param origGIProps: The user's `getInitialProps` function | ||
* @param origGIPropsHost: The user's object on which `getInitialProps` lives (used for `this`) | ||
* @param origGetInitialProps The user's `getInitialProps` function | ||
* @param parameterizedRoute The page's parameterized route | ||
* @returns A wrapped version of the function | ||
*/ | ||
export function withSentryGetInitialProps(origGIProps: GIProps['fn']): GIProps['wrappedFn'] { | ||
return async function (this: unknown, ...args: Parameters<GIProps['fn']>) { | ||
return await origGIProps.call(this, ...args); | ||
export function withSentryGetInitialProps( | ||
origGetInitialProps: GetInitialProps, | ||
parameterizedRoute: string, | ||
): GetInitialProps { | ||
return async function ( | ||
...getInitialPropsArguments: Parameters<GetInitialProps> | ||
): Promise<ReturnType<GetInitialProps>> { | ||
return callDataFetcherTraced(origGetInitialProps, getInitialPropsArguments, { | ||
parameterizedRoute, | ||
dataFetchingMethodName: 'getInitialProps', | ||
}); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,24 @@ | ||
import { GSSP } from './types'; | ||
import { wrapperCore } from './wrapperUtils'; | ||
import { GetServerSideProps } from 'next'; | ||
|
||
import { callDataFetcherTraced } from './wrapperUtils'; | ||
|
||
/** | ||
* Create a wrapped version of the user's exported `getServerSideProps` function | ||
* | ||
* @param origGetServerSideProps: The user's `getServerSideProps` function | ||
* @param route: The page's parameterized route | ||
* @param origGetServerSideProps The user's `getServerSideProps` function | ||
* @param parameterizedRoute The page's parameterized route | ||
* @returns A wrapped version of the function | ||
*/ | ||
export function withSentryGetServerSideProps(origGetServerSideProps: GSSP['fn'], route: string): GSSP['wrappedFn'] { | ||
return async function (context: GSSP['context']): Promise<GSSP['result']> { | ||
return wrapperCore<GSSP>(origGetServerSideProps, context, route); | ||
export function withSentryGetServerSideProps( | ||
origGetServerSideProps: GetServerSideProps, | ||
parameterizedRoute: string, | ||
): GetServerSideProps { | ||
return async function ( | ||
...getServerSidePropsArguments: Parameters<GetServerSideProps> | ||
): ReturnType<GetServerSideProps> { | ||
return callDataFetcherTraced(origGetServerSideProps, getServerSidePropsArguments, { | ||
parameterizedRoute, | ||
dataFetchingMethodName: 'getServerSideProps', | ||
}); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,26 @@ | ||
import { GSProps } from './types'; | ||
import { wrapperCore } from './wrapperUtils'; | ||
import { GetStaticProps } from 'next'; | ||
|
||
import { callDataFetcherTraced } from './wrapperUtils'; | ||
|
||
type Props = { [key: string]: unknown }; | ||
|
||
/** | ||
* Create a wrapped version of the user's exported `getStaticProps` function | ||
* | ||
* @param origGetStaticProps: The user's `getStaticProps` function | ||
* @param route: The page's parameterized route | ||
* @param origGetStaticProps The user's `getStaticProps` function | ||
* @param parameterizedRoute The page's parameterized route | ||
* @returns A wrapped version of the function | ||
*/ | ||
export function withSentryGetStaticProps(origGetStaticProps: GSProps['fn'], route: string): GSProps['wrappedFn'] { | ||
return async function (context: GSProps['context']): Promise<GSProps['result']> { | ||
return wrapperCore<GSProps>(origGetStaticProps, context, route); | ||
export function withSentryGetStaticProps( | ||
origGetStaticProps: GetStaticProps<Props>, | ||
parameterizedRoute: string, | ||
): GetStaticProps<Props> { | ||
return async function ( | ||
...getStaticPropsArguments: Parameters<GetStaticProps<Props>> | ||
): ReturnType<GetStaticProps<Props>> { | ||
return callDataFetcherTraced(origGetStaticProps, getStaticPropsArguments, { | ||
parameterizedRoute, | ||
dataFetchingMethodName: 'getStaticProps', | ||
}); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,58 +1,45 @@ | ||
import { captureException } from '@sentry/core'; | ||
import { getActiveTransaction } from '@sentry/tracing'; | ||
import { Span } from '@sentry/types'; | ||
|
||
import { DataFetchingFunction } from './types'; | ||
|
||
/** | ||
* Create a span to track the wrapped function and update transaction name with parameterized route. | ||
* Call a data fetcher and trace it. Only traces the function if there is an active transaction on the scope. | ||
* | ||
* @template T Types for `getInitialProps`, `getStaticProps`, and `getServerSideProps` | ||
* @param origFunction The user's exported `getInitialProps`, `getStaticProps`, or `getServerSideProps` function | ||
* @param context The context object passed by nextjs to the function | ||
* @param route The route currently being served | ||
* @returns The result of calling the user's function | ||
* We only do the following until we move transaction creation into this function: When called, the wrapped function | ||
* will also update the name of the active transaction with a parameterized route provided via the `options` argument. | ||
*/ | ||
export async function wrapperCore<T extends DataFetchingFunction>( | ||
origFunction: T['fn'], | ||
context: T['context'], | ||
route: string, | ||
): Promise<T['result']> { | ||
const transaction = getActiveTransaction(); | ||
|
||
if (transaction) { | ||
// Pull off any leading underscores we've added in the process of wrapping the function | ||
const wrappedFunctionName = origFunction.name.replace(/^_*/, ''); | ||
|
||
// TODO: Make sure that the given route matches the name of the active transaction (to prevent background data | ||
// fetching from switching the name to a completely other route) | ||
transaction.name = route; | ||
transaction.metadata.source = 'route'; | ||
export async function callDataFetcherTraced<F extends (...args: any[]) => Promise<any> | any>( | ||
origFunction: F, | ||
origFunctionArgs: Parameters<F>, | ||
options: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you think about making this the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is blocked on #5564. I have to see what we're gonna do with op and description there before I make any more changes here. Honestly, I wouldn't read too much into this PR. We're likely gonna scrap this wrapper anyways because it's too generic to be used across different data fetching methods. |
||
parameterizedRoute: string; | ||
dataFetchingMethodName: string; | ||
}, | ||
): Promise<ReturnType<F>> { | ||
const { parameterizedRoute, dataFetchingMethodName } = options; | ||
|
||
// Capture the route, since pre-loading, revalidation, etc might mean that this span may happen during another | ||
// route's transaction | ||
const span = transaction.startChild({ op: 'nextjs.data', description: `${wrappedFunctionName} (${route})` }); | ||
|
||
const props = await callOriginal(origFunction, context, span); | ||
|
||
span.finish(); | ||
const transaction = getActiveTransaction(); | ||
|
||
return props; | ||
if (!transaction) { | ||
return origFunction(...origFunctionArgs); | ||
} | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return callOriginal(origFunction, context); | ||
} | ||
// TODO: Make sure that the given route matches the name of the active transaction (to prevent background data | ||
// fetching from switching the name to a completely other route) -- We'll probably switch to creating a transaction | ||
// right here so making that check will probabably not even be necessary. | ||
// Logic will be: If there is no active transaction, start one with correct name and source. If there is an active | ||
// transaction, create a child span with correct name and source. | ||
Comment on lines
+29
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for me to get this right: If we already have a transaction, this means that we started it beforehand in a previous data fetcher function (iirc you said that getInitialProps can e.g. be executed before getServerSideProps), right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now it's started in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, something is weird, because there IS in fact a transaction in our integration tests, otherwise they wouldn't pass. And there's a transaction if you run the server inside of the debugger. But seemingly not if you just run the server straight. It's very mysterious. |
||
transaction.name = parameterizedRoute; | ||
transaction.metadata.source = 'route'; | ||
|
||
// Capture the route, since pre-loading, revalidation, etc might mean that this span may happen during another | ||
// route's transaction | ||
const span = transaction.startChild({ | ||
op: 'nextjs.data', | ||
description: `${dataFetchingMethodName} (${parameterizedRoute})`, | ||
}); | ||
|
||
/** Call the original function, capturing any errors and finishing the span (if any) in case of error */ | ||
async function callOriginal<T extends DataFetchingFunction>( | ||
origFunction: T['fn'], | ||
context: T['context'], | ||
span?: Span, | ||
): Promise<T['result']> { | ||
try { | ||
// eslint-disable-next-line prefer-const, @typescript-eslint/no-explicit-any | ||
return (origFunction as any)(context); | ||
return await origFunction(...origFunctionArgs); | ||
} catch (err) { | ||
if (span) { | ||
span.finish(); | ||
|
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.
Nice chaining approach! I like it! (I wish it'd work as nicely in all our SDKs)
(EDIT, I'm referring to the lines below 115, I just botched the line selection in the review :D)
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.
Props go to Katie.