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

…2319) This PR adds a new utility (getRouterPath) to recursively search the (possibly nested) parameterised router path when getting the layer metadata. This only affects the span name, not the http.route attribute of the span. http.route is still showing the mount path. (Active discussion on this: Express instrumentation does not properly handle router usage #1993 (comment)) Uses route primarily to assign the requestHandler span name instead of layerPath. layerPath does not represent the full path, only the path of the last matching router when nested routers are used. Co-authored-by: Abhijeet Prasad <[email protected]> Co-authored-by: Jamie Danielson <[email protected]> Co-authored-by: Jamie Danielson <[email protected]>
open-telemetry · Sep 17, 2024 · ee5c584 · ee5c584
1 parent aa9710f
commit ee5c584
Show file tree

Hide file tree

Showing 7 changed files with 320 additions and 9 deletions.
diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/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;

diff --git a/plugins/node/opentelemetry-instrumentation-express/src/internal-types.ts b/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 = {

diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/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) => {

diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/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, 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, 'request handler - /api/user/:id');
+        assert.strictEqual(spans[6].kind, testUtils.OtlpSpanKind.INTERNAL);
+        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,

diff --git a/...ns/node/opentelemetry-instrumentation-express/test/fixtures/use-express-nested-router.mjs b/...ns/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();
diff --git a/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-router.mjs b/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();