Skip to content

Express integration span name change and path unification #3078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion packages/node/src/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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`
Expand Down Expand Up @@ -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();
});
Expand All @@ -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 */
Expand Down
154 changes: 54 additions & 100 deletions packages/tracing/src/integrations/express.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand All @@ -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;
}

Expand All @@ -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 {
/**
Expand All @@ -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);
}
}

Expand All @@ -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);
});
};
}
Expand All @@ -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.
Expand All @@ -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;
});
Expand All @@ -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));
}