ref(node): Improve Express URL Parameterization

packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts

@@ -80,7 +80,7 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
       host: 'somewhere.not.sentry',
       // TraceId changes, hence we only expect that the string contains the traceid key
       baggage: expect.stringContaining(
-        'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=',
+        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public,sentry-trace_id=',
       ),
     },
   });
@@ -99,7 +99,7 @@ test('Should populate Sentry and ignore 3rd party content if sentry-trace header
       host: 'somewhere.not.sentry',
       // TraceId changes, hence we only expect that the string contains the traceid key
       baggage: expect.stringContaining(
-        'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=',
+        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public,sentry-trace_id=',
       ),
     },
   });

packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts

@@ -27,6 +27,7 @@ app.get('/test/express', (_req, res) => {
   const transaction = Sentry.getCurrentHub().getScope()?.getTransaction();
   if (transaction) {
     transaction.traceId = '86f39e84263a4de99c326acab3bfe3bd';
+    transaction.metadata.source = undefined;
   }
   const headers = http.get('http://somewhere.not.sentry/').getHeaders();
 

packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts

@@ -30,7 +30,7 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
     test_data: {
       host: 'somewhere.not.sentry',
       baggage: expect.stringContaining(
-        'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,sentry-public_key=public',
+        'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public',
       ),
     },
   });

packages/node/src/handlers.ts

@@ -40,12 +40,13 @@ export function tracingHandler(): (
 
     const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
 
+    const [name, source] = extractPathForTransaction(req, { path: true, method: true });
     const transaction = startTransaction(
       {
-        name: extractPathForTransaction(req, { path: true, method: true }),
+        name,
         op: 'http.server',
         ...traceparentData,
-        metadata: { baggage },
+        metadata: { baggage, source },
       },
       // extra context passed to the tracesSampler
       { request: extractRequestData(req) },

packages/node/test/requestdata.test.ts

@@ -6,11 +6,12 @@
 
 // TODO (v8 / #5257): Remove everything above
 
-import { Event, User } from '@sentry/types';
+import { Event, TransactionSource, User } from '@sentry/types';
 import {
   addRequestDataToEvent,
   AddRequestDataToEventOptions,
   CrossPlatformRequest,
+  extractPathForTransaction,
   extractRequestData as newExtractRequestData,
 } from '@sentry/utils';
 import * as cookie from 'cookie';
@@ -485,3 +486,95 @@ describe.each([oldExtractRequestData, newExtractRequestData])(
     });
   },
 );
+
+describe('extractPathForTransaction', () => {
+  it.each([
+    [
+      'extracts a parameterized route and method if available',
+      {
+        method: 'get',
+        baseUrl: '/api/users',
+        route: { path: '/:id/details' },
+        originalUrl: '/api/users/123/details',
+      } as CrossPlatformRequest,
+      { path: true, method: true },
+      'GET /api/users/:id/details',
+      'route' as TransactionSource,
+    ],
+    [
+      'ignores the method if specified',
+      {
+        method: 'get',
+        baseUrl: '/api/users',
+        route: { path: '/:id/details' },
+        originalUrl: '/api/users/123/details',
+      } as CrossPlatformRequest,
+      { path: true, method: false },
+      '/api/users/:id/details',
+      'route' as TransactionSource,
+    ],
+    [
+      'ignores the path if specified',
+      {
+        method: 'get',
+        baseUrl: '/api/users',
+        route: { path: '/:id/details' },
+        originalUrl: '/api/users/123/details',
+      } as CrossPlatformRequest,
+      { path: false, method: true },
+      'GET',
+      'route' as TransactionSource,
+    ],
+    [
+      'returns an empty string if everything should be ignored',
+      {
+        method: 'get',
+        baseUrl: '/api/users',
+        route: { path: '/:id/details' },
+        originalUrl: '/api/users/123/details',
+      } as CrossPlatformRequest,
+      { path: false, method: false },
+      '',
+      'route' as TransactionSource,
+    ],
+    [
+      'falls back to the raw URL if no parameterized route is available',
+      {
+        method: 'get',
+        baseUrl: '/api/users',
+        originalUrl: '/api/users/123/details',
+      } as CrossPlatformRequest,
+      { path: true, method: true },
+      'GET /api/users/123/details',
+      'url' as TransactionSource,
+    ],
+  ])(
+    '%s',
+    (
+      _: string,
+      req: CrossPlatformRequest,
+      options: { path?: boolean; method?: boolean },
+      expectedRoute: string,
+      expectedSource: TransactionSource,
+    ) => {
+      const [route, source] = extractPathForTransaction(req, options);
+
+      expect(route).toEqual(expectedRoute);
+      expect(source).toEqual(expectedSource);
+    },
+  );
+
+  it('overrides the requests information with a custom route if specified', () => {
+    const req = {
+      method: 'get',
+      baseUrl: '/api/users',
+      route: { path: '/:id/details' },
+      originalUrl: '/api/users/123/details',
+    } as CrossPlatformRequest;
+
+    const [route, source] = extractPathForTransaction(req, { path: true, method: true }, '/other/path/:id/details');
+
+    expect(route).toEqual('GET /other/path/:id/details');
+    expect(source).toEqual('route');
+  });
+});

