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-bad-tx-name/server.ts

@@ -0,0 +1,40 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import cors from 'cors';
+import express from 'express';
+import http from 'http';
+
+const app = express();
+
+export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  environment: 'prod',
+  integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
+  tracesSampleRate: 1.0,
+});
+
+Sentry.setUser({ id: 'user123', segment: 'SegmentA' });
+
+app.use(Sentry.Handlers.requestHandler());
+app.use(Sentry.Handlers.tracingHandler());
+
+app.use(cors());
+
+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();
+
+  // Responding with the headers outgoing request headers back to the assertions.
+  res.send({ test_data: headers });
+});
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;

packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/test.ts

@@ -0,0 +1,19 @@
+import * as path from 'path';
+
+import { getAPIResponse, runServer } from '../../../../utils/index';
+import { TestAPIResponse } from '../server';
+
+test('Does not include transaction name if transaction source is not set', async () => {
+  const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
+
+  const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;
+  const baggageString = response.test_data.baggage;
+
+  expect(response).toBeDefined();
+  expect(response).toMatchObject({
+    test_data: {
+      host: 'somewhere.not.sentry',
+    },
+  });
+  expect(baggageString).not.toContain('sentry-transaction=');
+});

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

@@ -13,24 +13,8 @@ test('should attach a `baggage` header to an outgoing request.', async () => {
     test_data: {
       host: 'somewhere.not.sentry',
       baggage:
-        'sentry-environment=prod,sentry-release=1.0,sentry-user_segment=SegmentA' +
+        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-user_segment=SegmentA' +
         ',sentry-public_key=public,sentry-trace_id=86f39e84263a4de99c326acab3bfe3bd,sentry-sample_rate=1',
     },
   });
 });
-
-test('Does not include transaction name if transaction source is not set', async () => {
-  const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
-
-  const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;
-  const baggageString = response.test_data.baggage;
-
-  expect(response).toBeDefined();
-  expect(response).toMatchObject({
-    test_data: {
-      host: 'somewhere.not.sentry',
-    },
-  });
-  expect(baggageString).not.toContain('sentry-user_id=');
-  expect(baggageString).not.toContain('sentry-transaction=');
-});

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,99 @@ 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,
+      customRoute: '/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,75 @@ 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 layer's partial route has params, the route is stored in layer.route. Otherwise, the hardcoded path
+    // (i.e. a partial route without params) is stored 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 to not "pollute" the transaction name
+    // with interim handlers (e.g. ones that check authentication or do other middleware stuff).
+    // We want to end up with the parameterized URL of the incoming request without any extraneous path segments.
+    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
+    // number of URL segments from the original URL to that of our reconstructed parameterized URL.
+    // If 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') {
+        // If the request URL is '/' or empty, the reconstructed route will be empty.
+        // Therefore, we fall back to setting the final route to '/' in this case.
+        const finalRoute = req._reconstructedRoute || '/';
+
+        transaction.setName(...extractPathForTransaction(req, { path: true, method: true, customRoute: finalRoute }));
+      }
+    }
+
+    return originalProcessParams.call(this, layer, called, req, res, done);
+  };
+}