fix(express): make rpcMetadata.route capture the last layer even when…

… if the last layer is not REQUEST_HANDLER (#1620)

* fix(express): make rpcMetadata.route capture the last layer even when if the last layer is mot REQUEST_HANDLER

* fix lint issue

* remove test.only

* revert code to change ignore order

* update test

* remove comment related to update span name

* Move rpcRoute.metadata calculation logic up

* Add more test

* Fix lint

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

@@ -186,6 +186,7 @@ export class ExpressInstrumentation extends InstrumentationBase<
         const route = (req[_LAYERS_STORE_PROPERTY] as string[])
           .filter(path => path !== '/' && path !== '/*')
           .join('');
+
         const attributes: SpanAttributes = {
           [SemanticAttributes.HTTP_ROUTE]: route.length > 0 ? route : '/',
         };
@@ -194,13 +195,8 @@ export class ExpressInstrumentation extends InstrumentationBase<
           AttributeNames.EXPRESS_TYPE
         ] as ExpressLayerType;
 
-        // Rename the root http span in case we haven't done it already
-        // once we reach the request handler
         const rpcMetadata = getRPCMetadata(context.active());
-        if (
-          type === ExpressLayerType.REQUEST_HANDLER &&
-          rpcMetadata?.type === RPCType.HTTP
-        ) {
+        if (rpcMetadata?.type === RPCType.HTTP) {
           rpcMetadata.route = route || '/';
         }
 
@@ -211,6 +207,7 @@ export class ExpressInstrumentation extends InstrumentationBase<
           }
           return original.apply(this, arguments);
         }
+
         if (trace.getSpan(context.active()) === undefined) {
           return original.apply(this, arguments);
         }
@@ -259,6 +256,7 @@ export class ExpressInstrumentation extends InstrumentationBase<
             span.end();
           }
         };
+
         // verify we have a callback
         const args = Array.from(arguments);
         const callbackIdx = args.findIndex(arg => typeof arg === 'function');

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

@@ -275,6 +275,97 @@ describe('ExpressInstrumentation', () => {
       assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
       assert.strictEqual(res, 'test');
     });
+
+    it('should update rpcMetadata.route with the bare middleware layer', async () => {
+      const rootSpan = tracer.startSpan('rootSpan');
+      let rpcMetadata: RPCMetadata | undefined;
+      const httpServer = await serverWithMiddleware(tracer, rootSpan, app => {
+        app.use(express.json());
+        app.use((req, res, next) => {
+          rpcMetadata = getRPCMetadata(context.active());
+          next();
+        });
+
+        app.use('/bare_middleware', (req, res) => {
+          return res.status(200).end('test');
+        });
+      });
+      server = httpServer.server;
+      port = httpServer.port;
+      await context.with(
+        trace.setSpan(context.active(), rootSpan),
+        async () => {
+          const response = await httpRequest.get(
+            `http://localhost:${port}/bare_middleware/ignore_route_segment`
+          );
+          assert.strictEqual(response, 'test');
+          rootSpan.end();
+          assert.strictEqual(rpcMetadata?.route, '/bare_middleware');
+        }
+      );
+    });
+
+    it('should update rpcMetadata.route with the latest middleware layer', async () => {
+      const rootSpan = tracer.startSpan('rootSpan');
+      let rpcMetadata: RPCMetadata | undefined;
+      const httpServer = await serverWithMiddleware(tracer, rootSpan, app => {
+        app.use(express.json());
+        app.use((req, res, next) => {
+          rpcMetadata = getRPCMetadata(context.active());
+          next();
+        });
+
+        const router = express.Router();
+
+        app.use('/router', router);
+
+        router.use('/router_middleware', (req, res) => {
+          return res.status(200).end('test');
+        });
+      });
+      server = httpServer.server;
+      port = httpServer.port;
+      await context.with(
+        trace.setSpan(context.active(), rootSpan),
+        async () => {
+          const response = await httpRequest.get(
+            `http://localhost:${port}/router/router_middleware/ignore_route_segment`
+          );
+          assert.strictEqual(response, 'test');
+          rootSpan.end();
+          assert.strictEqual(rpcMetadata?.route, '/router/router_middleware');
+        }
+      );
+    });
+
+    it('should update rpcMetadata.route with the bare request handler layer', async () => {
+      const rootSpan = tracer.startSpan('rootSpan');
+      let rpcMetadata: RPCMetadata | undefined;
+      const httpServer = await serverWithMiddleware(tracer, rootSpan, app => {
+        app.use(express.json());
+        app.use((req, res, next) => {
+          rpcMetadata = getRPCMetadata(context.active());
+          next();
+        });
+
+        app.get('/bare_route', (req, res) => {
+          return res.status(200).end('test');
+        });
+      });
+      server = httpServer.server;
+      port = httpServer.port;
+      await context.with(
+        trace.setSpan(context.active(), rootSpan),
+        async () => {
+          const response = await httpRequest.get(
+            `http://localhost:${port}/bare_route`
+          );
+          assert.strictEqual(response, 'test');
+          rootSpan.end();
+          assert.strictEqual(rpcMetadata?.route, '/bare_route');
+        }
+      );
+    });
   });
 
   describe('Disabling plugin', () => {

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

@@ -127,7 +127,7 @@ describe('ExpressInstrumentation', () => {
       );
     });
 
-    it('rpcMetadata.route should be modified to /todo/:id', async () => {
+    it('rpcMetadata.route still capture correct route', async () => {
       assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
       await context.with(
         trace.setSpan(context.active(), rootSpan),