ref(nextjs): UseRequestData integration for errors (#5729)

lobsterkatie · web-flow · commit f9d3139255f2 · 2022-09-21T06:38:46.000-07:00
In #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 #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.
diff --git 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<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 {
diff --git 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
@@ -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
@@ -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
@@ -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;
+  }
+}
