feat: enable root span route instrumentation without any express laye…

…r spans
open-telemetry · Dec 6, 2020 · 8fe3977 · 8fe3977
1 parent 6449f22
commit 8fe3977
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 11 deletions.
diff --git 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
@@ -170,7 +170,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 +180,33 @@ export class ExpressPlugin extends BasePlugin<typeof express> {
         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
@@ -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', () => {