From 927746c02e1472533c48d5742e05be1bc3e92031 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 16 Aug 2022 13:34:15 +0000 Subject: [PATCH] feat(nextjs): Create spans in serverside `getInitialProps` --- .../src/config/loaders/dataFetchersLoader.ts | 64 +++++++---- packages/nextjs/src/config/webpack.ts | 14 ++- packages/nextjs/src/config/wrappers/types.ts | 35 ------ .../wrappers/withSentryGetInitialProps.ts | 14 ++- .../wrappers/withSentryGetServerSideProps.ts | 17 ++- .../wrappers/withSentryGetStaticProps.ts | 16 ++- .../src/config/wrappers/wrapperUtils.ts | 100 +++++++++--------- 7 files changed, 136 insertions(+), 124 deletions(-) delete mode 100644 packages/nextjs/src/config/wrappers/types.ts diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 0d4c0d3becb6..71cc6d789858 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -38,6 +38,9 @@ const DATA_FETCHING_FUNCTIONS = { type LoaderOptions = { projectDir: string; pagesDir: string; + underscoreAppRegex: RegExp; + underscoreErrorRegex: RegExp; + underscoreDocumentRegex: RegExp; }; /** @@ -109,7 +112,23 @@ export default function wrapDataFetchersLoader(this: LoaderThis, } // We know one or the other will be defined, depending on the version of webpack being used - const { projectDir, pagesDir } = 'getOptions' in this ? this.getOptions() : this.query; + const { projectDir, pagesDir, underscoreAppRegex, underscoreDocumentRegex, underscoreErrorRegex } = + 'getOptions' in this ? this.getOptions() : this.query; + + // Get the parameterized route name from this page's filepath + const parameterizedRouteName = path + // Get the path of the file insde of the pages directory + .relative(pagesDir, this.resourcePath) + // Add a slash at the beginning + .replace(/(.*)/, '/$1') + // Pull off the file extension + .replace(/\.(jsx?|tsx?)/, '') + // Any page file named `index` corresponds to root of the directory its in, URL-wise, so turn `/xyz/index` into + // just `/xyz` + .replace(/\/index$/, '') + // In case all of the above have left us with an empty string (which will happen if we're dealing with the + // homepage), sub back in the root route + .replace(/^$/, '/'); // In the following branch we will proxy the user's file. This means we return code (basically an entirely new file) // that re - exports all the user file's originial export, but with a "sentry-proxy-loader" query in the module @@ -136,13 +155,26 @@ export default function wrapDataFetchersLoader(this: LoaderThis, if (hasDefaultExport(ast)) { outputFileContent += ` import { default as _sentry_default } from "${this.resourcePath}?sentry-proxy-loader"; - import { withSentryGetInitialProps } from "@sentry/nextjs"; - - if (typeof _sentry_default.getInitialProps === 'function') { - _sentry_default.getInitialProps = withSentryGetInitialProps(_sentry_default.getInitialProps); - } - - export default _sentry_default;`; + import { withSentryGetInitialProps } from "@sentry/nextjs";`; + + if (this.resourcePath.match(underscoreAppRegex)) { + // getInitialProps signature is a bit different in _app.js so we need a different wrapper + // Currently a no-op + } else if (this.resourcePath.match(underscoreErrorRegex)) { + // getInitialProps behaviour is a bit different in _error.js so we probably want different wrapper + // Currently a no-op + } else if (this.resourcePath.match(underscoreDocumentRegex)) { + // getInitialProps signature is a bit different in _document.js so we need a different wrapper + // Currently a no-op + } else { + // We enter this branch for any "normal" Next.js page + outputFileContent += ` + if (typeof _sentry_default.getInitialProps === 'function') { + _sentry_default.getInitialProps = withSentryGetInitialProps(_sentry_default.getInitialProps, '${parameterizedRouteName}'); + }`; + } + + outputFileContent += 'export default _sentry_default;'; } return outputFileContent; @@ -173,20 +205,8 @@ export default function wrapDataFetchersLoader(this: LoaderThis, // Fill in template placeholders let injectedCode = modifiedTemplateCode; - const route = path - // Get the path of the file insde of the pages directory - .relative(pagesDir, this.resourcePath) - // Add a slash at the beginning - .replace(/(.*)/, '/$1') - // Pull off the file extension - .replace(/\.(jsx?|tsx?)/, '') - // Any page file named `index` corresponds to root of the directory its in, URL-wise, so turn `/xyz/index` into - // just `/xyz` - .replace(/\/index$/, '') - // In case all of the above have left us with an empty string (which will happen if we're dealing with the - // homepage), sub back in the root route - .replace(/^$/, '/'); - injectedCode = injectedCode.replace('__FILEPATH__', route); + + injectedCode = injectedCode.replace('__FILEPATH__', parameterizedRouteName); for (const { placeholder, alias } of Object.values(DATA_FETCHING_FUNCTIONS)) { injectedCode = injectedCode.replace(new RegExp(placeholder, 'g'), alias); } diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 93b79a1c7313..7e576a9ce85b 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -78,6 +78,12 @@ export function constructWebpackConfigFunction( ], }; + const underscoreAppRegex = new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages/_app\\.(jsx?|tsx?)`); + const underscoreErrorRegex = new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages/_error\\.(jsx?|tsx?)`); + const underscoreDocumentRegex = new RegExp( + `${escapeStringForRegex(projectDir)}(/src)?/pages/_document\\.(jsx?|tsx?)`, + ); + if (userSentryOptions.experiments?.autoWrapDataFetchers) { const pagesDir = newConfig.resolve?.alias?.['private-next-pages'] as string; @@ -87,7 +93,13 @@ export function constructWebpackConfigFunction( use: [ { loader: path.resolve(__dirname, 'loaders/dataFetchersLoader.js'), - options: { projectDir, pagesDir }, + options: { + projectDir, + pagesDir, + underscoreAppRegex, + underscoreErrorRegex, + underscoreDocumentRegex, + }, }, ], }); diff --git a/packages/nextjs/src/config/wrappers/types.ts b/packages/nextjs/src/config/wrappers/types.ts deleted file mode 100644 index 85b5502703ca..000000000000 --- a/packages/nextjs/src/config/wrappers/types.ts +++ /dev/null @@ -1,35 +0,0 @@ -import type { - GetServerSideProps, - GetServerSidePropsContext, - GetServerSidePropsResult, - GetStaticProps, - GetStaticPropsContext, - GetStaticPropsResult, - NextPage, - NextPageContext, -} from 'next'; - -type Props = { [key: string]: unknown }; - -export type GSProps = { - fn: GetStaticProps; - wrappedFn: GetStaticProps; - context: GetStaticPropsContext; - result: GetStaticPropsResult; -}; - -export type GSSP = { - fn: GetServerSideProps; - wrappedFn: GetServerSideProps; - context: GetServerSidePropsContext; - result: GetServerSidePropsResult; -}; - -export type GIProps = { - fn: Required['getInitialProps']; - wrappedFn: NextPage['getInitialProps']; - context: NextPageContext; - result: unknown; -}; - -export type DataFetchingFunction = GSProps | GSSP | GIProps; diff --git a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts index 7e5e209ae98a..046b8c32aa4f 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts @@ -1,14 +1,18 @@ -import { GIProps } from './types'; +import { NextPage } from 'next'; + +import { callDataFetcherTraced } from './wrapperUtils'; + +type GetInitialProps = Required>['getInitialProps']; /** * Create a wrapped version of the user's exported `getInitialProps` function * - * @param origGIProps: The user's `getInitialProps` function + * @param origGetInitialProps: The user's `getInitialProps` function * @param origGIPropsHost: The user's object on which `getInitialProps` lives (used for `this`) * @returns A wrapped version of the function */ -export function withSentryGetInitialProps(origGIProps: GIProps['fn']): GIProps['wrappedFn'] { - return async function (this: unknown, ...args: Parameters) { - return await origGIProps.call(this, ...args); +export function withSentryGetInitialProps(origGetInitialProps: GetInitialProps, route: string): GetInitialProps { + return function (...getInitialPropsArguments: Parameters): ReturnType { + return callDataFetcherTraced(origGetInitialProps, getInitialPropsArguments, { route, op: 'getInitialProps' }); }; } diff --git a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts index 23e7050fdac0..0c5ea439b964 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts @@ -1,5 +1,6 @@ -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 @@ -8,8 +9,14 @@ import { wrapperCore } from './wrapperUtils'; * @param route: 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 { - return wrapperCore(origGetServerSideProps, context, route); +export function withSentryGetServerSideProps( + origGetServerSideProps: GetServerSideProps, + route: string, +): GetServerSideProps { + return function (...getServerSidePropsArguments: Parameters): ReturnType { + return callDataFetcherTraced(origGetServerSideProps, getServerSidePropsArguments, { + route, + op: 'getServerSideProps', + }); }; } diff --git a/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts index 00c286b1b500..bf391921d1c2 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts @@ -1,5 +1,8 @@ -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 @@ -8,8 +11,11 @@ import { wrapperCore } from './wrapperUtils'; * @param route: 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 { - return wrapperCore(origGetStaticProps, context, route); +export function withSentryGetStaticProps( + origGetStaticProps: GetStaticProps, + route: string, +): GetStaticProps { + return function (...getStaticPropsArguments: Parameters>): ReturnType> { + return callDataFetcherTraced(origGetStaticProps, getStaticPropsArguments, { route, op: 'getStaticProps' }); }; } diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index feca9cf4adb7..f586124e3c98 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -2,64 +2,62 @@ 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( - origFunction: T['fn'], - context: T['context'], - route: string, -): Promise { - 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'; - - // 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); +export function callDataFetcherTraced Promise | any>( + origFunction: F, + origFunctionArgs: Parameters, + options: { + route: string; + op: string; + }, +): ReturnType { + const { route, op } = options; - 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); -} - -/** Call the original function, capturing any errors and finishing the span (if any) in case of error */ -async function callOriginal( - origFunction: T['fn'], - context: T['context'], - span?: Span, -): Promise { - try { - // eslint-disable-next-line prefer-const, @typescript-eslint/no-explicit-any - return (origFunction as any)(context); - } catch (err) { - if (span) { + // 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) -- 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. + // We will probably need to put + transaction.name = route; + 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: `${wrappedFunctionName} (${route})` }); + + const result = origFunction(...origFunctionArgs); + + // We do the following instead of `await`-ing the return value of `origFunction`, because that would require us to + // make this function async which might in turn create a mismatch of function signatures between the original + // function and the wrapped one. + // This wraps `result`, which is potentially a Promise, into a Promise. + // If `result` is a non-Promise, the callback of `then` is immediately called and the span is finished. + // If `result` is a Promise, the callback of `then` is only called when `result` resolves + void Promise.resolve(result).then( + () => { span.finish(); - } + }, + err => { + // TODO: Can we somehow associate the error with the span? + span.finish(); + throw err; + }, + ); - // TODO Copy more robust error handling over from `withSentry` - captureException(err); - throw err; - } + return result; }