diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index c5d819b1b66b..ad353423c022 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -20,7 +20,7 @@ import { flush } from './sdk'; const DEFAULT_SHUTDOWN_TIMEOUT = 2000; -interface ExpressRequest extends http.IncomingMessage { +export interface ExpressRequest extends http.IncomingMessage { [key: string]: any; baseUrl?: string; ip?: string; @@ -59,7 +59,7 @@ export function tracingHandler(): ( } const transaction = startTransaction({ - name: extractRouteInfo(req, { path: true, method: true }), + name: extractExpressTransactionName(req, { path: true, method: true }), op: 'http.server', ...traceparentData, }); @@ -75,7 +75,8 @@ export function tracingHandler(): ( (res as any).__sentry_transaction = transaction; res.once('finish', () => { - // We schedule the immediate execution of the `finish` to let all the spans being closed first. + // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction + // closes setImmediate(() => { addExpressReqToTransaction(transaction, req); transaction.setHttpStatus(res.statusCode); @@ -93,27 +94,35 @@ export function tracingHandler(): ( */ function addExpressReqToTransaction(transaction: Transaction | undefined, req: ExpressRequest): void { if (!transaction) return; - transaction.name = extractRouteInfo(req, { path: true, method: true }); + transaction.name = extractExpressTransactionName(req, { path: true, method: true }); transaction.setData('url', req.originalUrl); transaction.setData('baseUrl', req.baseUrl); transaction.setData('query', req.query); } /** - * Extracts complete generalized path from the request object. - * eg. /mountpoint/user/:id + * Extracts complete generalized path from the request object and uses it to construct transaction name. + * + * eg. GET /mountpoint/user/:id + * + * @param req The ExpressRequest object + * @param options What to include in the transaction name (method, path, or both) + * + * @returns The fully constructed transaction name */ -function extractRouteInfo(req: ExpressRequest, options: { path?: boolean; method?: boolean } = {}): string { +function extractExpressTransactionName( + req: ExpressRequest, + options: { path?: boolean; method?: boolean } = {}, +): string { const method = req.method?.toUpperCase(); - let path; - if (req.baseUrl && req.route) { + + let path = ''; + if (req.route) { + // if the mountpoint is `/`, req.baseUrl is '' (not undefined), so it's safe to include it here + // see https://github.com/expressjs/express/blob/508936853a6e311099c9985d4c11a4b1b8f6af07/test/req.baseUrl.js#L7 path = `${req.baseUrl}${req.route.path}`; - } else if (req.route) { - path = `${req.route.path}`; } else if (req.originalUrl || req.url) { path = stripUrlQueryAndFragment(req.originalUrl || req.url || ''); - } else { - path = ''; } let info = ''; @@ -136,14 +145,14 @@ type TransactionTypes = 'path' | 'methodPath' | 'handler'; function extractTransaction(req: ExpressRequest, type: boolean | TransactionTypes): string { switch (type) { case 'path': { - return extractRouteInfo(req, { path: true }); + return extractExpressTransactionName(req, { path: true }); } case 'handler': { return req.route?.stack[0].name || ''; } case 'methodPath': default: { - return extractRouteInfo(req, { path: true, method: true }); + return extractExpressTransactionName(req, { path: true, method: true }); } } } diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index d189f7531ba0..6f1bdd43ba50 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -8,29 +8,34 @@ import * as net from 'net'; import { Event, Request, User } from '../src'; import { NodeClient } from '../src/client'; -import { parseRequest, tracingHandler } from '../src/handlers'; +import { ExpressRequest, parseRequest, tracingHandler } from '../src/handlers'; describe('parseRequest', () => { let mockReq: { [key: string]: any }; beforeEach(() => { mockReq = { + baseUrl: '/routerMountPath', body: 'foo', cookies: { test: 'test' }, headers: { host: 'mattrobenolt.com', }, method: 'POST', - originalUrl: '/some/originalUrl?key=value', + originalUrl: '/routerMountPath/subpath/specificValue?querystringKey=querystringValue', + path: '/subpath/specificValue', + query: { + querystringKey: 'querystringValue', + }, route: { - path: '/path', + path: '/subpath/:parameterName', stack: [ { - name: 'routeHandler', + name: 'parameterNameRouteHandler', }, ], }, - url: '/some/url?key=value', + url: '/subpath/specificValue?querystringKey=querystringValue', user: { custom_property: 'foo', email: 'tobias@mail.com', @@ -42,17 +47,17 @@ describe('parseRequest', () => { describe('parseRequest.contexts runtime', () => { test('runtime name must contain node', () => { - const parsedRequest: Event = parseRequest({}, mockReq); + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); expect((parsedRequest.contexts!.runtime as Runtime).name).toEqual('node'); }); test('runtime version must contain current node version', () => { - const parsedRequest: Event = parseRequest({}, mockReq); + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); expect((parsedRequest.contexts!.runtime as Runtime).version).toEqual(process.version); }); test('runtime disbaled by options', () => { - const parsedRequest: Event = parseRequest({}, mockReq, { + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { version: false, }); expect(parsedRequest).not.toHaveProperty('contexts.runtime'); @@ -64,12 +69,12 @@ describe('parseRequest', () => { const CUSTOM_USER_KEYS = ['custom_property']; test('parseRequest.user only contains the default properties from the user', () => { - const parsedRequest: Event = parseRequest({}, mockReq); + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); expect(Object.keys(parsedRequest.user as User)).toEqual(DEFAULT_USER_KEYS); }); test('parseRequest.user only contains the custom properties specified in the options.user array', () => { - const parsedRequest: Event = parseRequest({}, mockReq, { + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { user: CUSTOM_USER_KEYS, }); expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS); @@ -95,7 +100,7 @@ describe('parseRequest', () => { { ...mockReq, ip: '123', - }, + } as ExpressRequest, { ip: true, }, @@ -110,8 +115,8 @@ describe('parseRequest', () => { ...mockReq, connection: { remoteAddress: '321', - }, - }, + } as net.Socket, + } as ExpressRequest, { ip: true, }, @@ -123,55 +128,56 @@ describe('parseRequest', () => { describe('parseRequest.request properties', () => { test('parseRequest.request only contains the default set of properties from the request', () => { const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; - const parsedRequest: Event = parseRequest({}, mockReq); + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); expect(Object.keys(parsedRequest.request as Request)).toEqual(DEFAULT_REQUEST_PROPERTIES); }); test('parseRequest.request only contains the specified properties in the options.request array', () => { const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url']; - const parsedRequest: Event = parseRequest({}, mockReq, { + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { request: INCLUDED_PROPERTIES, }); expect(Object.keys(parsedRequest.request as Request)).toEqual(INCLUDED_PROPERTIES); }); test('parseRequest.request skips `body` property for GET and HEAD requests', () => { - expect(parseRequest({}, mockReq, {}).request).toHaveProperty('data'); - expect(parseRequest({}, { ...mockReq, method: 'GET' }, {}).request).not.toHaveProperty('data'); - expect(parseRequest({}, { ...mockReq, method: 'HEAD' }, {}).request).not.toHaveProperty('data'); + expect(parseRequest({}, mockReq as ExpressRequest, {}).request).toHaveProperty('data'); + expect(parseRequest({}, { ...mockReq, method: 'GET' } as ExpressRequest, {}).request).not.toHaveProperty('data'); + expect(parseRequest({}, { ...mockReq, method: 'HEAD' } as ExpressRequest, {}).request).not.toHaveProperty('data'); }); }); describe('parseRequest.transaction property', () => { - test('extracts method and full route path by default from `originalUrl`', () => { - const parsedRequest: Event = parseRequest({}, mockReq); - expect(parsedRequest.transaction).toEqual('POST /some/originalUrl'); + test('extracts method and full route path by default`', () => { + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName'); }); - test('extracts method and full route path by default from `url` if `originalUrl` is not present', () => { - delete mockReq.originalUrl; - const parsedRequest: Event = parseRequest({}, mockReq); - expect(parsedRequest.transaction).toEqual('POST /some/url'); + test('extracts method and full path by default when mountpoint is `/`', () => { + mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', ''); + mockReq.baseUrl = ''; + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); + // "sub"path is the full path here, because there's no router mount path + expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName'); }); - test('fallback to method and `route.path` if previous attempts failed', () => { - delete mockReq.originalUrl; - delete mockReq.url; - const parsedRequest: Event = parseRequest({}, mockReq); - expect(parsedRequest.transaction).toEqual('POST /path'); + test('fallback to method and `originalUrl` if route is missing', () => { + delete mockReq.route; + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); }); test('can extract path only instead if configured', () => { - const parsedRequest: Event = parseRequest({}, mockReq, { transaction: 'path' }); - expect(parsedRequest.transaction).toEqual('/some/originalUrl'); + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { transaction: 'path' }); + expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); }); test('can extract handler name instead if configured', () => { - const parsedRequest: Event = parseRequest({}, mockReq, { transaction: 'handler' }); - expect(parsedRequest.transaction).toEqual('routeHandler'); + const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { transaction: 'handler' }); + expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); }); }); -}); // end describe('parseRequest()') +}); describe('tracingHandler', () => { const urlString = 'http://dogs.are.great:1231/yay/';