packages/tracing/src/integrations/node/express.ts

@@ -1,5 +1,6 @@
+/* eslint-disable max-lines */
 import { Integration, Transaction } from '@sentry/types';
-import { logger } from '@sentry/utils';
+import { CrossPlatformRequest, extractPathForTransaction, logger } from '@sentry/utils';
 
 type Method =
   | 'all'
@@ -32,6 +33,32 @@ type Router = {
   [method in Method]: (...args: any) => any; // eslint-disable-line @typescript-eslint/no-explicit-any
 };
 
+/* Extend the CrossPlatformRequest type with a patched parameter to build a reconstructed route */
+type PatchedRequest = CrossPlatformRequest & { _reconstructedRoute?: string };
+
+/* Type used for pathing the express router prototype */
+type ExpressRouter = Router & {
+  _router?: ExpressRouter;
+  stack?: Layer[];
+  lazyrouter?: () => void;
+  settings?: unknown;
+  process_params: (
+    layer: Layer,
+    called: unknown,
+    req: PatchedRequest,
+    res: ExpressResponse,
+    done: () => void,
+  ) => unknown;
+};
+
+/* Type used for pathing the express router prototype */
+type Layer = {
+  match: (path: string) => boolean;
+  handle_request: (req: PatchedRequest, res: ExpressResponse, next: () => void) => void;
+  route?: { path: string };
+  path?: string;
+};
+
 interface ExpressResponse {
   once(name: string, callback: () => void): void;
 }
@@ -83,6 +110,7 @@ export class Express implements Integration {
       return;
     }
     instrumentMiddlewares(this._router, this._methods);
+    instrumentRouter(this._router as ExpressRouter);
   }
 }
 
