Merge pull request #273 from shyimo/express-plugin-enhancements

open-telemetry · Jan 7, 2021 · 31d7bee · 31d7bee
2 parents 130985b + 5c2dc21
commit 31d7bee
Show file tree

Hide file tree

Showing 4 changed files with 108 additions and 16 deletions.
diff --git 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
@@ -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<typeof express> {
       ) {
         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 +181,30 @@ export class ExpressPlugin extends BasePlugin<typeof express> {
         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);
         }
         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
@@ -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
@@ -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();