feat(instrumentation-express): Use router path in router span names

plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts

@@ -180,7 +180,7 @@ export class ExpressInstrumentation extends InstrumentationBase<ExpressInstrumen
         const attributes: Attributes = {
           [SEMATTRS_HTTP_ROUTE]: route.length > 0 ? route : '/',
         };
-        const metadata = getLayerMetadata(layer, layerPath);
+        const metadata = getLayerMetadata(route, layer, layerPath);
         const type = metadata.attributes[
           AttributeNames.EXPRESS_TYPE
         ] as ExpressLayerType;

plugins/node/opentelemetry-instrumentation-express/src/internal-types.ts

@@ -64,6 +64,7 @@ export type ExpressLayer = {
   params: { [key: string]: string };
   path: string;
   regexp: RegExp;
+  route?: ExpressLayer;
 };
 
 export type LayerMetadata = {

plugins/node/opentelemetry-instrumentation-express/src/utils.ts

@@ -44,33 +44,61 @@ export const storeLayerPath = (request: PatchedRequest, value?: string) => {
   (request[_LAYERS_STORE_PROPERTY] as string[]).push(value);
 };
 
+/**
+ * Recursively search the router path from layer stack
+ * @param path The path to reconstruct
+ * @param layer The layer to reconstruct from
+ * @returns The reconstructed path
+ */
+export const getRouterPath = (path: string, layer: ExpressLayer): string => {
+  const stackLayer = layer.handle?.stack?.[0];
+
+  if (stackLayer?.route?.path) {
+    return `${path}${stackLayer.route.path}`;
+  }
+
+  if (stackLayer?.handle?.stack) {
+    return getRouterPath(path, stackLayer);
+  }
+
+  return path;
+};
+
 /**
  * Parse express layer context to retrieve a name and attributes.
+ * @param route The route of the layer
  * @param layer Express layer
  * @param [layerPath] if present, the path on which the layer has been mounted
  */
 export const getLayerMetadata = (
+  route: string,
   layer: ExpressLayer,
   layerPath?: string
 ): {
   attributes: Attributes;
   name: string;
 } => {
   if (layer.name === 'router') {
+    const maybeRouterPath = getRouterPath('', layer);
+    const extractedRouterPath = maybeRouterPath
+      ? maybeRouterPath
+      : layerPath || route || '/';
+
     return {
       attributes: {
-        [AttributeNames.EXPRESS_NAME]: layerPath,
+        [AttributeNames.EXPRESS_NAME]: extractedRouterPath,
         [AttributeNames.EXPRESS_TYPE]: ExpressLayerType.ROUTER,
       },
-      name: `router - ${layerPath}`,
+      name: `router - ${extractedRouterPath}`,
     };
   } else if (layer.name === 'bound dispatch') {
     return {
       attributes: {
-        [AttributeNames.EXPRESS_NAME]: layerPath ?? 'request handler',
+        [AttributeNames.EXPRESS_NAME]:
+          (route || layerPath) ?? 'request handler',
         [AttributeNames.EXPRESS_TYPE]: ExpressLayerType.REQUEST_HANDLER,
       },
-      name: `request handler${layer.path ? ` - ${layerPath}` : ''}`,
+      name: `request handler${layer.path ? ` - ${route || layerPath}` : ''}`,
     };
   } else {
     return {
@@ -159,11 +187,13 @@ export const asErrorAndMessage = (
 export const getLayerPath = (
   args: [LayerPathSegment | LayerPathSegment[], ...unknown[]]
 ): string | undefined => {
-  if (Array.isArray(args[0])) {
-    return args[0].map(arg => extractLayerPathSegment(arg) || '').join(',');
+  const firstArg = args[0];
+
+  if (Array.isArray(firstArg)) {
+    return firstArg.map(arg => extractLayerPathSegment(arg) || '').join(',');
   }
 
-  return extractLayerPathSegment(args[0]);
+  return extractLayerPathSegment(firstArg);
 };
 
 const extractLayerPathSegment = (arg: LayerPathSegment) => {

plugins/node/opentelemetry-instrumentation-express/test/express.test.ts

@@ -641,6 +641,86 @@ describe('ExpressInstrumentation', () => {
     });
   });
 
+  it('should work with Express routers', async () => {
+    await testUtils.runTestFixture({
+      cwd: __dirname,
+      argv: ['fixtures/use-express-router.mjs'],
+      env: {
+        NODE_OPTIONS:
+          '--experimental-loader=@opentelemetry/instrumentation/hook.mjs',
+        NODE_NO_WARNINGS: '1',
+      },
+      checkResult: (err, stdout, stderr) => {
+        assert.ifError(err);
+      },
+      checkCollector: (collector: testUtils.TestCollector) => {
+        const spans = collector.sortedSpans;
+
+        assert.strictEqual(spans[0].name, 'GET');
+        assert.strictEqual(spans[0].kind, testUtils.OtlpSpanKind.CLIENT);
+        assert.strictEqual(spans[1].name, 'GET /api/user/:id');
+        assert.strictEqual(spans[1].kind, testUtils.OtlpSpanKind.SERVER);
+        assert.strictEqual(spans[1].parentSpanId, spans[0].spanId);
+        assert.strictEqual(spans[2].name, 'middleware - query');
+        assert.strictEqual(spans[3].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[3].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[4].name, 'middleware - simpleMiddleware');
+        assert.strictEqual(spans[4].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[4].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[5].name, 'router - /api/user/:id');
+        assert.strictEqual(spans[5].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[5].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[6].name, 'request handler - /api/user/:id');
+        assert.strictEqual(spans[6].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[6].parentSpanId, spans[1].spanId);
+      },
+    });
+  });
+
+  it('should work with nested Express routers', async () => {
+    await testUtils.runTestFixture({
+      cwd: __dirname,
+      argv: ['fixtures/use-express-nested-router.mjs'],
+      env: {
+        NODE_OPTIONS:
+          '--experimental-loader=@opentelemetry/instrumentation/hook.mjs',
+        NODE_NO_WARNINGS: '1',
+      },
+      checkResult: (err, stdout, stderr) => {
+        assert.ifError(err);
+      },
+      checkCollector: (collector: testUtils.TestCollector) => {
+        const spans = collector.sortedSpans;
+
+        assert.strictEqual(spans[0].name, 'GET');
+        assert.strictEqual(spans[0].kind, testUtils.OtlpSpanKind.CLIENT);
+        assert.strictEqual(spans[1].name, 'GET /api/user/:id/posts/:postId');
+        assert.strictEqual(spans[1].kind, testUtils.OtlpSpanKind.SERVER);
+        assert.strictEqual(spans[2].name, 'middleware - query');
+        assert.strictEqual(spans[2].kind, testUtils.OtlpSpanKind.INTERNAL);
+        assert.strictEqual(spans[2].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[3].name, 'middleware - expressInit');
+        assert.strictEqual(spans[3].kind, testUtils.OtlpSpanKind.INTERNAL);
+        assert.strictEqual(spans[3].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[4].name, 'middleware - simpleMiddleware');
+        assert.strictEqual(spans[4].kind, testUtils.OtlpSpanKind.INTERNAL);
+        assert.strictEqual(spans[4].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[5].name, 'router - /api/user/:id');
+        assert.strictEqual(spans[5].kind, testUtils.OtlpSpanKind.INTERNAL);
+        assert.strictEqual(spans[5].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[6].name, 'router - /:postId');
+        assert.strictEqual(spans[6].kind, testUtils.OtlpSpanKind.INTERNAL);
+        assert.strictEqual(spans[6].parentSpanId, spans[1].spanId);
+        assert.strictEqual(
+          spans[7].name,
+          'request handler - /api/user/:id/posts/:postId'
+        );
+        assert.strictEqual(spans[7].kind, testUtils.OtlpSpanKind.INTERNAL);
+        assert.strictEqual(spans[7].parentSpanId, spans[1].spanId);
+      },
+    });
+  });
+
   it('should set a correct transaction name for routes specified in RegEx', async () => {
     await testUtils.runTestFixture({
       cwd: __dirname,

plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-nested-router.mjs

@@ -0,0 +1,81 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { promisify } from 'util';
+import { createTestNodeSdk } from '@opentelemetry/contrib-test-utils';
+
+import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
+import { ExpressInstrumentation } from '../../build/src/index.js';
+
+const sdk = createTestNodeSdk({
+  serviceName: 'use-express-nested',
+  instrumentations: [
+    new HttpInstrumentation(),
+    new ExpressInstrumentation()
+  ]
+})
+
+sdk.start();
+
+import express from 'express';
+import * as http from 'http';
+
+const app = express();
+
+app.use(async function simpleMiddleware(req, res, next) {
+  // Wait a short delay to ensure this "middleware - ..." span clearly starts
+  // before the "router - ..." span. The test rely on a start-time-based sort
+  // of the produced spans. If they start in the same millisecond, then tests
+  // can be flaky.
+  await promisify(setTimeout)(10);
+  next();
+});
+
+const userRouter = express.Router();
+const postsRouter = express.Router();
+
+postsRouter.get('/:postId', (req, res, next) => {
+  res.json({ hello: 'yes' });
+  res.end();
+  next();
+});
+
+userRouter.get('/api/user/:id', (req, res, next) => {
+  res.json({ hello: 'yes' });
+  res.end();
+  next();
+});
+
+userRouter.use('/api/user/:id/posts', postsRouter);
+
+app.use(userRouter);
+
+const server = http.createServer(app);
+await new Promise(resolve => server.listen(0, resolve));
+const port = server.address().port;
+
+
+await new Promise(resolve => {
+  http.get(`http://localhost:${port}/api/user/123/posts/321`, (res) => {
+    res.resume();
+    res.on('end', data => {
+      resolve(data);
+    });
+  })
+});
+
+await new Promise(resolve => server.close(resolve));
+await sdk.shutdown();

plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-router.mjs

@@ -0,0 +1,72 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { promisify } from 'util';
+import { createTestNodeSdk } from '@opentelemetry/contrib-test-utils';
+
+import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
+import { ExpressInstrumentation } from '../../build/src/index.js';
+
+const sdk = createTestNodeSdk({
+  serviceName: 'use-express-nested',
+  instrumentations: [
+    new HttpInstrumentation(),
+    new ExpressInstrumentation()
+  ]
+})
+
+sdk.start();
+
+import express from 'express';
+import * as http from 'http';
+
+const app = express();
+
+app.use(async function simpleMiddleware(req, res, next) {
+  // Wait a short delay to ensure this "middleware - ..." span clearly starts
+  // before the "router - ..." span. The test rely on a start-time-based sort
+  // of the produced spans. If they start in the same millisecond, then tests
+  // can be flaky.
+  await promisify(setTimeout)(10);
+  next();
+});
+
+const router = express.Router();
+
+router.get('/api/user/:id', (req, res, next) => {
+  res.json({ hello: 'yes' });
+  res.end();
+  next();
+});
+
+app.use(router);
+
+const server = http.createServer(app);
+await new Promise(resolve => server.listen(0, resolve));
+const port = server.address().port;
+
+
+await new Promise(resolve => {
+  http.get(`http://localhost:${port}/api/user/123`, (res) => {
+    res.resume();
+    res.on('end', data => {
+      resolve(data);
+    });
+  })
+});
+
+await new Promise(resolve => server.close(resolve));
+await sdk.shutdown();