From e861cc436d32edd87a7235d7f119896680c70adf Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 16 Aug 2022 16:00:12 -0700 Subject: [PATCH] feat(nextjs): Add spans and route parameterization in data fetching wrappers (#5564) This adds span creation and route parameterization to the experimental `getStaticProps` and `getServerSideProps` wrappers. In order to do the parameterization, we store the parameterized route( which is the same as the filepath of the page file) as a global variable in the code we add using the `dataFetchersLoader` and then pass it to the wrappers, which then replace the name of the active transaction with the parameterized route we've stored. It also adds basic error-capturing to the wrappers. Open issues/questions (not solved by this PR): - Because of prefetching, revalidation of static pages, and the like, the data fetching functions can run in the background, during handling of a route different from their own. (For example, if page A has a nextjs `Link` component pointing to page B, next will prefetch the data for B (and therefore run its data fetching functions) while handling the request for A. We need to make sure that we don't accidentally overwrite the transaction name in that case to B. - There's more we should do in terms of error handling. Specifically, we should port much of the logic in `withSentry` into these wrappers also. - Porting the error-handling logic will mean we need to deal with the domain question - how to keep reactivating the correct domain as we go into and out of data fetching and rendering functions. (There are other items listed in the meta issue linked below, but the above are the ones directly relevant to the work in this PR.) Ref: https://github.com/getsentry/sentry-javascript/issues/5505 --- .../src/config/loaders/dataFetchersLoader.ts | 17 +++++- .../templates/dataFetchersLoaderTemplate.ts | 7 ++- packages/nextjs/src/config/webpack.ts | 9 +-- .../wrappers/withSentryGetServerSideProps.ts | 7 ++- .../wrappers/withSentryGetStaticProps.ts | 7 ++- .../src/config/wrappers/wrapperUtils.ts | 60 +++++++++++++++---- 6 files changed, 84 insertions(+), 23 deletions(-) diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 9f42e86ab6e9..0d4c0d3becb6 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -37,6 +37,7 @@ const DATA_FETCHING_FUNCTIONS = { type LoaderOptions = { projectDir: string; + pagesDir: string; }; /** @@ -108,7 +109,7 @@ 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 } = 'getOptions' in this ? this.getOptions() : this.query; + const { projectDir, pagesDir } = 'getOptions' in this ? this.getOptions() : this.query; // 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 @@ -172,6 +173,20 @@ 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); 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/templates/dataFetchersLoaderTemplate.ts b/packages/nextjs/src/config/templates/dataFetchersLoaderTemplate.ts index 78788d6826ac..4f9984f3f136 100644 --- a/packages/nextjs/src/config/templates/dataFetchersLoaderTemplate.ts +++ b/packages/nextjs/src/config/templates/dataFetchersLoaderTemplate.ts @@ -17,11 +17,14 @@ declare const __ORIG_GSPROPS__: GetStaticPropsFunction; // eslint-disable-next-line import/no-extraneous-dependencies, import/no-unresolved import * as ServerSideSentryNextjsSDK from '@sentry/nextjs'; +const PARAMETERIZED_ROUTE = '__FILEPATH__'; + export const getServerSideProps = typeof __ORIG_GSSP__ === 'function' - ? ServerSideSentryNextjsSDK.withSentryGetServerSideProps(__ORIG_GSSP__) + ? ServerSideSentryNextjsSDK.withSentryGetServerSideProps(__ORIG_GSSP__, PARAMETERIZED_ROUTE) : __ORIG_GSSP__; + export const getStaticProps = typeof __ORIG_GSPROPS__ === 'function' - ? ServerSideSentryNextjsSDK.withSentryGetStaticProps(__ORIG_GSPROPS__) + ? ServerSideSentryNextjsSDK.withSentryGetStaticProps(__ORIG_GSPROPS__, PARAMETERIZED_ROUTE) : __ORIG_GSPROPS__; diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index a636609a9a7b..93b79a1c7313 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -56,8 +56,6 @@ export function constructWebpackConfigFunction( newConfig = userNextConfig.webpack(newConfig, buildContext); } - const pageRegex = new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages(/.+)\\.(jsx?|tsx?)`); - if (isServer) { newConfig.module = { ...newConfig.module, @@ -81,12 +79,15 @@ export function constructWebpackConfigFunction( }; if (userSentryOptions.experiments?.autoWrapDataFetchers) { + const pagesDir = newConfig.resolve?.alias?.['private-next-pages'] as string; + newConfig.module.rules.push({ - test: pageRegex, + // Nextjs allows the `pages` folder to optionally live inside a `src` folder + test: new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages/.*\\.(jsx?|tsx?)`), use: [ { loader: path.resolve(__dirname, 'loaders/dataFetchersLoader.js'), - options: { projectDir }, + options: { projectDir, pagesDir }, }, ], }); diff --git a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts index 0293ad999c45..23e7050fdac0 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts @@ -1,14 +1,15 @@ import { GSSP } from './types'; -import { callOriginal } from './wrapperUtils'; +import { wrapperCore } 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 * @returns A wrapped version of the function */ -export function withSentryGetServerSideProps(origGetServerSideProps: GSSP['fn']): GSSP['wrappedFn'] { +export function withSentryGetServerSideProps(origGetServerSideProps: GSSP['fn'], route: string): GSSP['wrappedFn'] { return async function (context: GSSP['context']): Promise { - return callOriginal(origGetServerSideProps, context); + return wrapperCore(origGetServerSideProps, context, route); }; } diff --git a/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts index 23d6023a25bf..00c286b1b500 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts @@ -1,14 +1,15 @@ import { GSProps } from './types'; -import { callOriginal } from './wrapperUtils'; +import { wrapperCore } from './wrapperUtils'; /** * 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 * @returns A wrapped version of the function */ -export function withSentryGetStaticProps(origGetStaticProps: GSProps['fn']): GSProps['wrappedFn'] { +export function withSentryGetStaticProps(origGetStaticProps: GSProps['fn'], route: string): GSProps['wrappedFn'] { return async function (context: GSProps['context']): Promise { - return callOriginal(origGetStaticProps, context); + return wrapperCore(origGetStaticProps, context, route); }; } diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index d7a466cb8221..feca9cf4adb7 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -1,25 +1,65 @@ +import { captureException } from '@sentry/core'; +import { getActiveTransaction } from '@sentry/tracing'; +import { Span } from '@sentry/types'; + import { DataFetchingFunction } from './types'; /** - * Pass-through wrapper for the original function, used as a first step in eventually wrapping the data-fetching - * functions with code for tracing. + * Create a span to track the wrapped function and update transaction name with parameterized route. * * @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 */ -export async function callOriginal( +export async function wrapperCore( origFunction: T['fn'], context: T['context'], + route: string, ): Promise { - let props; + 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); + + span.finish(); - // 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 - props = await (origFunction as any)(context); + return props; + } + + // 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) { + span.finish(); + } - return props; + // TODO Copy more robust error handling over from `withSentry` + captureException(err); + throw err; + } }