From f144c027144ec7ecf08c6901008f2d65b53fba77 Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Tue, 1 Aug 2023 15:34:00 +0700 Subject: [PATCH 1/9] fix(express): make rpcMetadata.route capture the last layer even when if the last layer is mot REQUEST_HANDLER --- .../src/instrumentation.ts | 34 ++++++++----------- .../test/express.test.ts | 33 ++++++++++++++++++ 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 0c27a8a88f..706f0d6a5d 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -182,6 +182,20 @@ export class ExpressInstrumentation extends InstrumentationBase< req: PatchedRequest, res: express.Response ) { + if (trace.getSpan(context.active()) === undefined) { + return original.apply(this, arguments); + } + + const metadata = getLayerMetadata(layer, layerPath); + const type = metadata.attributes[ + AttributeNames.EXPRESS_TYPE + ] as ExpressLayerType; + + // verify against the config if the layer should be ignored + if (isLayerIgnored(metadata.name, type, instrumentation._config)) { + return original.apply(this, arguments); + } + storeLayerPath(req, layerPath); const route = (req[_LAYERS_STORE_PROPERTY] as string[]) .filter(path => path !== '/' && path !== '/*') @@ -189,32 +203,14 @@ export class ExpressInstrumentation extends InstrumentationBase< const attributes: SpanAttributes = { [SemanticAttributes.HTTP_ROUTE]: route.length > 0 ? route : '/', }; - const metadata = getLayerMetadata(layer, layerPath); - const type = metadata.attributes[ - 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 || '/'; } - // verify against the config if the layer should be ignored - if (isLayerIgnored(metadata.name, type, instrumentation._config)) { - if (type === ExpressLayerType.MIDDLEWARE) { - (req[_LAYERS_STORE_PROPERTY] as string[]).pop(); - } - return original.apply(this, arguments); - } - if (trace.getSpan(context.active()) === undefined) { - return original.apply(this, arguments); - } - const spanName = instrumentation._getSpanName( { request: req, diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 79b5f29488..383679e239 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -275,6 +275,39 @@ describe('ExpressInstrumentation', () => { assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); assert.strictEqual(res, 'test'); }); + + it.only('should update rpcMetadata.route with the latest middleware layer', async () => { + const rootSpan = tracer.startSpan('rootSpan'); + let finishListenerCount: number | undefined; + let rpcMetadata: RPCMetadata | undefined; + const httpServer = await serverWithMiddleware(tracer, rootSpan, app => { + app.use(express.json()); + app.use((req, res, next) => { + rpcMetadata = getRPCMetadata(context.active()); + res.on('finish', () => { + finishListenerCount = res.listenerCount('finish'); + }); + 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` + ); + assert.strictEqual(response, 'test'); + rootSpan.end(); + assert.strictEqual(rpcMetadata?.route, '/bare_middleware'); + } + ); + }); }); describe('Disabling plugin', () => { From 6d15f522a6e8ae13276e1d364ad9d95060642ecd Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Wed, 2 Aug 2023 21:05:29 +0700 Subject: [PATCH 2/9] fix lint issue --- .../test/express.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 383679e239..362e6c5f4e 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -289,13 +289,13 @@ describe('ExpressInstrumentation', () => { }); next(); }); - - app.use("/bare_middleware", (req, res) => { + + app.use('/bare_middleware', (req, res) => { return res.status(200).end('test'); }); }); server = httpServer.server; - port = httpServer.port; + port = httpServer.port; await context.with( trace.setSpan(context.active(), rootSpan), async () => { From b4f26f249514e8d19cfb6d924a6502b49e39b629 Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Wed, 2 Aug 2023 21:07:11 +0700 Subject: [PATCH 3/9] remove test.only --- .../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 362e6c5f4e..6582e59352 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -276,7 +276,7 @@ describe('ExpressInstrumentation', () => { assert.strictEqual(res, 'test'); }); - it.only('should update rpcMetadata.route with the latest middleware layer', async () => { + it('should update rpcMetadata.route with the latest middleware layer', async () => { const rootSpan = tracer.startSpan('rootSpan'); let finishListenerCount: number | undefined; let rpcMetadata: RPCMetadata | undefined; From 259d49cfdde695a9dd7398ba321a412263a8a5f3 Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Thu, 3 Aug 2023 09:35:37 +0700 Subject: [PATCH 4/9] revert code to change ignore order --- .../src/instrumentation.ts | 36 ++++++++++--------- .../test/ignore-all.test.ts | 4 +-- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 706f0d6a5d..cf0af4f0e3 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -182,10 +182,14 @@ export class ExpressInstrumentation extends InstrumentationBase< req: PatchedRequest, res: express.Response ) { - if (trace.getSpan(context.active()) === undefined) { - return original.apply(this, arguments); - } + storeLayerPath(req, layerPath); + const route = (req[_LAYERS_STORE_PROPERTY] as string[]) + .filter(path => path !== '/' && path !== '/*') + .join(''); + const attributes: SpanAttributes = { + [SemanticAttributes.HTTP_ROUTE]: route.length > 0 ? route : '/', + }; const metadata = getLayerMetadata(layer, layerPath); const type = metadata.attributes[ AttributeNames.EXPRESS_TYPE @@ -193,22 +197,14 @@ export class ExpressInstrumentation extends InstrumentationBase< // verify against the config if the layer should be ignored if (isLayerIgnored(metadata.name, type, instrumentation._config)) { + if (type === ExpressLayerType.MIDDLEWARE) { + (req[_LAYERS_STORE_PROPERTY] as string[]).pop(); + } return original.apply(this, arguments); } - storeLayerPath(req, layerPath); - const route = (req[_LAYERS_STORE_PROPERTY] as string[]) - .filter(path => path !== '/' && path !== '/*') - .join(''); - const attributes: SpanAttributes = { - [SemanticAttributes.HTTP_ROUTE]: route.length > 0 ? route : '/', - }; - - // 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 (rpcMetadata?.type === RPCType.HTTP) { - rpcMetadata.route = route || '/'; + if (trace.getSpan(context.active()) === undefined) { + return original.apply(this, arguments); } const spanName = instrumentation._getSpanName( @@ -255,6 +251,14 @@ export class ExpressInstrumentation extends InstrumentationBase< span.end(); } }; + + // 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 (rpcMetadata?.type === RPCType.HTTP) { + rpcMetadata.route = route || '/'; + } + // verify we have a callback const args = Array.from(arguments); const callbackIdx = args.findIndex(arg => typeof arg === 'function'); diff --git a/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts index 92c7c7046b..277b7ea68d 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts @@ -127,14 +127,14 @@ describe('ExpressInstrumentation', () => { ); }); - it('rpcMetadata.route should be modified to /todo/:id', async () => { + it('rpcMetadata.route should be undefined', async () => { assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); await context.with( trace.setSpan(context.active(), rootSpan), async () => { await httpRequest.get(`http://localhost:${port}/toto/tata`); rootSpan.end(); - assert.strictEqual(rpcMetadata.route, '/toto/:id'); + assert.strictEqual(rpcMetadata.route, undefined); } ); }); From 1ee99d1bbb7d3e91d33920146dccadf7d559d811 Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Thu, 3 Aug 2023 09:41:40 +0700 Subject: [PATCH 5/9] update test --- .../test/express.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 6582e59352..1a820a2a07 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -278,15 +278,11 @@ describe('ExpressInstrumentation', () => { it('should update rpcMetadata.route with the latest middleware layer', async () => { const rootSpan = tracer.startSpan('rootSpan'); - let finishListenerCount: number | undefined; let rpcMetadata: RPCMetadata | undefined; const httpServer = await serverWithMiddleware(tracer, rootSpan, app => { app.use(express.json()); app.use((req, res, next) => { rpcMetadata = getRPCMetadata(context.active()); - res.on('finish', () => { - finishListenerCount = res.listenerCount('finish'); - }); next(); }); From 171763d3f3b867f91676bdaba6ed94cc0bc61b9d Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Thu, 3 Aug 2023 09:56:59 +0700 Subject: [PATCH 6/9] remove comment related to update span name --- .../src/instrumentation.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index cf0af4f0e3..f51f50a61e 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -252,8 +252,6 @@ export class ExpressInstrumentation extends InstrumentationBase< } }; - // 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 (rpcMetadata?.type === RPCType.HTTP) { rpcMetadata.route = route || '/'; From f54ba4c0a0e4c6e7fc83f07fdf290ae6e4434e58 Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Fri, 4 Aug 2023 11:52:20 +0700 Subject: [PATCH 7/9] Move rpcRoute.metadata calculation logic up --- .../src/instrumentation.ts | 10 +++++----- .../test/ignore-all.test.ts | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index f51f50a61e..c476649750 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -195,6 +195,11 @@ export class ExpressInstrumentation extends InstrumentationBase< AttributeNames.EXPRESS_TYPE ] as ExpressLayerType; + const rpcMetadata = getRPCMetadata(context.active()); + if (rpcMetadata?.type === RPCType.HTTP) { + rpcMetadata.route = route || '/'; + } + // verify against the config if the layer should be ignored if (isLayerIgnored(metadata.name, type, instrumentation._config)) { if (type === ExpressLayerType.MIDDLEWARE) { @@ -252,11 +257,6 @@ export class ExpressInstrumentation extends InstrumentationBase< } }; - const rpcMetadata = getRPCMetadata(context.active()); - if (rpcMetadata?.type === RPCType.HTTP) { - rpcMetadata.route = route || '/'; - } - // verify we have a callback const args = Array.from(arguments); const callbackIdx = args.findIndex(arg => typeof arg === 'function'); diff --git a/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts index 277b7ea68d..0fdebeabfb 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts @@ -127,14 +127,14 @@ describe('ExpressInstrumentation', () => { ); }); - it('rpcMetadata.route should be undefined', async () => { + it('rpcMetadata.route still capture correct route', async () => { assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); await context.with( trace.setSpan(context.active(), rootSpan), async () => { await httpRequest.get(`http://localhost:${port}/toto/tata`); rootSpan.end(); - assert.strictEqual(rpcMetadata.route, undefined); + assert.strictEqual(rpcMetadata.route, '/toto/:id'); } ); }); From aafce107c9c54075a1cd77f8073318f2ff57962e Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Sun, 13 Aug 2023 09:24:00 +0700 Subject: [PATCH 8/9] Add more test --- .../test/express.test.ts | 66 ++++++++++++++++++- 1 file changed, 64 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 1a820a2a07..a0b6c09fad 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -276,7 +276,7 @@ describe('ExpressInstrumentation', () => { assert.strictEqual(res, 'test'); }); - it('should update rpcMetadata.route with the latest middleware layer', async () => { + 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 => { @@ -296,7 +296,7 @@ describe('ExpressInstrumentation', () => { trace.setSpan(context.active(), rootSpan), async () => { const response = await httpRequest.get( - `http://localhost:${port}/bare_middleware` + `http://localhost:${port}/bare_middleware/ignore_route_segment` ); assert.strictEqual(response, 'test'); rootSpan.end(); @@ -304,6 +304,68 @@ describe('ExpressInstrumentation', () => { } ); }); + + 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', () => { From 02eb3539227a51bb2f144728e92647f005273a1f Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Sun, 13 Aug 2023 11:13:59 +0700 Subject: [PATCH 9/9] Fix lint --- .../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 a0b6c09fad..64d6631409 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -315,7 +315,7 @@ describe('ExpressInstrumentation', () => { next(); }); - const router = express.Router() + const router = express.Router(); app.use('/router', router);