From 214b12694ccc2c0db20abbc6e4f4008ce8d6e0c2 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Thu, 22 May 2025 13:48:45 +0200 Subject: [PATCH 1/5] fix(express): Ensure 404 routes don't attach route attribute --- .../src/instrumentation.ts | 6 ++- .../test/express.test.ts | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 122427809a..5e5cc855be 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -194,8 +194,10 @@ export class ExpressInstrumentation extends InstrumentationBase 0 ? route : undefined; + const attributes: Attributes = { - [ATTR_HTTP_ROUTE]: route.length > 0 ? route : '/', + [ATTR_HTTP_ROUTE]: actualRoute, }; const metadata = getLayerMetadata(route, layer, layerPath); const type = metadata.attributes[ @@ -204,7 +206,7 @@ export class ExpressInstrumentation extends InstrumentationBase { server?.close(); }); + it('does not attach semantic route attribute for 404 page', async () => { + const rootSpan = tracer.startSpan('rootSpan'); + const httpServer = await serverWithMiddleware(tracer, rootSpan, app => { + app.use(express.json()); + }); + server = httpServer.server; + port = httpServer.port; + assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); + + await context.with( + trace.setSpan(context.active(), rootSpan), + async () => { + try { + await httpRequest.get( + `http://localhost:${port}/non-existing-route` + ); + } catch (error) {} + rootSpan.end(); + + const spans = memoryExporter.getFinishedSpans(); + + // Should have middleware spans but no request handler span + const middlewareSpans = spans.filter(span => + span.name.includes('middleware') || + span.name.includes('expressInit') || + span.name.includes('jsonParser') + ); + + assert.ok(middlewareSpans.length > 0, 'Middleware spans should be created'); + + for (const span of spans) { + assert.strictEqual( + span.attributes[SEMATTRS_HTTP_ROUTE], + undefined, // none of the spans have the HTTP route attribute + `Span "${span.name}" should not have HTTP route attribute for non-existing route` + ); + } + } + ); + }); it('should create a child span for middlewares', async () => { const rootSpan = tracer.startSpan('rootSpan'); const customMiddleware: express.RequestHandler = (req, res, next) => { From 726cdcfd4bdc2aca3e4d14f6d5750ec54a12bd52 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 23 May 2025 13:49:01 +0200 Subject: [PATCH 2/5] handle route naming --- .../src/instrumentation.ts | 20 +- .../src/utils.ts | 78 +++++ .../test/express.test.ts | 14 +- .../test/utils.test.ts | 271 +++++++++++++++++- 4 files changed, 366 insertions(+), 17 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 5e5cc855be..f948925c49 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -32,6 +32,8 @@ import { getLayerPath, isLayerIgnored, storeLayerPath, + getActualMatchedRoute, + getConstructedRoute, } from './utils'; /** @knipignore */ import { PACKAGE_NAME, PACKAGE_VERSION } from './version'; @@ -188,25 +190,21 @@ export class ExpressInstrumentation extends InstrumentationBase path !== '/' && path !== '/*') - .join('') - // remove duplicate slashes to normalize route - .replace(/\/{2,}/g, '/'); - const actualRoute = route.length > 0 ? route : undefined; + const constructedRoute = getConstructedRoute(req); + const actualMatchedRoute = getActualMatchedRoute(req); const attributes: Attributes = { - [ATTR_HTTP_ROUTE]: actualRoute, + [ATTR_HTTP_ROUTE]: actualMatchedRoute, }; - const metadata = getLayerMetadata(route, layer, layerPath); + const metadata = getLayerMetadata(constructedRoute, layer, layerPath); const type = metadata.attributes[ AttributeNames.EXPRESS_TYPE ] as ExpressLayerType; const rpcMetadata = getRPCMetadata(context.active()); if (rpcMetadata?.type === RPCType.HTTP) { - rpcMetadata.route = actualRoute; + rpcMetadata.route = actualMatchedRoute; } // verify against the config if the layer should be ignored @@ -225,7 +223,7 @@ export class ExpressInstrumentation extends InstrumentationBase { if (e) { diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index 69a20146b9..bd8f7f666b 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -213,3 +213,81 @@ const extractLayerPathSegment = (arg: LayerPathSegment) => { return; }; + +export function getConstructedRoute(req: { + originalUrl: PatchedRequest['originalUrl']; + [_LAYERS_STORE_PROPERTY]?: string[]; +}) { + const layersStore: string[] = Array.isArray(req[_LAYERS_STORE_PROPERTY]) + ? (req[_LAYERS_STORE_PROPERTY] as string[]) + : []; + + const meaningfulPaths = layersStore.filter( + path => path !== '/' && path !== '/*' + ); + + // Handle standalone wildcard case + if (meaningfulPaths.length === 1 && meaningfulPaths[0] === '*') { + return '*'; + } + + // Construct the route by joining paths and normalizing + return meaningfulPaths.join('').replace(/\/{2,}/g, '/'); // Remove duplicate slashes +} + +/** + * Extracts the actual matched route from Express request for OpenTelemetry instrumentation. + * Returns the route that should be used as the http.route attribute. + * + * @param req - The Express request object with layers store + * @param layersStoreProperty - The property name where layer paths are stored + * @returns The matched route string or undefined if no valid route is found + */ +export function getActualMatchedRoute(req: { + originalUrl: PatchedRequest['originalUrl']; + [_LAYERS_STORE_PROPERTY]?: string[]; +}): string | undefined { + const layersStore: string[] = Array.isArray(req[_LAYERS_STORE_PROPERTY]) + ? (req[_LAYERS_STORE_PROPERTY] as string[]) + : []; + + // If no layers are stored, no route can be determined + if (layersStore.length === 0) { + return undefined; + } + + // Handle root path case - if all paths are root, only return root if originalUrl is also root + if (layersStore.every(path => path === '/')) { + return req.originalUrl === '/' ? '/' : undefined; + } + + const constructedRoute = getConstructedRoute(req); + if (constructedRoute === '*') { + return constructedRoute; + } + + // Ensure route starts with '/' if it doesn't already + const normalizedRoute = constructedRoute.startsWith('/') + ? constructedRoute + : `/${constructedRoute}`; + + // Validate that this appears to be a matched route + // A route is considered matched if: + // 1. We have a constructed route + // 2. The original URL matches or starts with our route pattern + const isValidRoute = + normalizedRoute.length > 0 && + (req.originalUrl === normalizedRoute || + req.originalUrl.startsWith(normalizedRoute) || + isRoutePattern(normalizedRoute)); + + return isValidRoute ? normalizedRoute : undefined; +} + +/** + * Checks if a route contains parameter patterns (e.g., :id, :userId) + * which are valid even if they don't exactly match the original URL + */ +function isRoutePattern(route: string): boolean { + return route.includes(':') || route.includes('*'); +} diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 2d09ee40ac..b47d3f9d65 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -89,13 +89,17 @@ describe('ExpressInstrumentation', () => { const spans = memoryExporter.getFinishedSpans(); // Should have middleware spans but no request handler span - const middlewareSpans = spans.filter(span => - span.name.includes('middleware') || - span.name.includes('expressInit') || - span.name.includes('jsonParser') + const middlewareSpans = spans.filter( + span => + span.name.includes('middleware') || + span.name.includes('expressInit') || + span.name.includes('jsonParser') ); - assert.ok(middlewareSpans.length > 0, 'Middleware spans should be created'); + assert.ok( + middlewareSpans.length > 0, + 'Middleware spans should be created' + ); for (const span of spans) { assert.strictEqual( diff --git a/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts index f8dac5035b..721d9ff285 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts @@ -17,7 +17,11 @@ import * as utils from '../src/utils'; import * as assert from 'assert'; import { ExpressInstrumentationConfig } from '../src/types'; -import { ExpressLayer } from '../src/internal-types'; +import { + _LAYERS_STORE_PROPERTY, + ExpressLayer, + PatchedRequest, +} from '../src/internal-types'; import { ExpressLayerType } from '../src/enums/ExpressLayerType'; import { AttributeNames } from '../src/enums/AttributeNames'; @@ -236,4 +240,269 @@ describe('Utils', () => { ); }); }); + + describe('getActualMatchedRoute', () => { + interface PatchedRequestPart { + originalUrl: PatchedRequest['originalUrl']; + [_LAYERS_STORE_PROPERTY]?: string[]; + } + + function createMockRequest( + originalUrl: string, + layersStore?: string[] + ): PatchedRequestPart { + const req: PatchedRequestPart = { + originalUrl, + [_LAYERS_STORE_PROPERTY]: layersStore, + }; + return req; + } + + describe('Basic functionality', () => { + it('should return undefined when layers store is empty', () => { + const req = createMockRequest('/api/users', []); + const result = utils.getActualMatchedRoute( + req as unknown as PatchedRequestPart + ); + assert.strictEqual(result, undefined); + }); + + it('should return undefined when layers store property is undefined', () => { + const req = createMockRequest('/api/users'); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, undefined); + }); + + it('should return undefined when layers store is not an array', () => { + const req = createMockRequest('/api/users'); + (req as any)[_LAYERS_STORE_PROPERTY] = 'not-an-array'; + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, undefined); + }); + }); + + describe('Root path handling', () => { + it('should return "/" when originalUrl is "/" and layers store contains only root paths', () => { + const req = createMockRequest('/', ['/', '/']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/'); + }); + + it('should return "/" when originalUrl is "/" and layers store contains single root path', () => { + const req = createMockRequest('/', ['/']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/'); + }); + + it('should return undefined when originalUrl is not root but all layers are root paths', () => { + const req = createMockRequest('/some/path', ['/', '/', '/']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, undefined); + }); + + it('should return undefined when originalUrl is not included in layer with only slashes', () => { + const req = createMockRequest('/different/api/users', ['/', '/']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, undefined); + }); + }); + + describe('Case: originalUrl not included in layers store array', () => { + it('should return undefined when originalUrl does not match any route pattern', () => { + const req = createMockRequest('/api/orders', [ + '/', + '/api/users', + '/api/products', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, undefined); + }); + + it('should return undefined when originalUrl is completely different from layers', () => { + const req = createMockRequest('/completely/different/path', [ + '/', + '/api', + '/users', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, undefined); + }); + + it('should return undefined when originalUrl is longer but does not start with constructed route', () => { + const req = createMockRequest('/different/api/users', [ + '/', + '/api', + '/users', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, undefined); + }); + + it('should return undefined when originalUrl is not included in layer with only slashes', () => { + const req = createMockRequest('/different/api/users', ['/', '/']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, undefined); + }); + }); + + describe('Exact URL matching', () => { + it('should return route when originalUrl exactly matches constructed route', () => { + const req = createMockRequest('/api/users', ['/', '/api', '/users']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/api/users'); + }); + + it('should return route when originalUrl starts with constructed route', () => { + const req = createMockRequest('/api/users/123', [ + '/', + '/api', + '/users', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/api/users'); + }); + }); + + describe('Parameterized routes', () => { + it('should handle route with parameter pattern when originalUrl matches the pattern', () => { + const req = createMockRequest('/toto/tata', ['/', '/toto', '/:id']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/toto/:id'); + }); + + it('should handle multiple parameter patterns', () => { + const req = createMockRequest('/users/123/posts/456', [ + '/', + '/users', + '/:userId', + '/posts', + '/:postId', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/users/:userId/posts/:postId'); + }); + + it('should handle nested parameter routes', () => { + const req = createMockRequest('/api/v1/users/123', [ + '/', + '/api', + '/v1', + '/users', + '/:id', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/api/v1/users/:id'); + }); + + it('should remove wildcard pattern inside route path', () => { + const req = createMockRequest('/static/images/logo.png', [ + '/', + '/static', + '/*', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/static'); + }); + + it('should handle general wildcard pattern', () => { + const req = createMockRequest('/foo/3', ['/', '*']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '*'); + }); + }); + + describe('Route normalization', () => { + it('should normalize routes with duplicate slashes', () => { + const req = createMockRequest('/api/users', ['/', '//api', '//users']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/api/users'); + }); + + it('should handle mixed slash patterns', () => { + const req = createMockRequest('/api/v1/users', [ + '/', + '/api/', + '/v1/', + '/users', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/api/v1/users'); + }); + }); + + describe('Edge cases', () => { + it('should filter out wildcard paths correctly', () => { + const req = createMockRequest('/api/users', [ + '/', + '/*', + '/api', + '/users', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/api/users'); + }); + + it('should handle empty meaningful paths after filtering', () => { + const req = createMockRequest('/test', ['/', '/*']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/'); + }); + + it('should handle layers store with only wildcards', () => { + const req = createMockRequest('/anything', ['/*', '/*']); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/'); + }); + + it('should handle complex nested routes', () => { + const req = createMockRequest('/api/v2/users/123/posts/456/comments', [ + '/', + '/api', + '/v2', + '/users', + '/:userId', + '/posts', + '/:postId', + '/comments', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual( + result, + '/api/v2/users/:userId/posts/:postId/comments' + ); + }); + }); + + describe('Query parameters and fragments', () => { + it('should handle originalUrl with query parameters', () => { + const req = createMockRequest('/api/users?page=1&limit=10', [ + '/', + '/api', + '/users', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/api/users'); + }); + + it('should handle originalUrl with hash fragments', () => { + const req = createMockRequest('/api/users#section1', [ + '/', + '/api', + '/users', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/api/users'); + }); + + it('should handle parameterized routes with query parameters', () => { + const req = createMockRequest('/users/123?include=profile', [ + '/', + '/users', + '/:id', + ]); + const result = utils.getActualMatchedRoute(req); + assert.strictEqual(result, '/users/:id'); + }); + }); + }); }); From 1df9019c49db2d7f0d23294b4b17e19cda8abfc9 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 23 May 2025 13:53:53 +0200 Subject: [PATCH 3/5] improve comments --- .../node/opentelemetry-instrumentation-express/src/utils.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index bd8f7f666b..8aad92cd3f 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -226,13 +226,12 @@ export function getConstructedRoute(req: { path => path !== '/' && path !== '/*' ); - // Handle standalone wildcard case if (meaningfulPaths.length === 1 && meaningfulPaths[0] === '*') { return '*'; } - // Construct the route by joining paths and normalizing - return meaningfulPaths.join('').replace(/\/{2,}/g, '/'); // Remove duplicate slashes + // Join parts and remove duplicate slashes + return meaningfulPaths.join('').replace(/\/{2,}/g, '/'); } /** @@ -257,6 +256,7 @@ export function getActualMatchedRoute(req: { } // Handle root path case - if all paths are root, only return root if originalUrl is also root + // The layer store also includes root paths in case a non-existing url was requested if (layersStore.every(path => path === '/')) { return req.originalUrl === '/' ? '/' : undefined; } From 006539c91303185234e34e630a7795fac239bfea Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Tue, 3 Jun 2025 14:47:04 +0200 Subject: [PATCH 4/5] consider regex route paths --- .../opentelemetry-instrumentation-express/src/utils.ts | 10 ++++++++++ .../test/express.test.ts | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index 8aad92cd3f..808d072a55 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -266,6 +266,16 @@ export function getActualMatchedRoute(req: { return constructedRoute; } + // For RegExp routes or route arrays, return the constructed route + // This handles the case where the route is defined using RegExp or an array + if (constructedRoute.includes('/') && + (constructedRoute.includes(',') || + constructedRoute.includes('\\') || + constructedRoute.includes('*') || + constructedRoute.includes('['))) { + return constructedRoute; + } + // Ensure route starts with '/' if it doesn't already const normalizedRoute = constructedRoute.startsWith('/') ? constructedRoute diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index b47d3f9d65..6199f9dc00 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -103,7 +103,7 @@ describe('ExpressInstrumentation', () => { for (const span of spans) { assert.strictEqual( - span.attributes[SEMATTRS_HTTP_ROUTE], + span.attributes[ATTR_HTTP_ROUTE], undefined, // none of the spans have the HTTP route attribute `Span "${span.name}" should not have HTTP route attribute for non-existing route` ); From 2cb552a574c16dbd785f46e38c8a3eee6af9a331 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Thu, 5 Jun 2025 12:13:51 +0200 Subject: [PATCH 5/5] run lint --- .../src/utils.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index 808d072a55..a667fc8d08 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -268,11 +268,13 @@ export function getActualMatchedRoute(req: { // For RegExp routes or route arrays, return the constructed route // This handles the case where the route is defined using RegExp or an array - if (constructedRoute.includes('/') && - (constructedRoute.includes(',') || - constructedRoute.includes('\\') || - constructedRoute.includes('*') || - constructedRoute.includes('['))) { + if ( + constructedRoute.includes('/') && + (constructedRoute.includes(',') || + constructedRoute.includes('\\') || + constructedRoute.includes('*') || + constructedRoute.includes('[')) + ) { return constructedRoute; }