From 1929419013f12009b12731e6d00c6316595248b4 Mon Sep 17 00:00:00 2001 From: David Luna Date: Fri, 21 Jun 2024 20:10:17 +0200 Subject: [PATCH 1/4] fix(instr-express): fix handler patching for already patched router --- .../src/instrumentation.ts | 9 +++-- .../test/express.test.ts | 37 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index c3f5602cac..816b388adb 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/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 propeties + // 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 (let 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; }); } diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index e13c13a424..691b7d3c30 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -517,6 +517,43 @@ 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) => 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 has now express router's own properties in its prototype so + // they are not acessible 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', () => { From 770267f3ca08b923f7b2a01be6db58f692c0eca9 Mon Sep 17 00:00:00 2001 From: David Luna Date: Sat, 22 Jun 2024 17:21:02 +0200 Subject: [PATCH 2/4] Update plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts Co-authored-by: Abhijeet Prasad --- .../src/instrumentation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 816b388adb..a20d195b1c 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -315,7 +315,7 @@ 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 - // Also some apps/libs do their own patching before OTEL and have these propeties + // 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 From db19a982b358826c3c2616654b7d9ceb7ea132e3 Mon Sep 17 00:00:00 2001 From: David Luna Date: Mon, 1 Jul 2024 18:35:51 +0200 Subject: [PATCH 3/4] chore(instr-express): fix lint issues --- .../src/instrumentation.ts | 2 +- .../test/express.test.ts | 16 ++++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index a20d195b1c..7aeb400ab2 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -319,7 +319,7 @@ export class ExpressInstrumentation extends InstrumentationBase { // 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 (let key in original) { + for (const key in original) { Object.defineProperty(patched, key, { get() { return original[key]; diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 691b7d3c30..fba0b08a43 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -521,19 +521,23 @@ 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) => void = (req, res, next) => router(req, res, next); + const CustomRouter: (...p: Parameters) => 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'); + const stack = req.app._router.stack as any[]; + routerLayer = stack.find(router => router.name === 'CustomRouter'); + return res.status(200).end('bar'); }); // The patched router has now express router's own properties in its prototype so // they are not acessible through `Object.keys(...)` // https://github.com/TryGhost/Ghost/blob/fefb9ec395df8695d06442b6ecd3130dae374d94/ghost/core/core/frontend/web/site.js#L192 - Object.setPrototypeOf(CustomRouter, router) + Object.setPrototypeOf(CustomRouter, router); expressApp.use(CustomRouter); const httpServer = await createServer(expressApp); From ce0f46578de5132f947f17faf7db19c10e7dd21e Mon Sep 17 00:00:00 2001 From: Jamie Danielson Date: Tue, 9 Jul 2024 19:06:04 -0400 Subject: [PATCH 4/4] Update comments --- .../test/express.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index fba0b08a43..43b5da1324 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -534,8 +534,8 @@ describe('ExpressInstrumentation', () => { routerLayer = stack.find(router => router.name === 'CustomRouter'); return res.status(200).end('bar'); }); - // The patched router has now express router's own properties in its prototype so - // they are not acessible through `Object.keys(...)` + // 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);