From cbe07dc810b62483f7109074b24c9a2992be39a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Thu, 19 Nov 2020 12:54:23 +0100 Subject: [PATCH 1/3] ref: Change express span name to midddleware.method --- packages/tracing/src/integrations/express.ts | 82 ++++++++++---------- 1 file changed, 40 insertions(+), 42 deletions(-) diff --git a/packages/tracing/src/integrations/express.ts b/packages/tracing/src/integrations/express.ts index b9f6f4beeb75..bbc4f9d5e41a 100644 --- a/packages/tracing/src/integrations/express.ts +++ b/packages/tracing/src/integrations/express.ts @@ -27,10 +27,11 @@ type Method = | 'subscribe' | 'trace' | 'unlock' - | 'unsubscribe'; + | 'unsubscribe' + | 'use'; type Application = { - [method in Method | 'use']: (...args: any) => any; + [method in Method]: (...args: any) => any; }; type ErrorRequestHandler = (...args: any) => any; @@ -41,6 +42,16 @@ interface Response { once(name: string, callback: () => void): void; } +interface Request { + route?: { + path: string; + }; + method: string; + originalUrl: string; + baseUrl: string; + query: string; +} + /** * Internal helper for `__sentry_transaction` * @hidden @@ -77,7 +88,7 @@ export class Express implements Integration { */ public constructor(options: { app?: Application; methods?: Method[] } = {}) { this._app = options.app; - this._methods = options.methods; + this._methods = (Array.isArray(options.methods) ? options.methods : []).concat('use'); } /** @@ -88,8 +99,7 @@ export class Express implements Integration { logger.error('ExpressIntegration is missing an Express instance'); return; } - instrumentMiddlewares(this._app); - routeMiddlewares(this._app, this._methods); + instrumentMiddlewares(this._app, this._methods); } } @@ -106,7 +116,7 @@ export class Express implements Integration { * app.use(function (err, req, res, next) { ... }) */ // eslint-disable-next-line @typescript-eslint/ban-types -function wrap(fn: Function): RequestHandler | ErrorRequestHandler { +function wrap(fn: Function, method: Method): RequestHandler | ErrorRequestHandler { const arity = fn.length; switch (arity) { @@ -117,7 +127,7 @@ function wrap(fn: Function): RequestHandler | ErrorRequestHandler { if (transaction) { const span = transaction.startChild({ description: fn.name, - op: 'middleware', + op: `middleware.${method}`, }); res.once('finish', () => { span.finish(); @@ -140,7 +150,7 @@ function wrap(fn: Function): RequestHandler | ErrorRequestHandler { transaction && transaction.startChild({ description: fn.name, - op: 'middleware', + op: `middleware.${method}`, }); fn.call(this, req, res, function(this: NodeJS.Global): any { if (span) { @@ -165,7 +175,7 @@ function wrap(fn: Function): RequestHandler | ErrorRequestHandler { transaction && transaction.startChild({ description: fn.name, - op: 'middleware', + op: `middleware.${method}`, }); fn.call(this, err, req, res, function(this: NodeJS.Global): any { if (span) { @@ -182,23 +192,6 @@ 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 * and wraps every function, as well as array of functions with a call to our `wrap` method. @@ -209,16 +202,16 @@ function addExpressReqToTransaction(transaction: Transaction | undefined, req: a * app.use([], , ...) * app.use([], ...[]) */ -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; }); @@ -231,29 +224,34 @@ function wrapUseArgs(args: IArguments): unknown[] { /** * Patches original App to utilize our tracing functionality */ -function patchMiddleware(app: Application, method: Method | 'use'): Application { +function patchMiddleware(app: Application, method: Method): Application { const originalAppCallback = app[method]; - app[method] = function(): any { - // eslint-disable-next-line prefer-rest-params - return originalAppCallback.apply(this, wrapUseArgs(arguments)); + app[method] = function(...args: unknown[]): any { + return originalAppCallback.apply(this, wrapMiddlewareArgs(args, method)); }; return app; } /** - * Patches original app.use + * Patches original application methods */ -function instrumentMiddlewares(app: Application): void { - patchMiddleware(app, 'use'); +function instrumentMiddlewares(app: Application, methods: Method[] = []): void { + methods.forEach((method: Method) => patchMiddleware(app, method)); } /** - * Patches original app.METHOD + * Set parameterized as transaction name e.g.: `GET /users/:id` + * Also adds more context data on the transaction from the request */ -function routeMiddlewares(app: Application, methods: Method[] = []): void { - methods.forEach(function(method: Method) { - patchMiddleware(app, method); - }); +function addExpressReqToTransaction(transaction: Transaction | undefined, req: Request): void { + 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); + } } From 10465c0b645c1128e0f4f088c3d112fbe4536dc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Thu, 19 Nov 2020 13:32:29 +0100 Subject: [PATCH 2/3] ref: Change types to express Router instead of Application --- packages/tracing/src/integrations/express.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/tracing/src/integrations/express.ts b/packages/tracing/src/integrations/express.ts index bbc4f9d5e41a..45e35bab1d94 100644 --- a/packages/tracing/src/integrations/express.ts +++ b/packages/tracing/src/integrations/express.ts @@ -2,7 +2,6 @@ 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' @@ -30,7 +29,7 @@ type Method = | 'unsubscribe' | 'use'; -type Application = { +type Router = { [method in Method]: (...args: any) => any; }; @@ -80,13 +79,13 @@ export class Express implements Integration { /** * Express App instance */ - private readonly _app?: Application; + private readonly _app?: Router; private readonly _methods?: Method[]; /** * @inheritDoc */ - public constructor(options: { app?: Application; methods?: Method[] } = {}) { + public constructor(options: { app?: Router; methods?: Method[] } = {}) { this._app = options.app; this._methods = (Array.isArray(options.methods) ? options.methods : []).concat('use'); } @@ -224,7 +223,7 @@ function wrapMiddlewareArgs(args: unknown[], method: Method): unknown[] { /** * Patches original App to utilize our tracing functionality */ -function patchMiddleware(app: Application, method: Method): Application { +function patchMiddleware(app: Router, method: Method): Router { const originalAppCallback = app[method]; app[method] = function(...args: unknown[]): any { @@ -237,7 +236,7 @@ function patchMiddleware(app: Application, method: Method): Application { /** * Patches original application methods */ -function instrumentMiddlewares(app: Application, methods: Method[] = []): void { +function instrumentMiddlewares(app: Router, methods: Method[] = []): void { methods.forEach((method: Method) => patchMiddleware(app, method)); } From 32abd2c54dc7602039335214b2000c07a24ebe4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Thu, 19 Nov 2020 16:10:29 +0100 Subject: [PATCH 3/3] fix: Use correct paths for express transactions and unify express integration --- packages/node/src/handlers.ts | 27 +++- packages/tracing/src/integrations/express.ts | 133 +++++++------------ 2 files changed, 71 insertions(+), 89 deletions(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index bcdb94467353..de2945c7ca33 100644 --- a/packages/node/src/handlers.ts +++ b/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 */ diff --git a/packages/tracing/src/integrations/express.ts b/packages/tracing/src/integrations/express.ts index 45e35bab1d94..1c844d41be51 100644 --- a/packages/tracing/src/integrations/express.ts +++ b/packages/tracing/src/integrations/express.ts @@ -1,4 +1,3 @@ -/* eslint-disable @typescript-eslint/no-explicit-any */ import { Integration, Transaction } from '@sentry/types'; import { logger } from '@sentry/utils'; @@ -30,27 +29,13 @@ type Method = | 'use'; type Router = { - [method in Method]: (...args: any) => any; + [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; } -interface Request { - route?: { - path: string; - }; - method: string; - originalUrl: string; - baseUrl: string; - query: string; -} - /** * Internal helper for `__sentry_transaction` * @hidden @@ -62,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 { /** @@ -79,14 +63,14 @@ export class Express implements Integration { /** * Express App instance */ - private readonly _app?: Router; + private readonly _router?: Router; private readonly _methods?: Method[]; /** * @inheritDoc */ - public constructor(options: { app?: Router; methods?: Method[] } = {}) { - this._app = options.app; + 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'); } @@ -94,11 +78,11 @@ export class Express implements Integration { * @inheritDoc */ public setupOnce(): void { - if (!this._app) { + if (!this._router) { logger.error('ExpressIntegration is missing an Express instance'); return; } - instrumentMiddlewares(this._app, this._methods); + instrumentMiddlewares(this._router, this._methods); } } @@ -113,16 +97,17 @@ 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, method: Method): 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, @@ -132,56 +117,43 @@ function wrap(fn: Function, method: Method): RequestHandler | ErrorRequestHandle 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.${method}`, - }); - 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.${method}`, - }); - 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); }); }; } @@ -192,7 +164,7 @@ function wrap(fn: Function, method: Method): RequestHandler | ErrorRequestHandle } /** - * 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. @@ -221,36 +193,21 @@ function wrapMiddlewareArgs(args: unknown[], method: Method): unknown[] { } /** - * Patches original App to utilize our tracing functionality + * Patches original router to utilize our tracing functionality */ -function patchMiddleware(app: Router, method: Method): Router { - const originalAppCallback = app[method]; +function patchMiddleware(router: Router, method: Method): Router { + const originalCallback = router[method]; - app[method] = function(...args: unknown[]): any { - return originalAppCallback.apply(this, wrapMiddlewareArgs(args, method)); + router[method] = function(...args: unknown[]): void { + return originalCallback.call(this, ...wrapMiddlewareArgs(args, method)); }; - return app; + return router; } /** - * Patches original application methods + * Patches original router methods */ -function instrumentMiddlewares(app: Router, methods: Method[] = []): void { - methods.forEach((method: Method) => patchMiddleware(app, method)); -} - -/** - * 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: Request): void { - 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); - } +function instrumentMiddlewares(router: Router, methods: Method[] = []): void { + methods.forEach((method: Method) => patchMiddleware(router, method)); }