ref(utils): Stop setting transaction in requestDataIntegration (#…

…14306) This is not really necessary anymore - it only sets this on transaction events, and those get the `transaction` in different places already anyhow. With this, we can also actually remove some other stuff. One method is exported from utils but not otherwise used, we can also drop this in v9. Finally, this was also the only place that used `route` on the request, so we can also get rid of this in `remix`, which is weird anyhow because we set it for errors there but don't even use it for them. --------- Co-authored-by: Luca Forstner <[email protected]>
getsentry · Nov 22, 2024 · be893c0 · be893c0
1 parent d46b2b0
commit be893c0
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 107 deletions.
diff --git a/docs/migration/draft-v9-migration-guide.md b/docs/migration/draft-v9-migration-guide.md
@@ -0,0 +1,13 @@
+<!-- For now this is just a place where we can dump migration guide notes for v9 -->
+
+# Deprecations
+
+## `@sentry/utils`
+
+- Deprecated `AddRequestDataToEventOptions.transaction`. This option effectively doesn't do anything anymore, and will
+  be removed in v9.
+- Deprecated `TransactionNamingScheme` type.
+
+## `@sentry/core`
+
+- Deprecated `transactionNamingScheme` option in `requestDataIntegration`.
diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts
@@ -24,7 +24,11 @@ export type RequestDataIntegrationOptions = {
         };
   };
 
-  /** Whether to identify transactions by parameterized path, parameterized path with method, or handler name */
+  /**
+   * Whether to identify transactions by parameterized path, parameterized path with method, or handler name.
+   * @deprecated This option does not do anything anymore, and will be removed in v9.
+   */
+  // eslint-disable-next-line deprecation/deprecation
   transactionNamingScheme?: TransactionNamingScheme;
 };
 
