fix(express): Ensure 404 routes don't attach route attribute (#2843)

Author: s1gr1d

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<ExpressInstrumen
         res: express.Response
       ) {
         const { isLayerPathStored } = storeLayerPath(req, layerPath);
-        const route = (req[_LAYERS_STORE_PROPERTY] as string[])
-          .filter(path => 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<ExpressInstrumen
           {
             request: req,
             layerType: type,
-            route,
+            route: constructedRoute,
           },
           metadata.name
         );
@@ -241,7 +241,7 @@ export class ExpressInstrumentation extends InstrumentationBase<ExpressInstrumen
               requestHook(span, {
                 request: req,
                 layerType: type,
-                route,
+                route: constructedRoute,
               }),
             e => {
               if (e) {

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('*');
+}

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) => {