From f9d3139255f26738e735d61ab0ebc8cb34fda9c2 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 21 Sep 2022 06:38:46 -0700 Subject: [PATCH] ref(nextjs): Use`RequestData` integration for errors (#5729) In https://github.com/getsentry/sentry-javascript/pull/5703, a new integration, `RequestData`, was added to take the place of the request-specific event processors we've been using to add request data to transaction events in the nextjs SDK. This builds on that work by making the same switch for error events. Notes: - Because of https://github.com/getsentry/sentry-javascript/issues/5718, it's hard to reason about the potential side effects of making major changes to the logic in `@sentry/utils/requestData`. Therefore, the majority of the new logic in this PR has been added to the integration, and just overwrites the `transaction` value added by the functions in `requestData`. Once we've cleaned up the request data code, we can consolidate the logic. --- .../src/config/wrappers/wrapperUtils.ts | 14 +---- packages/nextjs/src/utils/_error.ts | 4 +- packages/nextjs/src/utils/instrumentServer.ts | 18 +----- packages/nextjs/src/utils/withSentry.ts | 6 +- packages/node/src/integrations/requestdata.ts | 55 +++++++++++++++++-- 5 files changed, 59 insertions(+), 38 deletions(-) diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index e5aef5a6ff21..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'; @@ -123,18 +122,7 @@ export function callTracedServerSideDataFetcher Pr const currentScope = getCurrentHub().getScope(); if (currentScope) { currentScope.setSpan(dataFetcherSpan); - 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, - ); + currentScope.setSDKProcessingMetadata({ request: req }); } try { diff --git a/packages/nextjs/src/utils/_error.ts b/packages/nextjs/src/utils/_error.ts index ea10c0181be3..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,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 diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 80c24b008388..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,8 @@ 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 if (hasTracingEnabled() && shouldTraceRequest(nextReq.url, publicDirFiles)) { @@ -297,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(() => { diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 6158008f6921..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,7 @@ 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()) { // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision) 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; + } +}