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; + } +}