diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 122427809a..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,23 +190,21 @@ export class ExpressInstrumentation extends InstrumentationBase path !== '/' && path !== '/*') - .join('') - // remove duplicate slashes to normalize route - .replace(/\/{2,}/g, '/'); + + const constructedRoute = getConstructedRoute(req); + const actualMatchedRoute = getActualMatchedRoute(req); const attributes: Attributes = { - [ATTR_HTTP_ROUTE]: route.length > 0 ? route : '/', + [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 = route || '/'; + rpcMetadata.route = actualMatchedRoute; } // verify against the config if the layer should be ignored @@ -223,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..a667fc8d08 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -213,3 +213,93 @@ 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 !== '/*' + ); + + if (meaningfulPaths.length === 1 && meaningfulPaths[0] === '*') { + return '*'; + } + + // Join parts and remove duplicate slashes + return meaningfulPaths.join('').replace(/\/{2,}/g, '/'); +} + +/** + * 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 + // The layer store also includes root paths in case a non-existing url was requested + if (layersStore.every(path => path === '/')) { + return req.originalUrl === '/' ? '/' : undefined; + } + + const constructedRoute = getConstructedRoute(req); + if (constructedRoute === '*') { + 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 + : `/${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 6ccd795159..6199f9dc00 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -67,6 +67,50 @@ describe('ExpressInstrumentation', () => { 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[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` + ); + } + } + ); + }); it('should create a child span for middlewares', async () => { const rootSpan = tracer.startSpan('rootSpan'); const customMiddleware: express.RequestHandler = (req, res, next) => { 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'); + }); + }); + }); });