diff --git a/plugins/node/opentelemetry-plugin-express/package.json b/plugins/node/opentelemetry-plugin-express/package.json index 8820b9f966..d6557375b0 100644 --- a/plugins/node/opentelemetry-plugin-express/package.json +++ b/plugins/node/opentelemetry-plugin-express/package.json @@ -50,6 +50,7 @@ "@types/node": "14.0.27", "@types/shimmer": "1.0.1", "codecov": "3.7.2", + "eslint-plugin-header": "^3.1.0", "express": "4.17.1", "gts": "2.0.2", "mocha": "7.2.0", diff --git a/plugins/node/opentelemetry-plugin-express/src/express.ts b/plugins/node/opentelemetry-plugin-express/src/express.ts index d5d66cefd3..f6b39e0483 100644 --- a/plugins/node/opentelemetry-plugin-express/src/express.ts +++ b/plugins/node/opentelemetry-plugin-express/src/express.ts @@ -29,6 +29,7 @@ import { _LAYERS_STORE_PROPERTY, ExpressPluginConfig, ExpressLayerType, + ExpressPluginSpan, } from './types'; import { getLayerMetadata, storeLayerPath, isLayerIgnored } from './utils'; import { VERSION } from './version'; @@ -170,7 +171,7 @@ export class ExpressPlugin extends BasePlugin { ) { storeLayerPath(req, layerPath); const route = (req[_LAYERS_STORE_PROPERTY] as string[]) - .filter(path => path !== '/') + .filter(path => path !== '/' && path !== '/*') .join(''); const attributes: Attributes = { [AttributeNames.COMPONENT]: ExpressPlugin.component, @@ -180,6 +181,22 @@ export class ExpressPlugin extends BasePlugin { 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 + if ( + metadata.attributes[AttributeNames.EXPRESS_TYPE] === + ExpressLayerType.REQUEST_HANDLER + ) { + const parent = plugin._tracer.getCurrentSpan() as ExpressPluginSpan; + if (parent?.name) { + const parentRoute = parent.name.split(' ')[1]; + if (!route.includes(parentRoute)) { + parent.updateName(`${req.method} ${route}`); + } + } + } + // verify against the config if the layer should be ignored if (isLayerIgnored(metadata.name, type, plugin._config)) { return original.apply(this, arguments); @@ -187,16 +204,7 @@ export class ExpressPlugin extends BasePlugin { if (plugin._tracer.getCurrentSpan() === undefined) { return original.apply(this, arguments); } - // Rename the root http span once we reach the request handler - if ( - metadata.attributes[AttributeNames.EXPRESS_TYPE] === - ExpressLayerType.REQUEST_HANDLER - ) { - const parent = plugin._tracer.getCurrentSpan(); - if (parent) { - parent.updateName(`${req.method} ${route}`); - } - } + const span = plugin._tracer.startSpan(metadata.name, { attributes: Object.assign(attributes, metadata.attributes), }); diff --git a/plugins/node/opentelemetry-plugin-express/src/types.ts b/plugins/node/opentelemetry-plugin-express/src/types.ts index c4bde0108a..051eed6431 100644 --- a/plugins/node/opentelemetry-plugin-express/src/types.ts +++ b/plugins/node/opentelemetry-plugin-express/src/types.ts @@ -16,7 +16,7 @@ import { kLayerPatched } from './express'; import { Request } from 'express'; -import { Attributes } from '@opentelemetry/api'; +import { Attributes, Span } from '@opentelemetry/api'; import { PluginConfig } from '@opentelemetry/core'; /** @@ -93,3 +93,10 @@ export interface ExpressPluginConfig extends PluginConfig { /** Ignore specific layers based on their type */ ignoreLayersType?: ExpressLayerType[]; } + +/** + * extends opentelemetry/api Span object to instrument the root span name of http plugin + */ +export interface ExpressPluginSpan extends Span { + name?: string; +} diff --git a/plugins/node/opentelemetry-plugin-express/test/express.test.ts b/plugins/node/opentelemetry-plugin-express/test/express.test.ts index 81f4d3259c..f3ba5442ba 100644 --- a/plugins/node/opentelemetry-plugin-express/test/express.test.ts +++ b/plugins/node/opentelemetry-plugin-express/test/express.test.ts @@ -31,6 +31,7 @@ import { AttributeNames, ExpressLayerType, ExpressPluginConfig, + ExpressPluginSpan, } from '../src/types'; const httpRequest = { @@ -77,7 +78,7 @@ describe('Express Plugin', () => { describe('Instrumenting normal get operations', () => { it('should create a child span for middlewares', async () => { - const rootSpan = tracer.startSpan('rootSpan'); + const rootSpan = tracer.startSpan('rootSpan') as ExpressPluginSpan; const app = express(); app.use((req, res, next) => tracer.withSpan(rootSpan, next)); app.use(express.json()); @@ -102,6 +103,7 @@ describe('Express Plugin', () => { await tracer.withSpan(rootSpan, async () => { await httpRequest.get(`http://localhost:${port}/toto/tata`); rootSpan.end(); + assert.strictEqual(rootSpan.name, 'GET /toto/:id'); assert.notStrictEqual( memoryExporter .getFinishedSpans() @@ -142,9 +144,7 @@ describe('Express Plugin', () => { const app = express(); app.use(express.json()); app.use((req, res, next) => { - for (let i = 0; i < 1000000; i++) { - continue; - } + for (let i = 0; i < 1000000; i++) {} return next(); }); const router = express.Router(); @@ -206,6 +206,82 @@ describe('Express Plugin', () => { }); }); + describe('when route exists', () => { + let server: http.Server; + let rootSpan: ExpressPluginSpan; + const config: ExpressPluginConfig = { + ignoreLayersType: [ + ExpressLayerType.MIDDLEWARE, + ExpressLayerType.ROUTER, + ExpressLayerType.REQUEST_HANDLER, + ], + }; + + beforeEach(async () => { + plugin.disable(); + plugin.enable(express, provider, logger, config); + rootSpan = tracer.startSpan('rootSpan') as ExpressPluginSpan; + const app = express(); + app.use((req, res, next) => tracer.withSpan(rootSpan, next)); + app.use(express.json()); + app.use((req, res, next) => { + for (let i = 0; i < 1000; i++) {} + return next(); + }); + const router = express.Router(); + app.use('/toto', router); + router.get('/:id', (req, res) => { + setImmediate(() => { + res.status(200).end(); + }); + }); + + server = http.createServer(app); + await new Promise(resolve => server.listen(0, resolve)); + }); + + afterEach(() => { + server.close(); + }); + + it('should ignore all ExpressLayerType based on config', async () => { + const port = (server.address() as AddressInfo).port; + assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); + await tracer.withSpan(rootSpan, async () => { + await httpRequest.get(`http://localhost:${port}/toto/tata`); + rootSpan.end(); + assert.deepStrictEqual( + memoryExporter + .getFinishedSpans() + .filter( + span => + span.attributes[AttributeNames.EXPRESS_TYPE] === + ExpressLayerType.MIDDLEWARE || + span.attributes[AttributeNames.EXPRESS_TYPE] === + ExpressLayerType.ROUTER || + span.attributes[AttributeNames.EXPRESS_TYPE] === + ExpressLayerType.REQUEST_HANDLER + ).length, + 0 + ); + }); + }); + + it('root span name should be modified to GET /todo/:id', async () => { + const port = (server.address() as AddressInfo).port; + assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); + await tracer.withSpan(rootSpan, async () => { + await httpRequest.get(`http://localhost:${port}/toto/tata`); + rootSpan.end(); + assert.strictEqual(rootSpan.name, 'GET /toto/:id'); + const exportedRootSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name === 'GET /toto/:id'); + assert.notStrictEqual(exportedRootSpan, undefined); + }); + }); + }); + describe('Disabling plugin', () => { it('should not create new spans', async () => { plugin.disable();