fix(instr-express): keep hidden properties in layer handlers (#2137)

* fix(instr-express): keep hidden properties in router layer handle function * chore(instr-express): re-enable ESM test * chore(instr-express): fix lint issues * chore(instr-express): fix lint issues * chore(instr-expres): update internal types * chore(instr-express): fix lint issues --------- Co-authored-by: Marc Pichler <[email protected]>
open-telemetry · Jun 10, 2024 · ce5f48d · ce5f48d
1 parent 93776fa
commit ce5f48d
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 8 deletions.
diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts
@@ -152,11 +152,7 @@ export class ExpressInstrumentation extends InstrumentationBase {
       ) {
         const route = original.apply(this, args);
         const layer = this._router.stack[this._router.stack.length - 1];
-        instrumentation._applyPatch.call(
-          instrumentation,
-          layer,
-          getLayerPath(args)
-        );
+        instrumentation._applyPatch(layer, getLayerPath(args));
         return route;
       };
     };
@@ -173,10 +169,11 @@ export class ExpressInstrumentation extends InstrumentationBase {
     if (layer[kLayerPatched] === true) return;
     layer[kLayerPatched] = true;
 
-    this._wrap(layer, 'handle', (original: Function) => {
+    this._wrap(layer, 'handle', original => {
       // TODO: instrument error handlers
       if (original.length === 4) return original;
-      return function (
+
+      const patched = function (
         this: ExpressLayer,
         req: PatchedRequest,
         res: express.Response
@@ -313,6 +310,23 @@ export class ExpressInstrumentation extends InstrumentationBase {
           }
         }
       };
+
+      // `handle` isn't just a regular function in some cases. It also contains
+      // some properties holding metadata and state so we need to proxy them
+      // through through patched function
+      // ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1950
+      Object.keys(original).forEach(key => {
+        Object.defineProperty(patched, key, {
+          get() {
+            return original[key];
+          },
+          set(value) {
+            original[key] = value;
+          },
+        });
+      });
+
+      return patched;
     });
   }
 

diff --git a/plugins/node/opentelemetry-instrumentation-express/src/internal-types.ts b/plugins/node/opentelemetry-instrumentation-express/src/internal-types.ts
@@ -58,7 +58,7 @@ export type ExpressRouter = {
 
 // https://github.com/expressjs/express/blob/main/lib/router/layer.js#L33
 export type ExpressLayer = {
-  handle: Function;
+  handle: Function & Record<string, any>;
   [kLayerPatched]?: boolean;
   name: string;
   params: { [key: string]: string };

diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts
@@ -488,6 +488,35 @@ describe('ExpressInstrumentation', () => {
         }
       );
     });
+
+    it('should keep stack in the router layer handle', async () => {
+      const rootSpan = tracer.startSpan('rootSpan');
+      let routerLayer: { name: string; handle: { stack: any[] } };
+      const httpServer = await serverWithMiddleware(tracer, rootSpan, app => {
+        app.use(express.json());
+        app.get('/bare_route', (req, res) => {
+          const stack = req.app._router.stack as any[];
+          routerLayer = stack.find(layer => layer.name === 'router');
+          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.ok(
+            routerLayer?.handle?.stack?.length === 1,
+            'router layer stack is accessible'
+          );
+        }
+      );
+    });
   });
 
   describe('Disabling plugin', () => {
@@ -527,6 +556,7 @@ describe('ExpressInstrumentation', () => {
       );
     });
   });
+
   it('should work with ESM usage', async () => {
     await testUtils.runTestFixture({
       cwd: __dirname,