Express integration span name change and path unification (#3078)

* ref: Change express span name to midddleware.method

* ref: Change types to express Router instead of Application

* fix: Use correct paths for express transactions and unify express integration

Author: kamilogorek

packages/node/src/handlers.ts

@@ -2,7 +2,7 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core';
 import { extractTraceparentData, Span } from '@sentry/tracing';
-import { Event } from '@sentry/types';
+import { Event, Transaction } from '@sentry/types';
 import {
   extractNodeRequestData,
   forget,
@@ -21,6 +21,16 @@ import { flush } from './sdk';
 
 const DEFAULT_SHUTDOWN_TIMEOUT = 2000;
 
+interface ExpressRequest {
+  route?: {
+    path: string;
+  };
+  method: string;
+  originalUrl: string;
+  baseUrl: string;
+  query: string;
+}
+
 /**
  * Express-compatible tracing handler.
  * @see Exposed as `Handlers.tracingHandler`
@@ -65,6 +75,7 @@ export function tracingHandler(): (
     res.once('finish', () => {
       // We schedule the immediate execution of the `finish` to let all the spans being closed first.
       setImmediate(() => {
+        addExpressReqToTransaction(transaction, (req as unknown) as ExpressRequest);
         transaction.setHttpStatus(res.statusCode);
         transaction.finish();
       });
@@ -74,6 +85,20 @@ export function tracingHandler(): (
   };
 }
 
+/**
+ * Set parameterized as transaction name e.g.: `GET /users/:id`
+ * Also adds more context data on the transaction from the request
+ */
+function addExpressReqToTransaction(transaction: Transaction | undefined, req: ExpressRequest): void {
+  if (!transaction) return;
+  if (req.route) {
+    transaction.name = `${req.method} ${req.baseUrl}${req.route.path}`;
+  }
+  transaction.setData('url', req.originalUrl);
+  transaction.setData('baseUrl', req.baseUrl);
+  transaction.setData('query', req.query);
+}
+
 type TransactionTypes = 'path' | 'methodPath' | 'handler';
 
 /** JSDoc */

packages/tracing/src/integrations/express.ts

@@ -1,8 +1,6 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
 import { Integration, Transaction } from '@sentry/types';
 import { logger } from '@sentry/utils';
 
-// Have to manually set types because we are using package-alias
 type Method =
   | 'all'
   | 'get'
@@ -27,17 +25,14 @@ type Method =
   | 'subscribe'
   | 'trace'
   | 'unlock'
-  | 'unsubscribe';
+  | 'unsubscribe'
+  | 'use';
 
-type Application = {
-  [method in Method | 'use']: (...args: any) => any;
+type Router = {
+  [method in Method]: (...args: unknown[]) => unknown;
 };
 
-type ErrorRequestHandler = (...args: any) => any;
-type RequestHandler = (...args: any) => any;
-type NextFunction = (...args: any) => any;
-
-interface Response {
+interface ExpressResponse {
   once(name: string, callback: () => void): void;
 }
 
@@ -52,8 +47,7 @@ interface SentryTracingResponse {
 /**
  * Express integration
  *
- * Provides an request and error handler for Express framework
- * as well as tracing capabilities
+ * Provides an request and error handler for Express framework as well as tracing capabilities
  */
 export class Express implements Integration {
   /**
@@ -69,27 +63,26 @@ export class Express implements Integration {
   /**
    * Express App instance
    */
-  private readonly _app?: Application;
+  private readonly _router?: Router;
   private readonly _methods?: Method[];
 
   /**
    * @inheritDoc
    */
-  public constructor(options: { app?: Application; methods?: Method[] } = {}) {
-    this._app = options.app;
-    this._methods = options.methods;
+  public constructor(options: { app?: Router; router?: Router; methods?: Method[] } = {}) {
+    this._router = options.router || options.app;
+    this._methods = (Array.isArray(options.methods) ? options.methods : []).concat('use');
   }
 
   /**
    * @inheritDoc
    */
   public setupOnce(): void {
-    if (!this._app) {
+    if (!this._router) {
       logger.error('ExpressIntegration is missing an Express instance');
       return;
     }
-    instrumentMiddlewares(this._app);
-    routeMiddlewares(this._app, this._methods);
+    instrumentMiddlewares(this._router, this._methods);
   }
 }
 
@@ -104,75 +97,63 @@ export class Express implements Integration {
  * app.use(function (req, res, next) { ... })
  * // error handler
  * app.use(function (err, req, res, next) { ... })
+ *
+ * They all internally delegate to the `router[method]` of the given application instance.
  */
-// eslint-disable-next-line @typescript-eslint/ban-types
-function wrap(fn: Function): RequestHandler | ErrorRequestHandler {
+// eslint-disable-next-line @typescript-eslint/ban-types, @typescript-eslint/no-explicit-any
+function wrap(fn: Function, method: Method): (...args: any[]) => void {
   const arity = fn.length;
 
   switch (arity) {
     case 2: {
-      return function(this: NodeJS.Global, req: Request, res: Response & SentryTracingResponse): any {
+      return function(this: NodeJS.Global, req: unknown, res: ExpressResponse & SentryTracingResponse): void {
         const transaction = res.__sentry_transaction;
-        addExpressReqToTransaction(transaction, req);
         if (transaction) {
           const span = transaction.startChild({
             description: fn.name,
-            op: 'middleware',
+            op: `middleware.${method}`,
           });
           res.once('finish', () => {
             span.finish();
           });
         }
-        // eslint-disable-next-line prefer-rest-params
-        return fn.apply(this, arguments);
+        return fn.call(this, req, res);
       };
     }
     case 3: {
       return function(
         this: NodeJS.Global,
-        req: Request,
-        res: Response & SentryTracingResponse,
-        next: NextFunction,
-      ): any {
+        req: unknown,
+        res: ExpressResponse & SentryTracingResponse,
+        next: () => void,
+      ): void {
         const transaction = res.__sentry_transaction;
-        addExpressReqToTransaction(transaction, req);
-        const span =
-          transaction &&
-          transaction.startChild({
-            description: fn.name,
-            op: 'middleware',
-          });
-        fn.call(this, req, res, function(this: NodeJS.Global): any {
-          if (span) {
-            span.finish();
-          }
-          // eslint-disable-next-line prefer-rest-params
-          return next.apply(this, arguments);
+        const span = transaction?.startChild({
+          description: fn.name,
+          op: `middleware.${method}`,
+        });
+        fn.call(this, req, res, function(this: NodeJS.Global, ...args: unknown[]): void {
+          span?.finish();
+          next.call(this, ...args);
         });
       };
     }
     case 4: {
       return function(
         this: NodeJS.Global,
-        err: any,
+        err: Error,
         req: Request,
         res: Response & SentryTracingResponse,
-        next: NextFunction,
-      ): any {
+        next: () => void,
+      ): void {
         const transaction = res.__sentry_transaction;
-        addExpressReqToTransaction(transaction, req);
-        const span =
-          transaction &&
-          transaction.startChild({
-            description: fn.name,
-            op: 'middleware',
-          });
-        fn.call(this, err, req, res, function(this: NodeJS.Global): any {
-          if (span) {
-            span.finish();
-          }
-          // eslint-disable-next-line prefer-rest-params
-          return next.apply(this, arguments);
+        const span = transaction?.startChild({
+          description: fn.name,
+          op: `middleware.${method}`,
+        });
+        fn.call(this, err, req, res, function(this: NodeJS.Global, ...args: unknown[]): void {
+          span?.finish();
+          next.call(this, ...args);
         });
       };
     }
@@ -183,24 +164,7 @@ function wrap(fn: Function): RequestHandler | ErrorRequestHandler {
 }
 
 /**
- * Set parameterized as transaction name e.g.: `GET /users/:id`
- * Also adds more context data on the transaction from the request
- */
-function addExpressReqToTransaction(transaction: Transaction | undefined, req: any): void {
-  /* eslint-disable @typescript-eslint/no-unsafe-member-access */
-  if (transaction) {
-    if (req.route && req.route.path) {
-      transaction.name = `${req.method} ${req.route.path}`;
-    }
-    transaction.setData('url', req.originalUrl);
-    transaction.setData('baseUrl', req.baseUrl);
-    transaction.setData('query', req.query);
-  }
-  /* eslint-enable @typescript-eslint/no-unsafe-member-access */
-}
-
-/**
- * Takes all the function arguments passed to the original `app.use` call
+ * Takes all the function arguments passed to the original `app` or `router` method, eg. `app.use` or `router.use`
  * and wraps every function, as well as array of functions with a call to our `wrap` method.
  * We have to take care of the arrays as well as iterate over all of the arguments,
  * as `app.use` can accept middlewares in few various forms.
@@ -209,16 +173,16 @@ function addExpressReqToTransaction(transaction: Transaction | undefined, req: a
  * app.use([<path>], <fn>, ...<fn>)
  * app.use([<path>], ...<fn>[])
  */
-function wrapUseArgs(args: IArguments): unknown[] {
-  return Array.from(args).map((arg: unknown) => {
+function wrapMiddlewareArgs(args: unknown[], method: Method): unknown[] {
+  return args.map((arg: unknown) => {
     if (typeof arg === 'function') {
-      return wrap(arg);
+      return wrap(arg, method);
     }
 
     if (Array.isArray(arg)) {
       return arg.map((a: unknown) => {
         if (typeof a === 'function') {
-          return wrap(a);
+          return wrap(a, method);
         }
         return a;
       });
@@ -229,31 +193,21 @@ function wrapUseArgs(args: IArguments): unknown[] {
 }
 
 /**
- * Patches original App to utilize our tracing functionality
+ * Patches original router to utilize our tracing functionality
  */
-function patchMiddleware(app: Application, method: Method | 'use'): Application {
-  const originalAppCallback = app[method];
+function patchMiddleware(router: Router, method: Method): Router {
+  const originalCallback = router[method];
 
-  app[method] = function(): any {
-    // eslint-disable-next-line prefer-rest-params
-    return originalAppCallback.apply(this, wrapUseArgs(arguments));
+  router[method] = function(...args: unknown[]): void {
+    return originalCallback.call(this, ...wrapMiddlewareArgs(args, method));
   };
 
-  return app;
+  return router;
 }
 
 /**
- * Patches original app.use
+ * Patches original router methods
  */
-function instrumentMiddlewares(app: Application): void {
-  patchMiddleware(app, 'use');
-}
-
-/**
- * Patches original app.METHOD
- */
-function routeMiddlewares(app: Application, methods: Method[] = []): void {
-  methods.forEach(function(method: Method) {
-    patchMiddleware(app, method);
-  });
+function instrumentMiddlewares(router: Router, methods: Method[] = []): void {
+  methods.forEach((method: Method) => patchMiddleware(router, method));
 }