@@ -110,6 +114,7 @@ function convertReqDataIntegrationOptsToAddReqDataOpts(
   integrationOptions: Required<RequestDataIntegrationOptions>,
 ): AddRequestDataToEventOptions {
   const {
+    // eslint-disable-next-line deprecation/deprecation
     transactionNamingScheme,
     include: { ip, user, ...requestOptions },
   } = integrationOptions;

diff --git a/packages/remix/src/utils/errors.ts b/packages/remix/src/utils/errors.ts
@@ -1,12 +1,5 @@
 import type { AppData, DataFunctionArgs, EntryContext, HandleDocumentRequestFunction } from '@remix-run/node';
-import {
-  captureException,
-  getActiveSpan,
-  getClient,
-  getRootSpan,
-  handleCallbackErrors,
-  spanToJSON,
-} from '@sentry/core';
+import { captureException, getClient, handleCallbackErrors } from '@sentry/core';
 import type { Span } from '@sentry/types';
 import { addExceptionMechanism, isPrimitive, logger, objectify } from '@sentry/utils';
 import { DEBUG_BUILD } from './debug-build';
@@ -61,19 +54,9 @@ export async function captureRemixServerException(
   const objectifiedErr = objectify(err);
 
   captureException(isResponse(objectifiedErr) ? await extractResponseError(objectifiedErr) : objectifiedErr, scope => {
-    const activeSpan = getActiveSpan();
-    const rootSpan = activeSpan && getRootSpan(activeSpan);
-    const activeRootSpanName = rootSpan ? spanToJSON(rootSpan).description : undefined;
-
     scope.setSDKProcessingMetadata({
       request: {
         ...normalizedRequest,
-        // When `route` is not defined, `RequestData` integration uses the full URL
-        route: activeRootSpanName
-          ? {
-              path: activeRootSpanName,
-            }
-          : undefined,
       },
     });
 

diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts
@@ -314,9 +314,6 @@ function wrapRequestHandler(
       isolationScope.setSDKProcessingMetadata({
         request: {
           ...normalizedRequest,
-          route: {
-            path: name,
-          },
         },
       });
 

diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts
@@ -21,7 +21,6 @@ import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress';
 const DEFAULT_INCLUDES = {
   ip: false,
   request: true,
-  transaction: true,
   user: true,
 };
 const DEFAULT_REQUEST_INCLUDES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
@@ -35,6 +34,8 @@ export type AddRequestDataToEventOptions = {
   include?: {
     ip?: boolean;
     request?: boolean | Array<(typeof DEFAULT_REQUEST_INCLUDES)[number]>;
+    /** @deprecated This option will be removed in v9. It does not do anything anymore, the `transcation` is set in other places. */
+    // eslint-disable-next-line deprecation/deprecation
     transaction?: boolean | TransactionNamingScheme;
     user?: boolean | Array<(typeof DEFAULT_USER_INCLUDES)[number]>;
   };
@@ -52,6 +53,9 @@ export type AddRequestDataToEventOptions = {
   };
 };
 
+/**
+ * @deprecated This type will be removed in v9. It is not in use anymore.
+ */
 export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
 
 /**
@@ -67,6 +71,7 @@ export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
  *                used instead of the request's route)
  *
  * @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either 'route' or 'url')
+ * @deprecated This method will be removed in v9. It is not in use anymore.
  */
 export function extractPathForTransaction(
   req: PolymorphicRequest,
@@ -102,23 +107,6 @@ export function extractPathForTransaction(
   return [name, source];
 }
 
-function extractTransaction(req: PolymorphicRequest, type: boolean | TransactionNamingScheme): string {
-  switch (type) {
-    case 'path': {
-      return extractPathForTransaction(req, { path: true })[0];
-    }
-    case 'handler': {
-      return (req.route && req.route.stack && req.route.stack[0] && req.route.stack[0].name) || '<anonymous>';
-    }
-    case 'methodPath':
-    default: {
-      // if exist _reconstructedRoute return that path instead of route.path
-      const customRoute = req._reconstructedRoute ? req._reconstructedRoute : undefined;
-      return extractPathForTransaction(req, { path: true, method: true, customRoute })[0];
-    }
-  }
-}
-
 function extractUserData(
   user: {
     [key: string]: unknown;
@@ -379,12 +367,6 @@ export function addRequestDataToEvent(
     }
   }
 
-  if (include.transaction && !event.transaction && event.type === 'transaction') {
-    // TODO do we even need this anymore?
-    // TODO make this work for nextjs
-    event.transaction = extractTransaction(req, include.transaction);
-  }
-
   return event;
 }
 

diff --git a/packages/utils/test/requestdata.test.ts b/packages/utils/test/requestdata.test.ts
@@ -369,67 +369,6 @@ describe('addRequestDataToEvent', () => {
       }
     });
   });
-
-  describe('transaction property', () => {
-    describe('for transaction events', () => {
-      beforeEach(() => {
-        mockEvent.type = 'transaction';
-      });
-
-      test('extracts method and full route path by default`', () => {
-        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
-
-        expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName');
-      });
-
-      test('extracts method and full path by default when mountpoint is `/`', () => {
-        mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', '');
-        mockReq.baseUrl = '';
-
-        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
-
-        // `subpath/` is the full path here, because there's no router mount path
-        expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName');
-      });
-
-      test('fallback to method and `originalUrl` if route is missing', () => {
-        delete mockReq.route;
-
-        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
-
-        expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue');
-      });
-
-      test('can extract path only instead if configured', () => {
-        const optionsWithPathTransaction = {
-          include: {
-            transaction: 'path',
-          },
-        } as const;
-
-        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithPathTransaction);
-
-        expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName');
-      });
-
-      test('can extract handler name instead if configured', () => {
-        const optionsWithHandlerTransaction = {
-          include: {
-            transaction: 'handler',
-          },
-        } as const;
-
-        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithHandlerTransaction);
-
-        expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler');
-      });
-    });
-    it('transaction is not applied to non-transaction events', () => {
-      const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
-
-      expect(parsedRequest.transaction).toBeUndefined();
-    });
-  });
 });
 
 describe('extractRequestData', () => {
@@ -763,6 +702,7 @@ describe('extractPathForTransaction', () => {
       expectedRoute: string,
       expectedSource: TransactionSource,
     ) => {
+      // eslint-disable-next-line deprecation/deprecation
       const [route, source] = extractPathForTransaction(req, options);
 
       expect(route).toEqual(expectedRoute);
@@ -778,6 +718,7 @@ describe('extractPathForTransaction', () => {
       originalUrl: '/api/users/123/details',
     } as PolymorphicRequest;
 
+    // eslint-disable-next-line deprecation/deprecation
     const [route, source] = extractPathForTransaction(req, {
       path: true,
       method: true,