From 8fe3977d4b4245a0c59a9579588a78d663c684fe Mon Sep 17 00:00:00 2001 From: shyimo Date: Sun, 6 Dec 2020 09:04:05 +0200 Subject: [PATCH 1/5] feat: enable root span route instrumentation without any express layer spans --- .../opentelemetry-plugin-express/package.json | 1 + .../src/express.ts | 32 ++++++---- .../test/express.test.ts | 61 +++++++++++++++++++ 3 files changed, 83 insertions(+), 11 deletions(-) diff --git a/plugins/node/opentelemetry-plugin-express/package.json b/plugins/node/opentelemetry-plugin-express/package.json index f565359ccc..1da61611f8 100644 --- a/plugins/node/opentelemetry-plugin-express/package.json +++ b/plugins/node/opentelemetry-plugin-express/package.json @@ -49,6 +49,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..2f34d7874c 100644 --- a/plugins/node/opentelemetry-plugin-express/src/express.ts +++ b/plugins/node/opentelemetry-plugin-express/src/express.ts @@ -170,7 +170,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,23 +180,33 @@ export class ExpressPlugin extends BasePlugin { const type = metadata.attributes[ AttributeNames.EXPRESS_TYPE ] as ExpressLayerType; - // verify against the config if the layer should be ignored - if (isLayerIgnored(metadata.name, type, plugin._config)) { - return original.apply(this, arguments); - } - if (plugin._tracer.getCurrentSpan() === undefined) { - return original.apply(this, arguments); - } - // Rename the root http span once we reach the request handler + + // 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(); - if (parent) { - parent.updateName(`${req.method} ${route}`); + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore + const parentSpanName = parent && parent.name; + if (parent && parentSpanName) { + const parentRoute = parentSpanName.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); + } + if (plugin._tracer.getCurrentSpan() === undefined) { + return original.apply(this, arguments); + } + const span = plugin._tracer.startSpan(metadata.name, { attributes: Object.assign(attributes, metadata.attributes), }); diff --git a/plugins/node/opentelemetry-plugin-express/test/express.test.ts b/plugins/node/opentelemetry-plugin-express/test/express.test.ts index 81f4d3259c..8515e35df2 100644 --- a/plugins/node/opentelemetry-plugin-express/test/express.test.ts +++ b/plugins/node/opentelemetry-plugin-express/test/express.test.ts @@ -102,6 +102,8 @@ describe('Express Plugin', () => { await tracer.withSpan(rootSpan, async () => { await httpRequest.get(`http://localhost:${port}/toto/tata`); rootSpan.end(); + // @ts-ignore + assert.strictEqual(rootSpan.name, 'GET /toto/:id'); assert.notStrictEqual( memoryExporter .getFinishedSpans() @@ -204,6 +206,65 @@ describe('Express Plugin', () => { }); server.close(); }); + it('should ignore all ExpressLayerType based on config. root span name should be modified when route exists', async () => { + plugin.disable(); + const config: ExpressPluginConfig = { + ignoreLayersType: [ + ExpressLayerType.MIDDLEWARE, + ExpressLayerType.ROUTER, + ExpressLayerType.REQUEST_HANDLER, + ], + }; + plugin.enable(express, provider, logger, config); + const rootSpan = tracer.startSpan('rootSpan'); + 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++) { + continue; + } + return next(); + }); + const router = express.Router(); + app.use('/toto', router); + router.get('/:id', (req, res) => { + setImmediate(() => { + res.status(200).end(); + }); + }); + const server = http.createServer(app); + await new Promise(resolve => server.listen(0, resolve)); + + 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(); + // @ts-ignore + assert.strictEqual(rootSpan.name, 'GET /toto/:id'); + + 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 + ); + const exportedRootSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name === 'GET /toto/:id'); + assert.notStrictEqual(exportedRootSpan, undefined); + }); + server.close(); + }); }); describe('Disabling plugin', () => { From 77effeba9c5fbeaf905c700098f150e93123e10e Mon Sep 17 00:00:00 2001 From: shyimo Date: Sun, 6 Dec 2020 09:19:09 +0200 Subject: [PATCH 2/5] fix lint error in graphql insturmantation --- .../node/opentelemetry-instrumentation-graphql/src/types.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts index 9c157e5d92..c3c78b3653 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts @@ -63,9 +63,7 @@ export interface GraphQLInstrumentationConfig { /** * Merged and parsed config of default instrumentation config and GraphQL */ -export type GraphQLInstrumentationParsedConfig = Required< - GraphQLInstrumentationConfig -> & +export type GraphQLInstrumentationParsedConfig = Required & InstrumentationConfig; export type executeFunctionWithObj = ( From 014088feb440b246ced3c53ea6a92d2abd086e76 Mon Sep 17 00:00:00 2001 From: shyimo Date: Tue, 15 Dec 2020 10:45:56 +0200 Subject: [PATCH 3/5] fix obecny code review notes --- .../node/opentelemetry-plugin-express/src/express.ts | 10 ++++------ plugins/node/opentelemetry-plugin-express/src/types.ts | 9 ++++++++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/plugins/node/opentelemetry-plugin-express/src/express.ts b/plugins/node/opentelemetry-plugin-express/src/express.ts index 2f34d7874c..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'; @@ -187,12 +188,9 @@ export class ExpressPlugin extends BasePlugin { metadata.attributes[AttributeNames.EXPRESS_TYPE] === ExpressLayerType.REQUEST_HANDLER ) { - const parent = plugin._tracer.getCurrentSpan(); - // eslint-disable-next-line @typescript-eslint/ban-ts-ignore - // @ts-ignore - const parentSpanName = parent && parent.name; - if (parent && parentSpanName) { - const parentRoute = parentSpanName.split(' ')[1]; + 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}`); } diff --git a/plugins/node/opentelemetry-plugin-express/src/types.ts b/plugins/node/opentelemetry-plugin-express/src/types.ts index 1374a90bb0..5d8814521a 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 { PluginConfig, Attributes } from '@opentelemetry/api'; +import { PluginConfig, Attributes, Span } from '@opentelemetry/api'; /** * This const define where on the `request` object the plugin will mount the @@ -92,3 +92,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; +} From 5bac5935406776afc2c702027c98544080794698 Mon Sep 17 00:00:00 2001 From: shyimo Date: Tue, 22 Dec 2020 15:52:40 +0200 Subject: [PATCH 4/5] removed ts-ignore from unit tests --- .../opentelemetry-plugin-express/test/express.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/plugins/node/opentelemetry-plugin-express/test/express.test.ts b/plugins/node/opentelemetry-plugin-express/test/express.test.ts index 8515e35df2..f31e8a4594 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,7 +103,6 @@ describe('Express Plugin', () => { await tracer.withSpan(rootSpan, async () => { await httpRequest.get(`http://localhost:${port}/toto/tata`); rootSpan.end(); - // @ts-ignore assert.strictEqual(rootSpan.name, 'GET /toto/:id'); assert.notStrictEqual( memoryExporter @@ -216,7 +216,7 @@ describe('Express Plugin', () => { ], }; plugin.enable(express, provider, logger, config); - 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()); @@ -241,9 +241,7 @@ describe('Express Plugin', () => { await tracer.withSpan(rootSpan, async () => { await httpRequest.get(`http://localhost:${port}/toto/tata`); rootSpan.end(); - // @ts-ignore assert.strictEqual(rootSpan.name, 'GET /toto/:id'); - assert.deepStrictEqual( memoryExporter .getFinishedSpans() From a82f395a470aceb16b95e89289145ec22a5f2679 Mon Sep 17 00:00:00 2001 From: shyimo Date: Thu, 7 Jan 2021 10:01:40 +0200 Subject: [PATCH 5/5] fix: fix express unit tests according to PR rejects --- .../test/express.test.ts | 53 ++++++++++++------- 1 file changed, 35 insertions(+), 18 deletions(-) diff --git a/plugins/node/opentelemetry-plugin-express/test/express.test.ts b/plugins/node/opentelemetry-plugin-express/test/express.test.ts index f31e8a4594..f3ba5442ba 100644 --- a/plugins/node/opentelemetry-plugin-express/test/express.test.ts +++ b/plugins/node/opentelemetry-plugin-express/test/express.test.ts @@ -144,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,24 +204,28 @@ describe('Express Plugin', () => { }); server.close(); }); - it('should ignore all ExpressLayerType based on config. root span name should be modified when route exists', async () => { + }); + + 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(); - const config: ExpressPluginConfig = { - ignoreLayersType: [ - ExpressLayerType.MIDDLEWARE, - ExpressLayerType.ROUTER, - ExpressLayerType.REQUEST_HANDLER, - ], - }; plugin.enable(express, provider, logger, config); - const rootSpan = tracer.startSpan('rootSpan') as ExpressPluginSpan; + 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++) { - continue; - } + for (let i = 0; i < 1000; i++) {} return next(); }); const router = express.Router(); @@ -233,15 +235,21 @@ describe('Express Plugin', () => { res.status(200).end(); }); }); - const server = http.createServer(app); + + 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.strictEqual(rootSpan.name, 'GET /toto/:id'); assert.deepStrictEqual( memoryExporter .getFinishedSpans() @@ -256,12 +264,21 @@ describe('Express Plugin', () => { ).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); }); - server.close(); }); });