From 69f2f2485a53540f8a327767012ac5043c9bafae Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 22 Jul 2022 09:16:47 +0200 Subject: [PATCH 1/8] ref(node): Improve Express URL parameterization --- packages/node/src/handlers.ts | 4 +- .../tracing/src/integrations/node/express.ts | 101 +++++++++++++++++- 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 417b60c4795f..ee5f06064597 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -65,7 +65,9 @@ export function tracingHandler(): ( // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction // closes setImmediate(() => { - addRequestDataToTransaction(transaction, req); + if (!transaction.metadata.source || transaction.metadata.source === 'url') { + addRequestDataToTransaction(transaction, req); + } transaction.setHttpStatus(res.statusCode); transaction.finish(); }); diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index df9af31f00c9..d5c8c25014d4 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -1,5 +1,7 @@ +/* eslint-disable max-lines */ +import { getCurrentHub } from '@sentry/hub'; import { Integration, Transaction } from '@sentry/types'; -import { logger } from '@sentry/utils'; +import { CrossPlatformRequest, logger } from '@sentry/utils'; type Method = | 'all' @@ -32,6 +34,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 +111,7 @@ export class Express implements Integration { return; } instrumentMiddlewares(this._router, this._methods); + instrumentRouter(this._router as ExpressRouter); } } @@ -211,3 +240,73 @@ 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, + 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 layers partial route has params, it's stored in layer.route, else it's 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 of not "polluting" the transaction name + // with interim handlers (e.g. that check authentication or do other middleware stuff) + // We want to end up with the parameterized URL of the incoming request with nothing in between + 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 + // numbers of URL segments from the original URL against our reconstructed parameterized URL. + // In case 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 = getCurrentHub().getScope()?.getTransaction(); + if (transaction && transaction.metadata.source !== 'custom') { + const finalRoute = req._reconstructedRoute.replace(/\/$/, ''); + const method = req.method && req.method.toUpperCase(); + transaction.setName(`${method} ${finalRoute}`, 'route'); + logger.debug('TX is now called', transaction.name); + } + } + + return originalProcessParams.call(this, layer, called, req, res, done); + }; +} From abcefd1203d8b86f43e7756b85dc026dd37d0d44 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 22 Jul 2022 14:32:05 +0200 Subject: [PATCH 2/8] adjust tests --- .../suites/express/sentry-trace/baggage-header-assign/test.ts | 4 ++-- .../suites/express/sentry-trace/baggage-header-out/server.ts | 1 + .../baggage-other-vendors-with-sentry-entries/test.ts | 2 +- packages/node/src/handlers.ts | 4 +--- packages/utils/src/requestdata.ts | 4 +++- 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts index f50c782fdb4a..a8dc964e9c72 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts +++ b/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=', ), }, }); diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts index 2ec3c7ca010d..15d660a418ec 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts @@ -27,6 +27,7 @@ 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(); diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts index 30cfc024016b..30c86c349092 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts +++ b/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', ), }, }); diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index ee5f06064597..417b60c4795f 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -65,9 +65,7 @@ export function tracingHandler(): ( // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction // closes setImmediate(() => { - if (!transaction.metadata.source || transaction.metadata.source === 'url') { - addRequestDataToTransaction(transaction, req); - } + addRequestDataToTransaction(transaction, req); transaction.setHttpStatus(res.statusCode); transaction.finish(); }); diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 3bcc9b3aa306..87d95e8539d3 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -113,7 +113,9 @@ export function addRequestDataToTransaction( deps?: InjectedNodeDeps, ): void { if (!transaction) return; - transaction.name = extractPathForTransaction(req, { path: true, method: true }); + if (!transaction.metadata.source || transaction.metadata.source === 'url') { + transaction.name = extractPathForTransaction(req, { path: true, method: true }); + } transaction.setData('url', req.originalUrl || req.url); if (req.baseUrl) { transaction.setData('baseUrl', req.baseUrl); From a8855d5fdf8c375b2be4bb2d2ab0dcf275eec286 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 22 Jul 2022 15:30:38 +0200 Subject: [PATCH 3/8] set transaction source, refactor extractPathForTransaction --- packages/node/src/handlers.ts | 5 +- packages/node/test/requestdata.test.ts | 95 ++++++++++++++++++- .../tracing/src/integrations/node/express.ts | 6 +- packages/utils/src/requestdata.ts | 37 +++++--- 4 files changed, 122 insertions(+), 21 deletions(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 417b60c4795f..eedee94abf0f 100644 --- a/packages/node/src/handlers.ts +++ b/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) }, diff --git a/packages/node/test/requestdata.test.ts b/packages/node/test/requestdata.test.ts index 063d30d505e2..cad1e0b9d46d 100644 --- a/packages/node/test/requestdata.test.ts +++ b/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,95 @@ 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 }, '/other/path/:id/details'); + + expect(route).toEqual('GET /other/path/:id/details'); + expect(source).toEqual('route'); + }); +}); diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index d5c8c25014d4..e4ffb41804a5 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -1,7 +1,7 @@ /* eslint-disable max-lines */ import { getCurrentHub } from '@sentry/hub'; import { Integration, Transaction } from '@sentry/types'; -import { CrossPlatformRequest, logger } from '@sentry/utils'; +import { CrossPlatformRequest, extractPathForTransaction, logger } from '@sentry/utils'; type Method = | 'all' @@ -301,9 +301,7 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { const transaction = getCurrentHub().getScope()?.getTransaction(); if (transaction && transaction.metadata.source !== 'custom') { const finalRoute = req._reconstructedRoute.replace(/\/$/, ''); - const method = req.method && req.method.toUpperCase(); - transaction.setName(`${method} ${finalRoute}`, 'route'); - logger.debug('TX is now called', transaction.name); + transaction.setName(...extractPathForTransaction(req, { path: true, method: true }, finalRoute)); } } diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 87d95e8539d3..7b673ae0b173 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -12,7 +12,7 @@ /* eslint-disable max-lines */ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { Event, ExtractedNodeRequestData, Transaction } from '@sentry/types'; +import { Event, ExtractedNodeRequestData, Transaction, TransactionSource } from '@sentry/types'; import { isPlainObject, isString } from './is'; import { stripUrlQueryAndFragment } from './misc'; @@ -114,7 +114,7 @@ export function addRequestDataToTransaction( ): void { if (!transaction) return; if (!transaction.metadata.source || transaction.metadata.source === 'url') { - transaction.name = extractPathForTransaction(req, { path: true, method: true }); + transaction.setName(...extractPathForTransaction(req, { path: true, method: true })); } transaction.setData('url', req.originalUrl || req.url); if (req.baseUrl) { @@ -124,43 +124,52 @@ export function addRequestDataToTransaction( } /** - * Extracts complete generalized path from the request object and uses it to construct transaction name. + * Extracts a complete and parameterized path from the request object and uses it to construct transaction name. + * If the parameterized transaction name cannot be extracted, we fall back to the raw URL. + * + * Additionally, this function determines and returns the transaction name source * * eg. GET /mountpoint/user/:id * * @param req A request object * @param options What to include in the transaction name (method, path, or both) + * @param customRoute An optional string that should be used as transaction name instead of the requests path * - * @returns The fully constructed transaction name + * @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either route or url) */ export function extractPathForTransaction( req: CrossPlatformRequest, options: { path?: boolean; method?: boolean } = {}, -): string { + customRoute?: string, +): [string, TransactionSource] { const method = req.method && req.method.toUpperCase(); let path = ''; + let source: TransactionSource = 'url'; + // Check to see if there's a parameterized route we can use (as there is in Express) - if (req.route) { - path = `${req.baseUrl || ''}${req.route.path}`; + if (customRoute || req.route) { + path = customRoute || `${req.baseUrl || ''}${req.route && req.route.path}`; + source = 'route'; } + // Otherwise, just take the original URL else if (req.originalUrl || req.url) { path = stripUrlQueryAndFragment(req.originalUrl || req.url || ''); } - let info = ''; + let name = ''; if (options.method && method) { - info += method; + name += method; } if (options.method && options.path) { - info += ' '; + name += ' '; } if (options.path && path) { - info += path; + name += path; } - return info; + return [name, source]; } type TransactionNamingScheme = 'path' | 'methodPath' | 'handler'; @@ -169,14 +178,14 @@ type TransactionNamingScheme = 'path' | 'methodPath' | 'handler'; function extractTransaction(req: CrossPlatformRequest, type: boolean | TransactionNamingScheme): string { switch (type) { case 'path': { - return extractPathForTransaction(req, { path: true }); + return extractPathForTransaction(req, { path: true })[0]; } case 'handler': { return (req.route && req.route.stack && req.route.stack[0] && req.route.stack[0].name) || ''; } case 'methodPath': default: { - return extractPathForTransaction(req, { path: true, method: true }); + return extractPathForTransaction(req, { path: true, method: true })[0]; } } } From 0f4d234342f8a3573ca9a8e39be6e31bdc091b43 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 22 Jul 2022 16:50:07 +0200 Subject: [PATCH 4/8] change transaction retrieval --- packages/tracing/src/integrations/node/express.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index e4ffb41804a5..1fda2219112b 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -1,5 +1,4 @@ /* eslint-disable max-lines */ -import { getCurrentHub } from '@sentry/hub'; import { Integration, Transaction } from '@sentry/types'; import { CrossPlatformRequest, extractPathForTransaction, logger } from '@sentry/utils'; @@ -266,7 +265,7 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { layer: Layer, called: unknown, req: PatchedRequest, - res: ExpressResponse, + res: ExpressResponse & SentryTracingResponse, done: () => unknown, ) { // Base case: We're in the first part of the URL (thus we start with the root '/') @@ -298,7 +297,7 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { 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 = getCurrentHub().getScope()?.getTransaction(); + const transaction = res.__sentry_transaction; if (transaction && transaction.metadata.source !== 'custom') { const finalRoute = req._reconstructedRoute.replace(/\/$/, ''); transaction.setName(...extractPathForTransaction(req, { path: true, method: true }, finalRoute)); From e89040230d0801fb50a2c42a8ea23519132f4503 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 26 Jul 2022 12:45:35 +0200 Subject: [PATCH 5/8] split tests --- .../baggage-header-out-bad-tx-name/server.ts | 40 +++++++++++++++++++ .../baggage-header-out-bad-tx-name/test.ts | 19 +++++++++ .../sentry-trace/baggage-header-out/server.ts | 1 - .../sentry-trace/baggage-header-out/test.ts | 18 +-------- 4 files changed, 60 insertions(+), 18 deletions(-) create mode 100644 packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/server.ts create mode 100644 packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/test.ts diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/server.ts new file mode 100644 index 000000000000..15d660a418ec --- /dev/null +++ b/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://public@dsn.ingest.sentry.io/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; diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/test.ts new file mode 100644 index 000000000000..eb969e6e3a8a --- /dev/null +++ b/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='); +}); diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts index 15d660a418ec..2ec3c7ca010d 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts @@ -27,7 +27,6 @@ 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(); diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts index a2eaebbf8c42..099e4e4bd65f 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts +++ b/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='); -}); From ca0f622b34ac12bba5def78f5a4ae60f8f0dfc08 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 26 Jul 2022 12:45:42 +0200 Subject: [PATCH 6/8] apply suggestions from code revew --- .../tracing/src/integrations/node/express.ts | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index 1fda2219112b..f0a0eb3be505 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -253,8 +253,8 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { 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(); + if (isApp && appOrRouter._router === undefined && appOrRouter.lazyrouter) { + appOrRouter.lazyrouter(); } const router = isApp ? appOrRouter._router : appOrRouter; @@ -270,17 +270,18 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { ) { // Base case: We're in the first part of the URL (thus we start with the root '/') if (!req._reconstructedRoute) { - req._reconstructedRoute = '/'; + req._reconstructedRoute = ''; } - // If the layers partial route has params, it's stored in layer.route, else it's in layer.path + // 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 of not "polluting" the transaction name - // with interim handlers (e.g. that check authentication or do other middleware stuff) - // We want to end up with the parameterized URL of the incoming request with nothing in between + // 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('*')) @@ -288,18 +289,21 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { // If we found a valid partial URL, we append it to the reconstructed route if (finalPartialRoute.length > 0) { - req._reconstructedRoute += `${finalPartialRoute}/`; + req._reconstructedRoute += `/${finalPartialRoute}`; } // Now we check if we are in the "last" part of the route. We determine this by comparing the - // numbers of URL segments from the original URL against our reconstructed parameterized URL. - // In case we've reached our final destination, we update the transaction name. + // 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') { - const finalRoute = req._reconstructedRoute.replace(/\/$/, ''); + // 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 }, finalRoute)); } } From bf2494c13f07a3a0f6c751292bf31ccba58a35d9 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 26 Jul 2022 12:46:33 +0200 Subject: [PATCH 7/8] Update packages/utils/src/requestdata.ts Co-authored-by: Katie Byers --- packages/utils/src/requestdata.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 7b673ae0b173..8736615712d6 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -114,6 +114,7 @@ export function addRequestDataToTransaction( ): void { if (!transaction) return; if (!transaction.metadata.source || transaction.metadata.source === 'url') { + // Attempt to grab a parameterized route off of the request transaction.setName(...extractPathForTransaction(req, { path: true, method: true })); } transaction.setData('url', req.originalUrl || req.url); From 7581a1a721b5c2079f50924a33b3bcbca87c912d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 26 Jul 2022 12:54:28 +0200 Subject: [PATCH 8/8] change addRequestDataToTransaction third param to options property --- packages/node/test/requestdata.test.ts | 6 +++++- packages/tracing/src/integrations/node/express.ts | 2 +- packages/utils/src/requestdata.ts | 13 ++++++------- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/packages/node/test/requestdata.test.ts b/packages/node/test/requestdata.test.ts index cad1e0b9d46d..e79e86bb981b 100644 --- a/packages/node/test/requestdata.test.ts +++ b/packages/node/test/requestdata.test.ts @@ -572,7 +572,11 @@ describe('extractPathForTransaction', () => { originalUrl: '/api/users/123/details', } as CrossPlatformRequest; - const [route, source] = extractPathForTransaction(req, { path: true, method: true }, '/other/path/:id/details'); + 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'); diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index f0a0eb3be505..451fc847ff94 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -304,7 +304,7 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { // 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 }, finalRoute)); + transaction.setName(...extractPathForTransaction(req, { path: true, method: true, customRoute: finalRoute })); } } diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 8736615712d6..72c12b2e5399 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -133,15 +133,14 @@ export function addRequestDataToTransaction( * eg. GET /mountpoint/user/:id * * @param req A request object - * @param options What to include in the transaction name (method, path, or both) - * @param customRoute An optional string that should be used as transaction name instead of the requests path + * @param options What to include in the transaction name (method, path, or a custom route name to be + * used instead of the request's route) * - * @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either route or url) + * @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either 'route' or 'url') */ export function extractPathForTransaction( req: CrossPlatformRequest, - options: { path?: boolean; method?: boolean } = {}, - customRoute?: string, + options: { path?: boolean; method?: boolean; customRoute?: string } = {}, ): [string, TransactionSource] { const method = req.method && req.method.toUpperCase(); @@ -149,8 +148,8 @@ export function extractPathForTransaction( let source: TransactionSource = 'url'; // Check to see if there's a parameterized route we can use (as there is in Express) - if (customRoute || req.route) { - path = customRoute || `${req.baseUrl || ''}${req.route && req.route.path}`; + if (options.customRoute || req.route) { + path = options.customRoute || `${req.baseUrl || ''}${req.route && req.route.path}`; source = 'route'; }