From 2ab7218739ad51b9e205639663a871e1b29c2696 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 | 44 +++++++--- 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 | 82 +++++++++++-------- 7 files changed, 123 insertions(+), 99 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 a1a75a5d8268..af34c69dae06 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; pageRegex: RegExp; + underscoreAppRegex: RegExp; + underscoreErrorRegex: RegExp; + underscoreDocumentRegex: RegExp; }; /** @@ -109,7 +112,11 @@ 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, pageRegex } = 'getOptions' in this ? this.getOptions() : this.query; + const { projectDir, pageRegex, underscoreAppRegex, underscoreDocumentRegex, underscoreErrorRegex } = + 'getOptions' in this ? this.getOptions() : this.query; + + // Get the parameterized route name from this page's filepath + let parameterizedRouteName = this.resourcePath.match(pageRegex)?.[2]; // 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 +143,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; @@ -171,9 +191,7 @@ export default function wrapDataFetchersLoader(this: LoaderThis, path.relative(projectDir, this.resourcePath), ); - // Get the route name from this page's filepath - let route = this.resourcePath.match(pageRegex)?.[2]; - if (!route) { + if (!parameterizedRouteName) { logger.warn(`Unable to wrap code from page ${this.resourcePath}, because the route regex has no matches.`); return userCode; } @@ -185,8 +203,8 @@ export default function wrapDataFetchersLoader(this: LoaderThis, } // Any route ending in '/index' will correspond to the root of that directory, '/'. - route = route.replace(/index$/, ''); - injectedCode = injectedCode.replace('__FILEPATH__', route); + parameterizedRouteName = parameterizedRouteName.replace(/index$/, ''); + injectedCode = injectedCode.replace('__FILEPATH__', parameterizedRouteName); return `${modifiedUserCode}\n${injectedCode}`; } diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 109f37653d0c..15b041a875d9 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -90,13 +90,25 @@ export function constructWebpackConfigFunction( // by the `/.*` const pageRegex = new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages(/.*)(/?index)?\\.(jsx?|tsx?)`); + 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) { newConfig.module.rules.push({ test: pageRegex, use: [ { loader: path.resolve(__dirname, 'loaders/dataFetchersLoader.js'), - options: { projectDir, pageRegex }, + options: { + projectDir, + pageRegex, + 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 4e0bd2f69698..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({ origFunction: origGetServerSideProps, context, route, op: 'getServerSideProps' }); +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 154d514f936a..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({ origFunction: origGetStaticProps, context, route, op: 'getStaticProps' }); +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 38121ed37b8a..cd61b7fd135b 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -1,46 +1,58 @@ import { getActiveTransaction } from '@sentry/tracing'; -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 - * @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(options: { - origFunction: T['fn']; - context: T['context']; - route: string; - op: string; -}): Promise { - const { origFunction, context, route, op } = options; +export function callDataFetcherTraced Promise | any>( + origFunction: F, + origFunctionArgs: Parameters, + options: { + route: string; + op: string; + }, +): ReturnType { + const { route, op } = options; const transaction = getActiveTransaction(); - if (transaction) { - // 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, data: { route } }); - - // TODO: Can't figure out how to tell TS that the types are correlated - that a `GSPropsFunction` will only get passed - // `GSPropsContext` and never, say, `GSSPContext`. That's what wrapping everything in objects and using the generic - // and pulling the types from the generic rather than specifying them directly was supposed to do, but... no luck. - // eslint-disable-next-line prefer-const, @typescript-eslint/no-explicit-any - const props = await (origFunction as any)(context); - - span.finish(); - - return props; + if (!transaction) { + return origFunction(...origFunctionArgs); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return (origFunction as any)(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. + // 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, data: { 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; + }, + ); + + return result; }