ref(nextjs): UseRequestData integration for errors

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<F extends (...args: any[]) => 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 {

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

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(() => {

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)

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