@@ -211,3 +239,71 @@ function patchMiddleware(router: Router, method: Method): Router {
 function instrumentMiddlewares(router: Router, methods: Method[] = []): void {
   methods.forEach((method: Method) => patchMiddleware(router, method));
 }
+
+/**
+ * Patches the prototype of Express.Router to accumulate the resolved route
+ * if a layer instance's `match` function was called and it returned a successful match.
+ *
+ * @see https://github.com/expressjs/express/blob/master/lib/router/index.js
+ *
+ * @param appOrRouter the router instance which can either be an app (i.e. top-level) or a (nested) router.
+ */
+function instrumentRouter(appOrRouter: ExpressRouter): void {
+  // This is how we can distinguish between app and routers
+  const isApp = 'settings' in appOrRouter;
+
+  // In case the app's top-level router hasn't been initialized yet, we have to do it now
+  if (isApp && appOrRouter._router === undefined) {
+    appOrRouter.lazyrouter && appOrRouter.lazyrouter();
+  }
+
+  const router = isApp ? appOrRouter._router : appOrRouter;
+  const routerProto = Object.getPrototypeOf(router) as ExpressRouter;
+
+  const originalProcessParams = routerProto.process_params;
+  routerProto.process_params = function process_params(
+    layer: Layer,
+    called: unknown,
+    req: PatchedRequest,
+    res: ExpressResponse & SentryTracingResponse,
+    done: () => unknown,
+  ) {
+    // Base case: We're in the first part of the URL (thus we start with the root '/')
+    if (!req._reconstructedRoute) {
+      req._reconstructedRoute = '/';
+    }
+
+    // If the layers partial route has params, it's stored in layer.route, else it's in layer.path
+    const partialRoute = layer.route?.path || layer.path || '';
+
+    // Normalize the partial route so that it doesn't contain leading or trailing slashes
+    // and exclude empty or '*' wildcard routes.
+    // The exclusion of '*' routes is our best effort of not "polluting" the transaction name
+    // with interim handlers (e.g. that check authentication or do other middleware stuff)
+    // We want to end up with the parameterized URL of the incoming request with nothing in between
+    const finalPartialRoute = partialRoute
+      .split('/')
+      .filter(segment => segment.length > 0 && !segment.includes('*'))
+      .join('/');
+
+    // If we found a valid partial URL, we append it to the reconstructed route
+    if (finalPartialRoute.length > 0) {
+      req._reconstructedRoute += `${finalPartialRoute}/`;
+    }
+
+    // Now we check if we are in the "last" part of the route. We determine this by comparing the
+    // numbers of URL segments from the original URL against our reconstructed parameterized URL.
+    // In case we've reached our final destination, we update the transaction name.
+    const urlLength = req.originalUrl?.split('/').filter(s => s.length > 0).length;
+    const routeLength = req._reconstructedRoute.split('/').filter(s => s.length > 0).length;
+    if (urlLength === routeLength) {
+      const transaction = res.__sentry_transaction;
+      if (transaction && transaction.metadata.source !== 'custom') {
+        const finalRoute = req._reconstructedRoute.replace(/\/$/, '');
+        transaction.setName(...extractPathForTransaction(req, { path: true, method: true }, finalRoute));
+      }
+    }
+
+    return originalProcessParams.call(this, layer, called, req, res, done);
+  };
+}

packages/utils/src/requestdata.ts

@@ -12,7 +12,7 @@
 /* eslint-disable max-lines */
 /* eslint-disable @typescript-eslint/no-explicit-any */
 
-import { Event, ExtractedNodeRequestData, Transaction } from '@sentry/types';
+import { Event, ExtractedNodeRequestData, Transaction, TransactionSource } from '@sentry/types';
 
 import { isPlainObject, isString } from './is';
 import { stripUrlQueryAndFragment } from './misc';
@@ -113,7 +113,9 @@ export function addRequestDataToTransaction(
   deps?: InjectedNodeDeps,
 ): void {
   if (!transaction) return;
-  transaction.name = extractPathForTransaction(req, { path: true, method: true });
+  if (!transaction.metadata.source || transaction.metadata.source === 'url') {
+    transaction.setName(...extractPathForTransaction(req, { path: true, method: true }));
+  }
   transaction.setData('url', req.originalUrl || req.url);
   if (req.baseUrl) {
     transaction.setData('baseUrl', req.baseUrl);
@@ -122,43 +124,52 @@ export function addRequestDataToTransaction(
 }
 
 /**
- * Extracts complete generalized path from the request object and uses it to construct transaction name.
+ * Extracts a complete and parameterized path from the request object and uses it to construct transaction name.
+ * If the parameterized transaction name cannot be extracted, we fall back to the raw URL.
+ *
+ * Additionally, this function determines and returns the transaction name source
  *
  * eg. GET /mountpoint/user/:id
  *
  * @param req A request object
  * @param options What to include in the transaction name (method, path, or both)
+ * @param customRoute An optional string that should be used as transaction name instead of the requests path
  *
- * @returns The fully constructed transaction name
+ * @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either route or url)
  */
 export function extractPathForTransaction(
   req: CrossPlatformRequest,
   options: { path?: boolean; method?: boolean } = {},
-): string {
+  customRoute?: string,
+): [string, TransactionSource] {
   const method = req.method && req.method.toUpperCase();
 
   let path = '';
+  let source: TransactionSource = 'url';
+
   // Check to see if there's a parameterized route we can use (as there is in Express)
-  if (req.route) {
-    path = `${req.baseUrl || ''}${req.route.path}`;
+  if (customRoute || req.route) {
+    path = customRoute || `${req.baseUrl || ''}${req.route && req.route.path}`;
+    source = 'route';
   }
+
   // Otherwise, just take the original URL
   else if (req.originalUrl || req.url) {
     path = stripUrlQueryAndFragment(req.originalUrl || req.url || '');
   }
 
-  let info = '';
+  let name = '';
   if (options.method && method) {
-    info += method;
+    name += method;
   }
   if (options.method && options.path) {
-    info += ' ';
+    name += ' ';
   }
   if (options.path && path) {
-    info += path;
+    name += path;
   }
 
-  return info;
+  return [name, source];
 }
 
 type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
@@ -167,14 +178,14 @@ type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
 function extractTransaction(req: CrossPlatformRequest, type: boolean | TransactionNamingScheme): string {
   switch (type) {
     case 'path': {
-      return extractPathForTransaction(req, { path: true });
+      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: {
-      return extractPathForTransaction(req, { path: true, method: true });
+      return extractPathForTransaction(req, { path: true, method: true })[0];
     }
   }
 }