From eeda32a03a4d75166013188bd0a295a17b2da1dc Mon Sep 17 00:00:00 2001 From: Chi Ma Date: Sun, 13 Aug 2023 13:42:24 +0700 Subject: [PATCH] fix(express): make rpcMetadata.route capture the last layer even when if the last layer is not REQUEST_HANDLER (#1620) * fix(express): make rpcMetadata.route capture the last layer even when if the last layer is mot REQUEST_HANDLER * fix lint issue * remove test.only * revert code to change ignore order * update test * remove comment related to update span name * Move rpcRoute.metadata calculation logic up * Add more test * Fix lint --- .../src/instrumentation.ts | 10 +- .../test/express.test.ts | 91 +++++++++++++++++++ .../test/ignore-all.test.ts | 2 +- 3 files changed, 96 insertions(+), 7 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 0c27a8a88f..c476649750 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -186,6 +186,7 @@ export class ExpressInstrumentation extends InstrumentationBase< const route = (req[_LAYERS_STORE_PROPERTY] as string[]) .filter(path => path !== '/' && path !== '/*') .join(''); + const attributes: SpanAttributes = { [SemanticAttributes.HTTP_ROUTE]: route.length > 0 ? route : '/', }; @@ -194,13 +195,8 @@ export class ExpressInstrumentation extends InstrumentationBase< 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 || '/'; } @@ -211,6 +207,7 @@ export class ExpressInstrumentation extends InstrumentationBase< } return original.apply(this, arguments); } + if (trace.getSpan(context.active()) === undefined) { return original.apply(this, arguments); } @@ -259,6 +256,7 @@ export class ExpressInstrumentation extends InstrumentationBase< span.end(); } }; + // 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/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 79b5f29488..64d6631409 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -275,6 +275,97 @@ describe('ExpressInstrumentation', () => { assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); assert.strictEqual(res, 'test'); }); + + 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 => { + app.use(express.json()); + app.use((req, res, next) => { + rpcMetadata = getRPCMetadata(context.active()); + 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/ignore_route_segment` + ); + assert.strictEqual(response, 'test'); + rootSpan.end(); + assert.strictEqual(rpcMetadata?.route, '/bare_middleware'); + } + ); + }); + + 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', () => { 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..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,7 +127,7 @@ describe('ExpressInstrumentation', () => { ); }); - it('rpcMetadata.route should be modified to /todo/:id', async () => { + it('rpcMetadata.route still capture correct route', async () => { assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); await context.with( trace.setSpan(context.active(), rootSpan),