From 8f8a7516e0fc45c240bd7774d0dda4c21f592d02 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 28 Jul 2022 11:05:42 +0200 Subject: [PATCH 1/3] fix(node): Adjust Express URL parameterization for RegEx routes --- .../tracing/src/integrations/node/express.ts | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index 451fc847ff94..edaebe16c086 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -1,6 +1,6 @@ /* eslint-disable max-lines */ import { Integration, Transaction } from '@sentry/types'; -import { CrossPlatformRequest, extractPathForTransaction, logger } from '@sentry/utils'; +import { CrossPlatformRequest, extractPathForTransaction, isRegExp, isString, logger } from '@sentry/utils'; type Method = | 'all' @@ -55,7 +55,7 @@ type ExpressRouter = Router & { type Layer = { match: (path: string) => boolean; handle_request: (req: PatchedRequest, res: ExpressResponse, next: () => void) => void; - route?: { path: string }; + route?: { path: string | RegExp }; path?: string; }; @@ -273,9 +273,14 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { req._reconstructedRoute = ''; } - // 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 || ''; + // If the layer's partial route has params, the route is stored in layer.route. + // Since a route might be defined with a RegExp, we convert it toString to make sure we end up with a string + const lrp = layer.route?.path; + const isRegex = isRegExp(lrp); + const layerRoutePath = isRegex ? lrp?.toString() : (lrp as string); + + // Otherwise, the hardcoded path (i.e. a partial route without params) is stored in layer.path + const partialRoute = layerRoutePath || layer.path || ''; // Normalize the partial route so that it doesn't contain leading or trailing slashes // and exclude empty or '*' wildcard routes. @@ -288,15 +293,17 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { .join('/'); // If we found a valid partial URL, we append it to the reconstructed route - if (finalPartialRoute.length > 0) { - req._reconstructedRoute += `/${finalPartialRoute}`; + if (finalPartialRoute && finalPartialRoute.length > 0) { + // If the partial route is from a regex route, we append a '/' to close the regex + req._reconstructedRoute += `/${finalPartialRoute}${isRegex ? '/' : ''}`; } // Now we check if we are in the "last" part of the route. We determine this by comparing the // 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; + const urlLength = getNumberOfUrlSegments(req.originalUrl || ''); + const routeLength = getNumberOfUrlSegments(req._reconstructedRoute); + if (urlLength === routeLength) { const transaction = res.__sentry_transaction; if (transaction && transaction.metadata.source !== 'custom') { @@ -311,3 +318,8 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { return originalProcessParams.call(this, layer, called, req, res, done); }; } + +function getNumberOfUrlSegments(url: string): number { + // split at '/' or at '\/' to split regex urls correctly + return url.split(/\\?\//).filter(s => s.length > 0).length; +} From 812066434d004d074720ed319d292befa4376665 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 28 Jul 2022 13:05:25 +0200 Subject: [PATCH 2/3] remove unused import --- packages/tracing/src/integrations/node/express.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index edaebe16c086..dc3ae0696c09 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -1,6 +1,6 @@ /* eslint-disable max-lines */ import { Integration, Transaction } from '@sentry/types'; -import { CrossPlatformRequest, extractPathForTransaction, isRegExp, isString, logger } from '@sentry/utils'; +import { CrossPlatformRequest, extractPathForTransaction, isRegExp, logger } from '@sentry/utils'; type Method = | 'all' From 01eb6486cda0e9542cbce01e371890811d1ecc2e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 28 Jul 2022 15:49:24 +0200 Subject: [PATCH 3/3] add test --- .../suites/express/tracing/server.ts | 4 +++ .../suites/express/tracing/test.ts | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/packages/node-integration-tests/suites/express/tracing/server.ts b/packages/node-integration-tests/suites/express/tracing/server.ts index 1eac2074ddaa..acfaca932f49 100644 --- a/packages/node-integration-tests/suites/express/tracing/server.ts +++ b/packages/node-integration-tests/suites/express/tracing/server.ts @@ -21,6 +21,10 @@ app.get('/test/express', (_req, res) => { res.send({ response: 'response 1' }); }); +app.get(/\/test\/regex/, (_req, res) => { + res.send({ response: 'response 2' }); +}); + app.use(Sentry.Handlers.errorHandler()); export default app; diff --git a/packages/node-integration-tests/suites/express/tracing/test.ts b/packages/node-integration-tests/suites/express/tracing/test.ts index 3af4c6fa0238..8190f4bbed6a 100644 --- a/packages/node-integration-tests/suites/express/tracing/test.ts +++ b/packages/node-integration-tests/suites/express/tracing/test.ts @@ -27,3 +27,29 @@ test('should create and send transactions for Express routes and spans for middl ], }); }); + +test('should set a correct transaction name for routes specified in RegEx', async () => { + const url = await runServer(__dirname, `${__dirname}/server.ts`); + const envelope = await getEnvelopeRequest(`${url}/regex`); + + expect(envelope).toHaveLength(3); + + assertSentryTransaction(envelope[2], { + transaction: 'GET /\\/test\\/regex/', + transaction_info: { + source: 'route', + }, + contexts: { + trace: { + data: { + url: '/test/regex', + }, + op: 'http.server', + status: 'ok', + tags: { + 'http.status_code': '200', + }, + }, + }, + }); +});