From 7152206cc170b3fe3ecfa21973a9f537b9b6faf3 Mon Sep 17 00:00:00 2001 From: David Luna Date: Mon, 22 Apr 2024 19:12:06 +0200 Subject: [PATCH 1/6] fix(instr-express): keep hidden properties in router layer handle function --- .../src/instrumentation.ts | 25 ++++++++++++++-- .../test/express.test.ts | 29 ++++++++++++++++++- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 3df15aa699..d409717be4 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -165,8 +165,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, + instrumentation._applyPatch( layer, typeof args[0] === 'string' ? args[0] : undefined ); @@ -189,7 +188,8 @@ export class ExpressInstrumentation extends InstrumentationBase< this._wrap(layer, 'handle', (original: Function) => { // TODO: instrument error handlers if (original.length === 4) return original; - return function ( + + const patched = function ( this: ExpressLayer, req: PatchedRequest, res: express.Response @@ -326,6 +326,25 @@ 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() { + // @ts-expect-error -- the original function has hidden props indexed by strings + return original[key]; + }, + set(value) { + // @ts-expect-error -- the original function has hidden props indexed by strings + 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 f573a4a669..2b6af411b9 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -488,6 +488,32 @@ 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,7 +553,8 @@ describe('ExpressInstrumentation', () => { ); }); }); - it('should work with ESM usage', async () => { + + it.skip('should work with ESM usage', async () => { await testUtils.runTestFixture({ cwd: __dirname, argv: ['fixtures/use-express.mjs'], From 82b4b8369cdd03052b275308402d7fb5f37533ef Mon Sep 17 00:00:00 2001 From: David Luna Date: Mon, 22 Apr 2024 19:27:11 +0200 Subject: [PATCH 2/6] chore(instr-express): re-enable ESM test --- .../opentelemetry-instrumentation-express/test/express.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 2b6af411b9..4c72ba8ad2 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -554,7 +554,7 @@ describe('ExpressInstrumentation', () => { }); }); - it.skip('should work with ESM usage', async () => { + it('should work with ESM usage', async () => { await testUtils.runTestFixture({ cwd: __dirname, argv: ['fixtures/use-express.mjs'], From e35bc7b7e927b98354fed59131f355f25b589f86 Mon Sep 17 00:00:00 2001 From: David Luna Date: Tue, 23 Apr 2024 09:46:30 +0200 Subject: [PATCH 3/6] chore(instr-express): fix lint issues --- .../src/instrumentation.ts | 10 +++++----- .../test/express.test.ts | 7 +++++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index d409717be4..9fd5858e4d 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -188,7 +188,7 @@ export class ExpressInstrumentation extends InstrumentationBase< this._wrap(layer, 'handle', (original: Function) => { // TODO: instrument error handlers if (original.length === 4) return original; - + const patched = function ( this: ExpressLayer, req: PatchedRequest, @@ -329,9 +329,9 @@ 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 + // through through patched function // ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1950 - Object.keys(original).forEach((key) => { + Object.keys(original).forEach(key => { Object.defineProperty(patched, key, { get() { // @ts-expect-error -- the original function has hidden props indexed by strings @@ -340,8 +340,8 @@ export class ExpressInstrumentation extends InstrumentationBase< set(value) { // @ts-expect-error -- the original function has hidden props indexed by strings 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 4c72ba8ad2..6222ffb88a 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -496,7 +496,7 @@ describe('ExpressInstrumentation', () => { 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'); + routerLayer = stack.find(layer => layer.name === 'router'); return res.status(200).end('test'); }); }); @@ -510,7 +510,10 @@ describe('ExpressInstrumentation', () => { ); assert.strictEqual(response, 'test'); rootSpan.end(); - assert.ok(routerLayer?.handle?.stack?.length === 1, 'router layer stack is accessible'); + assert.ok( + routerLayer?.handle?.stack?.length === 1, + 'router layer stack is accessible' + ); } ); }); From 8f2f66c9e5f66070b920f5a674e5656b32f45c02 Mon Sep 17 00:00:00 2001 From: David Luna Date: Mon, 6 May 2024 18:30:54 +0200 Subject: [PATCH 4/6] chore(instr-express): fix lint issues --- .../src/instrumentation.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 3cbe45a5df..2b7e10762f 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -152,10 +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( - layer, - getLayerPath(args) - ); + instrumentation._applyPatch(layer, getLayerPath(args)); return route; }; }; From 618e80b0290a055afc15b25fbddedf7fc18da85f Mon Sep 17 00:00:00 2001 From: David Luna Date: Fri, 7 Jun 2024 12:44:02 +0200 Subject: [PATCH 5/6] chore(instr-expres): update internal types --- .../src/instrumentation.ts | 4 +--- .../src/internal-types.ts | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 7816008c47..007b7b9f4f 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -169,7 +169,7 @@ 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; @@ -318,11 +318,9 @@ export class ExpressInstrumentation extends InstrumentationBase { Object.keys(original).forEach(key => { Object.defineProperty(patched, key, { get() { - // @ts-expect-error -- the original function has hidden props indexed by strings return original[key]; }, set(value) { - // @ts-expect-error -- the original function has hidden props indexed by strings original[key] = value; }, }); diff --git a/plugins/node/opentelemetry-instrumentation-express/src/internal-types.ts b/plugins/node/opentelemetry-instrumentation-express/src/internal-types.ts index 29d1de2d8e..a661bf9de4 100644 --- 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; [kLayerPatched]?: boolean; name: string; params: { [key: string]: string }; From 686c669cc349865bb2ac960e4df27b00f505d2fb Mon Sep 17 00:00:00 2001 From: David Luna Date: Fri, 7 Jun 2024 13:47:54 +0200 Subject: [PATCH 6/6] chore(instr-express): fix lint issues --- .../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 007b7b9f4f..f15e8cb2ce 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -169,7 +169,7 @@ export class ExpressInstrumentation extends InstrumentationBase { if (layer[kLayerPatched] === true) return; layer[kLayerPatched] = true; - this._wrap(layer, 'handle', (original) => { + this._wrap(layer, 'handle', original => { // TODO: instrument error handlers if (original.length === 4) return original;