From a49e9d7b558f428a6a1be9bc23d5435357d0d718 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tam=C3=A1s=20Rig=C3=B3?= <tamas.rigo@dakai.io>
Date: Mon, 27 Jan 2025 19:18:28 +0100
Subject: [PATCH] fix(express): span name if middleware on nested router is
 used

---
 .../src/instrumentation.ts                            |  4 ++--
 .../src/utils.ts                                      | 10 ++++++++--
 .../test/express.test.ts                              | 11 +++++++----
 .../test/fixtures/use-express-nested-router.mjs       | 10 +++++++++-
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts
index 2e283158b8..465fda085b 100644
--- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts
+++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts
@@ -171,7 +171,7 @@ export class ExpressInstrumentation extends InstrumentationBase<ExpressInstrumen
         req: PatchedRequest,
         res: express.Response
       ) {
-        storeLayerPath(req, layerPath);
+        const { isLayerPathStored } = storeLayerPath(req, layerPath);
         const route = (req[_LAYERS_STORE_PROPERTY] as string[])
           .filter(path => path !== '/' && path !== '/*')
           .join('')
@@ -274,7 +274,7 @@ export class ExpressInstrumentation extends InstrumentationBase<ExpressInstrumen
               req.res?.removeListener('finish', onResponseFinish);
               span.end();
             }
-            if (!(req.route && isError)) {
+            if (!(req.route && isError) && isLayerPathStored) {
               (req[_LAYERS_STORE_PROPERTY] as string[]).pop();
             }
             const callback = args[callbackIdx] as Function;
diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts
index fdc8f3728c..325ce4eee1 100644
--- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts
+++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts
@@ -33,15 +33,21 @@ import {
  * @param request The request where
  * @param [value] the value to push into the array
  */
-export const storeLayerPath = (request: PatchedRequest, value?: string) => {
+export const storeLayerPath = (
+  request: PatchedRequest,
+  value?: string
+): { isLayerPathStored: boolean } => {
   if (Array.isArray(request[_LAYERS_STORE_PROPERTY]) === false) {
     Object.defineProperty(request, _LAYERS_STORE_PROPERTY, {
       enumerable: false,
       value: [],
     });
   }
-  if (value === undefined) return;
+  if (value === undefined) return { isLayerPathStored: false };
+
   (request[_LAYERS_STORE_PROPERTY] as string[]).push(value);
+
+  return { isLayerPathStored: true };
 };
 
 /**
diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts
index 13806b2698..a507e75647 100644
--- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts
+++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts
@@ -709,15 +709,18 @@ describe('ExpressInstrumentation', () => {
         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].name, 'router - /api/user/:id/posts');
         assert.strictEqual(spans[6].kind, testUtils.OtlpSpanKind.INTERNAL);
         assert.strictEqual(spans[6].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[7].name, 'middleware - simpleMiddleware2');
+        assert.strictEqual(spans[7].kind, testUtils.OtlpSpanKind.INTERNAL);
+        assert.strictEqual(spans[7].parentSpanId, spans[1].spanId);
         assert.strictEqual(
-          spans[7].name,
+          spans[8].name,
           'request handler - /api/user/:id/posts/:postId'
         );
-        assert.strictEqual(spans[7].kind, testUtils.OtlpSpanKind.INTERNAL);
-        assert.strictEqual(spans[7].parentSpanId, spans[1].spanId);
+        assert.strictEqual(spans[8].kind, testUtils.OtlpSpanKind.INTERNAL);
+        assert.strictEqual(spans[8].parentSpanId, spans[1].spanId);
       },
     });
   });
diff --git a/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-nested-router.mjs b/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-nested-router.mjs
index 98ba0bf236..6f86eb18e5 100644
--- a/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-nested-router.mjs
+++ b/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-nested-router.mjs
@@ -46,6 +46,14 @@ app.use(async function simpleMiddleware(req, res, next) {
 
 const userRouter = express.Router();
 const postsRouter = express.Router();
+postsRouter.use(async function simpleMiddleware2(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();
+});
 
 postsRouter.get('/:postId', (req, res, next) => {
   res.json({ hello: 'yes' });
@@ -74,7 +82,7 @@ await new Promise(resolve => {
     res.on('end', data => {
       resolve(data);
     });
-  })
+  });
 });
 
 await new Promise(resolve => server.close(resolve));