From 2fa11eb54f13b305dbee35687539748614583d6e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 16 Dec 2022 14:54:10 +0100 Subject: [PATCH] fix(nextjs): Pass `this` through wrappers (#6572) --- .../wrappers/withSentryGetServerSideProps.ts | 32 ++--- .../withSentryServerSideAppGetInitialProps.ts | 24 ++-- ...SentryServerSideDocumentGetInitialProps.ts | 15 ++- ...ithSentryServerSideErrorGetInitialProps.ts | 22 +-- .../withSentryServerSideGetInitialProps.ts | 22 +-- .../src/config/wrappers/wrapperUtils.ts | 125 +++++++++--------- .../test/integration/pages/_document.tsx | 18 +++ 7 files changed, 138 insertions(+), 120 deletions(-) create mode 100644 packages/nextjs/test/integration/pages/_document.tsx diff --git a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts index 700d1899d56b..8aa2244ea90e 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts @@ -3,7 +3,7 @@ import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; import { GetServerSideProps } from 'next'; import { isBuild } from '../../utils/isBuild'; -import { callTracedServerSideDataFetcher, getTransactionFromRequest, withErrorInstrumentation } from './wrapperUtils'; +import { getTransactionFromRequest, withErrorInstrumentation, withTracedServerSideDataFetcher } from './wrapperUtils'; /** * Create a wrapped version of the user's exported `getServerSideProps` function @@ -16,30 +16,26 @@ export function withSentryGetServerSideProps( origGetServerSideProps: GetServerSideProps, parameterizedRoute: string, ): GetServerSideProps { - return async function ( - ...getServerSidePropsArguments: Parameters - ): ReturnType { + return async function (this: unknown, ...args: Parameters): ReturnType { if (isBuild()) { - return origGetServerSideProps(...getServerSidePropsArguments); + return origGetServerSideProps.apply(this, args); } - const [context] = getServerSidePropsArguments; + const [context] = args; const { req, res } = context; const errorWrappedGetServerSideProps = withErrorInstrumentation(origGetServerSideProps); if (hasTracingEnabled()) { - const serverSideProps = await callTracedServerSideDataFetcher( - errorWrappedGetServerSideProps, - getServerSidePropsArguments, - req, - res, - { - dataFetcherRouteName: parameterizedRoute, - requestedRouteName: parameterizedRoute, - dataFetchingMethodName: 'getServerSideProps', - }, - ); + const tracedGetServerSideProps = withTracedServerSideDataFetcher(errorWrappedGetServerSideProps, req, res, { + dataFetcherRouteName: parameterizedRoute, + requestedRouteName: parameterizedRoute, + dataFetchingMethodName: 'getServerSideProps', + }); + + const serverSideProps = await (tracedGetServerSideProps.apply(this, args) as ReturnType< + typeof tracedGetServerSideProps + >); if ('props' in serverSideProps) { const requestTransaction = getTransactionFromRequest(req); @@ -53,7 +49,7 @@ export function withSentryGetServerSideProps( return serverSideProps; } else { - return errorWrappedGetServerSideProps(...getServerSidePropsArguments); + return errorWrappedGetServerSideProps.apply(this, args); } }; } diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts index 87c7addbfd58..a136bbcdc96a 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts @@ -3,7 +3,7 @@ import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; import App from 'next/app'; import { isBuild } from '../../utils/isBuild'; -import { callTracedServerSideDataFetcher, getTransactionFromRequest, withErrorInstrumentation } from './wrapperUtils'; +import { getTransactionFromRequest, withErrorInstrumentation, withTracedServerSideDataFetcher } from './wrapperUtils'; type AppGetInitialProps = typeof App['getInitialProps']; @@ -16,14 +16,12 @@ type AppGetInitialProps = typeof App['getInitialProps']; * @returns A wrapped version of the function */ export function withSentryServerSideAppGetInitialProps(origAppGetInitialProps: AppGetInitialProps): AppGetInitialProps { - return async function ( - ...appGetInitialPropsArguments: Parameters - ): ReturnType { + return async function (this: unknown, ...args: Parameters): ReturnType { if (isBuild()) { - return origAppGetInitialProps(...appGetInitialPropsArguments); + return origAppGetInitialProps.apply(this, args); } - const [context] = appGetInitialPropsArguments; + const [context] = args; const { req, res } = context.ctx; const errorWrappedAppGetInitialProps = withErrorInstrumentation(origAppGetInitialProps); @@ -33,16 +31,18 @@ export function withSentryServerSideAppGetInitialProps(origAppGetInitialProps: A // This does not seem to be the case in dev mode. Because we have no clean way of associating the the data fetcher // span with each other when there are no req or res objects, we simply do not trace them at all here. if (hasTracingEnabled() && req && res) { + const tracedGetInitialProps = withTracedServerSideDataFetcher(errorWrappedAppGetInitialProps, req, res, { + dataFetcherRouteName: '/_app', + requestedRouteName: context.ctx.pathname, + dataFetchingMethodName: 'getInitialProps', + }); + const appGetInitialProps: { pageProps: { _sentryTraceData?: string; _sentryBaggage?: string; }; - } = await callTracedServerSideDataFetcher(errorWrappedAppGetInitialProps, appGetInitialPropsArguments, req, res, { - dataFetcherRouteName: '/_app', - requestedRouteName: context.ctx.pathname, - dataFetchingMethodName: 'getInitialProps', - }); + } = await tracedGetInitialProps.apply(this, args); const requestTransaction = getTransactionFromRequest(req); @@ -64,7 +64,7 @@ export function withSentryServerSideAppGetInitialProps(origAppGetInitialProps: A return appGetInitialProps; } else { - return errorWrappedAppGetInitialProps(...appGetInitialPropsArguments); + return errorWrappedAppGetInitialProps.apply(this, args); } }; } diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideDocumentGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideDocumentGetInitialProps.ts index 31c16ec247a7..a04aa11a3398 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideDocumentGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideDocumentGetInitialProps.ts @@ -2,7 +2,7 @@ import { hasTracingEnabled } from '@sentry/tracing'; import Document from 'next/document'; import { isBuild } from '../../utils/isBuild'; -import { callTracedServerSideDataFetcher, withErrorInstrumentation } from './wrapperUtils'; +import { withErrorInstrumentation, withTracedServerSideDataFetcher } from './wrapperUtils'; type DocumentGetInitialProps = typeof Document.getInitialProps; @@ -18,13 +18,14 @@ export function withSentryServerSideDocumentGetInitialProps( origDocumentGetInitialProps: DocumentGetInitialProps, ): DocumentGetInitialProps { return async function ( - ...documentGetInitialPropsArguments: Parameters + this: unknown, + ...args: Parameters ): ReturnType { if (isBuild()) { - return origDocumentGetInitialProps(...documentGetInitialPropsArguments); + return origDocumentGetInitialProps.apply(this, args); } - const [context] = documentGetInitialPropsArguments; + const [context] = args; const { req, res } = context; const errorWrappedGetInitialProps = withErrorInstrumentation(origDocumentGetInitialProps); @@ -34,13 +35,15 @@ export function withSentryServerSideDocumentGetInitialProps( // This does not seem to be the case in dev mode. Because we have no clean way of associating the the data fetcher // span with each other when there are no req or res objects, we simply do not trace them at all here. if (hasTracingEnabled() && req && res) { - return callTracedServerSideDataFetcher(errorWrappedGetInitialProps, documentGetInitialPropsArguments, req, res, { + const tracedGetInitialProps = withTracedServerSideDataFetcher(errorWrappedGetInitialProps, req, res, { dataFetcherRouteName: '/_document', requestedRouteName: context.pathname, dataFetchingMethodName: 'getInitialProps', }); + + return await tracedGetInitialProps.apply(this, args); } else { - return errorWrappedGetInitialProps(...documentGetInitialPropsArguments); + return errorWrappedGetInitialProps.apply(this, args); } }; } diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts index 7b4be0cde29d..8ceae817e1bc 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts @@ -4,7 +4,7 @@ import { NextPageContext } from 'next'; import { ErrorProps } from 'next/error'; import { isBuild } from '../../utils/isBuild'; -import { callTracedServerSideDataFetcher, getTransactionFromRequest, withErrorInstrumentation } from './wrapperUtils'; +import { getTransactionFromRequest, withErrorInstrumentation, withTracedServerSideDataFetcher } from './wrapperUtils'; type ErrorGetInitialProps = (context: NextPageContext) => Promise; @@ -19,14 +19,12 @@ type ErrorGetInitialProps = (context: NextPageContext) => Promise; export function withSentryServerSideErrorGetInitialProps( origErrorGetInitialProps: ErrorGetInitialProps, ): ErrorGetInitialProps { - return async function ( - ...errorGetInitialPropsArguments: Parameters - ): ReturnType { + return async function (this: unknown, ...args: Parameters): ReturnType { if (isBuild()) { - return origErrorGetInitialProps(...errorGetInitialPropsArguments); + return origErrorGetInitialProps.apply(this, args); } - const [context] = errorGetInitialPropsArguments; + const [context] = args; const { req, res } = context; const errorWrappedGetInitialProps = withErrorInstrumentation(origErrorGetInitialProps); @@ -36,15 +34,17 @@ export function withSentryServerSideErrorGetInitialProps( // This does not seem to be the case in dev mode. Because we have no clean way of associating the the data fetcher // span with each other when there are no req or res objects, we simply do not trace them at all here. if (hasTracingEnabled() && req && res) { - const errorGetInitialProps: ErrorProps & { - _sentryTraceData?: string; - _sentryBaggage?: string; - } = await callTracedServerSideDataFetcher(errorWrappedGetInitialProps, errorGetInitialPropsArguments, req, res, { + const tracedGetInitialProps = withTracedServerSideDataFetcher(errorWrappedGetInitialProps, req, res, { dataFetcherRouteName: '/_error', requestedRouteName: context.pathname, dataFetchingMethodName: 'getInitialProps', }); + const errorGetInitialProps: ErrorProps & { + _sentryTraceData?: string; + _sentryBaggage?: string; + } = await tracedGetInitialProps.apply(this, args); + const requestTransaction = getTransactionFromRequest(req); if (requestTransaction) { errorGetInitialProps._sentryTraceData = requestTransaction.toTraceparent(); @@ -55,7 +55,7 @@ export function withSentryServerSideErrorGetInitialProps( return errorGetInitialProps; } else { - return errorWrappedGetInitialProps(...errorGetInitialPropsArguments); + return errorWrappedGetInitialProps.apply(this, args); } }; } diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts index 8f8a0b4b8608..06d0f7766fec 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts @@ -3,7 +3,7 @@ import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; import { NextPage } from 'next'; import { isBuild } from '../../utils/isBuild'; -import { callTracedServerSideDataFetcher, getTransactionFromRequest, withErrorInstrumentation } from './wrapperUtils'; +import { getTransactionFromRequest, withErrorInstrumentation, withTracedServerSideDataFetcher } from './wrapperUtils'; type GetInitialProps = Required['getInitialProps']; @@ -15,14 +15,12 @@ type GetInitialProps = Required['getInitialProps']; * @returns A wrapped version of the function */ export function withSentryServerSideGetInitialProps(origGetInitialProps: GetInitialProps): GetInitialProps { - return async function ( - ...getInitialPropsArguments: Parameters - ): Promise> { + return async function (this: unknown, ...args: Parameters): Promise> { if (isBuild()) { - return origGetInitialProps(...getInitialPropsArguments); + return origGetInitialProps.apply(this, args); } - const [context] = getInitialPropsArguments; + const [context] = args; const { req, res } = context; const errorWrappedGetInitialProps = withErrorInstrumentation(origGetInitialProps); @@ -32,15 +30,17 @@ export function withSentryServerSideGetInitialProps(origGetInitialProps: GetInit // This does not seem to be the case in dev mode. Because we have no clean way of associating the the data fetcher // span with each other when there are no req or res objects, we simply do not trace them at all here. if (hasTracingEnabled() && req && res) { - const initialProps: { - _sentryTraceData?: string; - _sentryBaggage?: string; - } = await callTracedServerSideDataFetcher(errorWrappedGetInitialProps, getInitialPropsArguments, req, res, { + const tracedGetInitialProps = withTracedServerSideDataFetcher(errorWrappedGetInitialProps, req, res, { dataFetcherRouteName: context.pathname, requestedRouteName: context.pathname, dataFetchingMethodName: 'getInitialProps', }); + const initialProps: { + _sentryTraceData?: string; + _sentryBaggage?: string; + } = await tracedGetInitialProps.apply(this, args); + const requestTransaction = getTransactionFromRequest(req); if (requestTransaction) { initialProps._sentryTraceData = requestTransaction.toTraceparent(); @@ -51,7 +51,7 @@ export function withSentryServerSideGetInitialProps(origGetInitialProps: GetInit return initialProps; } else { - return errorWrappedGetInitialProps(...getInitialPropsArguments); + return errorWrappedGetInitialProps.apply(this, args); } }; } diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index a38ce595296e..0a56143102bf 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -38,7 +38,7 @@ export function withErrorInstrumentation any>( ): (...params: Parameters) => Promise> { return async function (this: unknown, ...origFunctionArguments: Parameters): Promise> { try { - return await origFunction.call(this, ...origFunctionArguments); + return await origFunction.apply(this, origFunctionArguments); } catch (e) { // TODO: Extract error logic from `withSentry` in here or create a new wrapper with said logic or something like that. captureException(e); @@ -54,16 +54,15 @@ export function withErrorInstrumentation any>( * * 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 origDataFetcher 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 Promise | any>( - origFunction: F, - origFunctionArguments: Parameters, +export function withTracedServerSideDataFetcher Promise | any>( + origDataFetcher: F, req: IncomingMessage, res: ServerResponse, options: { @@ -74,70 +73,72 @@ export function callTracedServerSideDataFetcher Pr /** Name of the data fetching method - will be used for describing the data fetcher's span. */ dataFetchingMethodName: string; }, -): Promise> { - return domain.create().bind(async () => { - let requestTransaction: Transaction | undefined = getTransactionFromRequest(req); - - if (requestTransaction === undefined) { - const sentryTraceHeader = req.headers['sentry-trace']; - const rawBaggageString = req.headers && req.headers.baggage; - const traceparentData = - typeof sentryTraceHeader === 'string' ? extractTraceparentData(sentryTraceHeader) : undefined; - - const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(rawBaggageString); - - const newTransaction = startTransaction( - { - op: 'http.server', - name: options.requestedRouteName, - ...traceparentData, - status: 'ok', - metadata: { - source: 'route', - dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, +): (...params: Parameters) => Promise> { + return async function (this: unknown, ...args: Parameters): Promise> { + return domain.create().bind(async () => { + let requestTransaction: Transaction | undefined = getTransactionFromRequest(req); + + if (requestTransaction === undefined) { + const sentryTraceHeader = req.headers['sentry-trace']; + const rawBaggageString = req.headers && req.headers.baggage; + const traceparentData = + typeof sentryTraceHeader === 'string' ? extractTraceparentData(sentryTraceHeader) : undefined; + + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(rawBaggageString); + + const newTransaction = startTransaction( + { + op: 'http.server', + name: options.requestedRouteName, + ...traceparentData, + status: 'ok', + metadata: { + source: 'route', + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + }, }, - }, - { request: req }, - ); + { request: req }, + ); - requestTransaction = newTransaction; - autoEndTransactionOnResponseEnd(newTransaction, res); + requestTransaction = newTransaction; + autoEndTransactionOnResponseEnd(newTransaction, res); - // Link the transaction and the request together, so that when we would normally only have access to one, it's - // still possible to grab the other. - setTransactionOnRequest(newTransaction, req); - newTransaction.setMetadata({ request: req }); - } - - const dataFetcherSpan = requestTransaction.startChild({ - op: 'function.nextjs', - description: `${options.dataFetchingMethodName} (${options.dataFetcherRouteName})`, - status: 'ok', - }); - - const currentScope = getCurrentHub().getScope(); - if (currentScope) { - currentScope.setSpan(dataFetcherSpan); - currentScope.setSDKProcessingMetadata({ request: req }); - } + // Link the transaction and the request together, so that when we would normally only have access to one, it's + // still possible to grab the other. + setTransactionOnRequest(newTransaction, req); + newTransaction.setMetadata({ request: req }); + } - try { - return await origFunction(...origFunctionArguments); - } catch (e) { - // Since we finish the span before the error can bubble up and trigger the handlers in `registerErrorInstrumentation` - // that set the transaction status, we need to manually set the status of the span & transaction - dataFetcherSpan.setStatus('internal_error'); + const dataFetcherSpan = requestTransaction.startChild({ + op: 'function.nextjs', + description: `${options.dataFetchingMethodName} (${options.dataFetcherRouteName})`, + status: 'ok', + }); - const transaction = dataFetcherSpan.transaction; - if (transaction) { - transaction.setStatus('internal_error'); + const currentScope = getCurrentHub().getScope(); + if (currentScope) { + currentScope.setSpan(dataFetcherSpan); + currentScope.setSDKProcessingMetadata({ request: req }); } - throw e; - } finally { - dataFetcherSpan.finish(); - } - })(); + try { + return await origDataFetcher.apply(this, args); + } catch (e) { + // Since we finish the span before the error can bubble up and trigger the handlers in `registerErrorInstrumentation` + // that set the transaction status, we need to manually set the status of the span & transaction + dataFetcherSpan.setStatus('internal_error'); + + const transaction = dataFetcherSpan.transaction; + if (transaction) { + transaction.setStatus('internal_error'); + } + + throw e; + } finally { + dataFetcherSpan.finish(); + } + })(); + }; } /** diff --git a/packages/nextjs/test/integration/pages/_document.tsx b/packages/nextjs/test/integration/pages/_document.tsx new file mode 100644 index 000000000000..9479e30f712b --- /dev/null +++ b/packages/nextjs/test/integration/pages/_document.tsx @@ -0,0 +1,18 @@ +import Document, { DocumentContext } from 'next/document'; + +class MyDocument extends Document { + static async getInitialProps(ctx: DocumentContext) { + // Verify that wrapping correctly passes `this` + this.testFunction(); + + const initialProps = await Document.getInitialProps(ctx); + + return initialProps; + } + + static testFunction() { + // noop + } +} + +export default MyDocument;