From 8c1333797e74b8bb473f1101fad503ab279e8f10 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 12 Sep 2022 20:18:59 -0700 Subject: [PATCH 1/9] overwrite `transaction` value after running `addRequestDataToEvent` --- packages/node/src/integrations/requestdata.ts | 55 +++++++++++++++++-- 1 file changed, 51 insertions(+), 4 deletions(-) diff --git a/packages/node/src/integrations/requestdata.ts b/packages/node/src/integrations/requestdata.ts index e5ef0b6ff989..472caf88fd43 100644 --- a/packages/node/src/integrations/requestdata.ts +++ b/packages/node/src/integrations/requestdata.ts @@ -1,7 +1,8 @@ // TODO (v8 or v9): Whenever this becomes a default integration for `@sentry/browser`, move this to `@sentry/core`. For // now, we leave it in `@sentry/integrations` so that it doesn't contribute bytes to our CDN bundles. -import { EventProcessor, Hub, Integration } from '@sentry/types'; +import { EventProcessor, Hub, Integration, Transaction } from '@sentry/types'; +import { extractPathForTransaction } from '@sentry/utils'; import { addRequestDataToEvent, @@ -86,10 +87,15 @@ export class RequestData implements Integration { * @inheritDoc */ public setupOnce(addGlobalEventProcessor: (eventProcessor: EventProcessor) => void, getCurrentHub: () => Hub): void { - const { include, addRequestData } = this._options; + // Note: In the long run, most of the logic here should probably move into the request data utility functions. For + // the moment it lives here, though, until https://github.com/getsentry/sentry-javascript/issues/5718 is addressed. + // (TL;DR: Those functions touch many parts of the repo in many different ways, and need to be clened up. Once + // that's happened, it will be easier to add this logic in without worrying about unexpected side effects.) + const { include, addRequestData, transactionNamingScheme } = this._options; addGlobalEventProcessor(event => { - const self = getCurrentHub().getIntegration(RequestData); + const hub = getCurrentHub(); + const self = hub.getIntegration(RequestData); const req = event.sdkProcessingMetadata && event.sdkProcessingMetadata.request; // If the globally installed instance of this integration isn't associated with the current hub, `self` will be @@ -98,7 +104,36 @@ export class RequestData implements Integration { return event; } - return addRequestData(event, req, { include: formatIncludeOption(include) }); + const processedEvent = addRequestData(event, req, { include: formatIncludeOption(include) }); + + // Transaction events already have the right `transaction` value + if (event.type === 'transaction' || transactionNamingScheme === 'handler') { + return processedEvent; + } + + // In all other cases, use the request's associated transaction (if any) to overwrite the event's `transaction` + // value with a high-quality one + const reqWithTransaction = req as { _sentryTransaction?: Transaction }; + const transaction = reqWithTransaction._sentryTransaction; + if (transaction) { + // TODO (v8): Remove the nextjs check and just base it on `transactionNamingScheme` for all SDKs. (We have to + // keep it the way it is for the moment, because changing the names of transactions in Sentry has the potential + // to break things like alert rules.) + const shouldIncludeMethodInTransactionName = + getSDKName(hub) === 'sentry.javascript.nextjs' + ? transaction.name.startsWith('/api') + : transactionNamingScheme !== 'path'; + + const [transactionValue] = extractPathForTransaction(req, { + path: true, + method: shouldIncludeMethodInTransactionName, + customRoute: transaction.name, + }); + + processedEvent.transaction = transactionValue; + } + + return processedEvent; }); } } @@ -123,3 +158,15 @@ function formatIncludeOption( request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined, }; } + +function getSDKName(hub: Hub): string | undefined { + try { + // For a long chain like this, it's fewer bytes to combine a try-catch with assuming everything is there than to + // write out a long chain of `a && a.b && a.b.c && ...` + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + return hub.getClient()!.getOptions()!._metadata!.sdk!.name; + } catch (err) { + // In theory we should never get here + return undefined; + } +} From b0d3b94e7c470867816ca60c35fa4ac55801ba77 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 8 Sep 2022 23:24:54 -0700 Subject: [PATCH 2/9] add request to scope metadata in data fetchers wrapper --- packages/nextjs/src/config/wrappers/wrapperUtils.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index e5aef5a6ff21..ab99500caad1 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -123,6 +123,7 @@ export function callTracedServerSideDataFetcher Pr const currentScope = getCurrentHub().getScope(); if (currentScope) { currentScope.setSpan(dataFetcherSpan); + currentScope.setSDKProcessingMetadata({ request: req }); currentScope.addEventProcessor(event => event.type !== 'transaction' ? addRequestDataToEvent(event, req, { From c803491a2893f4799049b5ea79ac8640ece33b33 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 12 Sep 2022 19:47:31 -0700 Subject: [PATCH 3/9] add request to scope metadata in `instrumentServer` --- packages/nextjs/src/utils/instrumentServer.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 80c24b008388..755e96f73378 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -247,6 +247,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { currentScope.addEventProcessor(event => event.type !== 'transaction' ? addRequestDataToEvent(event, nextReq) : event, ); + currentScope.setSDKProcessingMetadata({ request: req }); // We only want to record page and API requests if (hasTracingEnabled() && shouldTraceRequest(nextReq.url, publicDirFiles)) { From b9a2461898a0e47c468d8ad248cfc9a81ef1a4bb Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 8 Sep 2022 23:33:37 -0700 Subject: [PATCH 4/9] add request to scope metadata in `_error` helper --- packages/nextjs/src/utils/_error.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nextjs/src/utils/_error.ts b/packages/nextjs/src/utils/_error.ts index ea10c0181be3..14436228ec0a 100644 --- a/packages/nextjs/src/utils/_error.ts +++ b/packages/nextjs/src/utils/_error.ts @@ -56,6 +56,7 @@ export async function captureUnderscoreErrorException(contextOrProps: ContextOrP if (req) { scope.addEventProcessor(event => addRequestDataToEvent(event, req)); + scope.setSDKProcessingMetadata({ request: req }); } // If third-party libraries (or users themselves) throw something falsy, we want to capture it as a message (which From 3f6835f572f5abb7e2e287f10be30c74a7c13eb1 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 9 Sep 2022 00:01:49 -0700 Subject: [PATCH 5/9] add request to scope metadata in `withSentry` --- packages/nextjs/src/utils/withSentry.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 6158008f6921..bed34bfa9d4c 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -59,6 +59,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = currentScope.addEventProcessor(event => event.type !== 'transaction' ? addRequestDataToEvent(event, req) : event, ); + currentScope.setSDKProcessingMetadata({ request: req }); if (hasTracingEnabled()) { // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision) From 520b4509aea44323665171f331a33f2c2f6a8e4a Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 8 Sep 2022 22:17:13 -0700 Subject: [PATCH 6/9] stop using event processor in datafetchers wrapper --- packages/nextjs/src/config/wrappers/wrapperUtils.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index ab99500caad1..cf7e56d8f5f6 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -1,5 +1,4 @@ import { captureException, getCurrentHub, startTransaction } from '@sentry/core'; -import { addRequestDataToEvent } from '@sentry/node'; import { getActiveTransaction } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; import { baggageHeaderToDynamicSamplingContext, extractTraceparentData, fill } from '@sentry/utils'; @@ -124,18 +123,6 @@ export function callTracedServerSideDataFetcher Pr if (currentScope) { currentScope.setSpan(dataFetcherSpan); currentScope.setSDKProcessingMetadata({ request: req }); - currentScope.addEventProcessor(event => - event.type !== 'transaction' - ? addRequestDataToEvent(event, req, { - include: { - // When the `transaction` option is set to true, it tries to extract a transaction name from the request - // object. We don't want this since we already have a high-quality transaction name with a parameterized - // route. Setting `transaction` to `true` will clobber that transaction name. - transaction: false, - }, - }) - : event, - ); } try { From db88154b164bd6611994414b0655be3dc65d64ad Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 12 Sep 2022 19:45:06 -0700 Subject: [PATCH 7/9] stop using event processor in `instrumentServer` --- packages/nextjs/src/utils/instrumentServer.ts | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 755e96f73378..d92ed3e14ae7 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,12 +1,5 @@ /* eslint-disable max-lines */ -import { - addRequestDataToEvent, - captureException, - configureScope, - deepReadDirSync, - getCurrentHub, - startTransaction, -} from '@sentry/node'; +import { captureException, configureScope, deepReadDirSync, getCurrentHub, startTransaction } from '@sentry/node'; import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { addExceptionMechanism, @@ -244,9 +237,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => - event.type !== 'transaction' ? addRequestDataToEvent(event, nextReq) : event, - ); + // Store the request on the scope so we can pull data from it and add it to the event currentScope.setSDKProcessingMetadata({ request: req }); // We only want to record page and API requests @@ -298,10 +289,6 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { if (transaction) { transaction.setHttpStatus(res.statusCode); - // we'll collect this data in a more targeted way in the event processor we added above, - // `addRequestDataToEvent` - delete transaction.metadata.requestPath; - // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the // transaction closes setImmediate(() => { From b6d22fe5eb7612cff5ad581542cb9cae174c96b7 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 8 Sep 2022 23:33:58 -0700 Subject: [PATCH 8/9] stop using event processor in `_error` helper --- packages/nextjs/src/utils/_error.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/nextjs/src/utils/_error.ts b/packages/nextjs/src/utils/_error.ts index 14436228ec0a..9871bb277071 100644 --- a/packages/nextjs/src/utils/_error.ts +++ b/packages/nextjs/src/utils/_error.ts @@ -1,6 +1,6 @@ import { captureException, withScope } from '@sentry/core'; import { getCurrentHub } from '@sentry/hub'; -import { addExceptionMechanism, addRequestDataToEvent } from '@sentry/utils'; +import { addExceptionMechanism } from '@sentry/utils'; import { NextPageContext } from 'next'; type ContextOrProps = { @@ -55,7 +55,6 @@ export async function captureUnderscoreErrorException(contextOrProps: ContextOrP }); if (req) { - scope.addEventProcessor(event => addRequestDataToEvent(event, req)); scope.setSDKProcessingMetadata({ request: req }); } From 19c4fb156ac33fe6c38dd803729936e3f80d0dc7 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 12 Sep 2022 19:33:43 -0700 Subject: [PATCH 9/9] stop using event processor in `withSentry` --- packages/nextjs/src/utils/withSentry.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index bed34bfa9d4c..65cadd78ed89 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -1,4 +1,4 @@ -import { addRequestDataToEvent, captureException, flush, getCurrentHub, startTransaction } from '@sentry/node'; +import { captureException, flush, getCurrentHub, startTransaction } from '@sentry/node'; import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; import { @@ -56,9 +56,6 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => - event.type !== 'transaction' ? addRequestDataToEvent(event, req) : event, - ); currentScope.setSDKProcessingMetadata({ request: req }); if (hasTracingEnabled()) {