fix(instr-express): fix handler patching for already patched router (#2294)

* fix(instr-express): fix handler patching for already patched router

---------

Co-authored-by: Abhijeet Prasad <[email protected]>
Co-authored-by: Jamie Danielson <[email protected]>

Author: 3 people

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

@@ -315,7 +315,11 @@ export class ExpressInstrumentation extends InstrumentationBase {
       // 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 => {
+      // Also some apps/libs do their own patching before OTEL and have these properties
+      // in the proptotype. So we use a `for...in` loop to get own properties and also
+      // any enumerable prop in the prototype chain
+      // ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2271
+      for (const key in original) {
         Object.defineProperty(patched, key, {
           get() {
             return original[key];
@@ -324,8 +328,7 @@ export class ExpressInstrumentation extends InstrumentationBase {
             original[key] = value;
           },
         });
-      });
-
+      }
       return patched;
     });
   }

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

@@ -517,6 +517,47 @@ describe('ExpressInstrumentation', () => {
         }
       );
     });
+
+    it('should keep the handle properties even if router is patched before instrumentation does it', async () => {
+      const rootSpan = tracer.startSpan('rootSpan');
+      let routerLayer: { name: string; handle: { stack: any[] } };
+
+      const expressApp = express();
+      const router = express.Router();
+      const CustomRouter: (...p: Parameters<typeof router>) => void = (
+        req,
+        res,
+        next
+      ) => router(req, res, next);
+      router.use('/:slug', (req, res, next) => {
+        const stack = req.app._router.stack as any[];
+        routerLayer = stack.find(router => router.name === 'CustomRouter');
+        return res.status(200).end('bar');
+      });
+      // The patched router now has express router's own properties in its prototype so
+      // they are not accessible through `Object.keys(...)`
+      // https://github.com/TryGhost/Ghost/blob/fefb9ec395df8695d06442b6ecd3130dae374d94/ghost/core/core/frontend/web/site.js#L192
+      Object.setPrototypeOf(CustomRouter, router);
+      expressApp.use(CustomRouter);
+
+      const httpServer = await createServer(expressApp);
+      server = httpServer.server;
+      port = httpServer.port;
+      await context.with(
+        trace.setSpan(context.active(), rootSpan),
+        async () => {
+          const response = await httpRequest.get(
+            `http://localhost:${port}/foo`
+          );
+          assert.strictEqual(response, 'bar');
+          rootSpan.end();
+          assert.ok(
+            routerLayer.handle.stack.length === 1,
+            'router layer stack is accessible'
+          );
+        }
+      );
+    });
   });
 
   describe('Disabling plugin', () => {