feat(instrumentation-express): Support non-string routes (#2008)

* feat(instrumentation-express): Support multiple routes with RegExp. * Cover more uses. * Update plugins/node/opentelemetry-instrumentation-express/src/utils.ts Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com> * Fix linter errors. * Add getLayerPath unit tests. * Address review suggestions. * Fix linter errors --------- Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com> Co-authored-by: Jamie Danielson <jamiedanielson@honeycomb.io> Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
open-telemetry · May 2, 2024 · 525bbba · 525bbba
1 parent 6e6ef62
commit 525bbba
Show file tree

Hide file tree

Showing 6 changed files with 312 additions and 10 deletions.
diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts
@@ -29,6 +29,7 @@ import { AttributeNames } from './enums/AttributeNames';
 import {
   asErrorAndMessage,
   getLayerMetadata,
+  getLayerPath,
   isLayerIgnored,
   storeLayerPath,
 } from './utils';
@@ -115,10 +116,7 @@ export class ExpressInstrumentation extends InstrumentationBase {
       ) {
         const route = original.apply(this, args);
         const layer = this.stack[this.stack.length - 1] as ExpressLayer;
-        instrumentation._applyPatch(
-          layer,
-          typeof args[0] === 'string' ? args[0] : undefined
-        );
+        instrumentation._applyPatch(layer, getLayerPath(args));
         return route;
       };
     };
@@ -136,10 +134,7 @@ export class ExpressInstrumentation extends InstrumentationBase {
       ) {
         const route = original.apply(this, args);
         const layer = this.stack[this.stack.length - 1] as ExpressLayer;
-        instrumentation._applyPatch(
-          layer,
-          typeof args[0] === 'string' ? args[0] : undefined
-        );
+        instrumentation._applyPatch(layer, getLayerPath(args));
         return route;
       };
     };
@@ -160,7 +155,7 @@ export class ExpressInstrumentation extends InstrumentationBase {
         instrumentation._applyPatch.call(
           instrumentation,
           layer,
-          typeof args[0] === 'string' ? args[0] : undefined
+          getLayerPath(args)
         );
         return route;
       };

diff --git a/plugins/node/opentelemetry-instrumentation-express/src/types.ts b/plugins/node/opentelemetry-instrumentation-express/src/types.ts
@@ -18,6 +18,8 @@ import { Span } from '@opentelemetry/api';
 import { InstrumentationConfig } from '@opentelemetry/instrumentation';
 import { ExpressLayerType } from './enums/ExpressLayerType';
 
+export type LayerPathSegment = string | RegExp | number;
+
 export type IgnoreMatcher = string | RegExp | ((name: string) => boolean);
 
 export type ExpressRequestInfo<T = any> = {

diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts
@@ -15,7 +15,11 @@
  */
 
 import { Attributes } from '@opentelemetry/api';
-import { IgnoreMatcher, ExpressInstrumentationConfig } from './types';
+import {
+  IgnoreMatcher,
+  ExpressInstrumentationConfig,
+  LayerPathSegment,
+} from './types';
 import { ExpressLayerType } from './enums/ExpressLayerType';
 import { AttributeNames } from './enums/AttributeNames';
 import {
@@ -145,3 +149,31 @@ export const asErrorAndMessage = (
   error instanceof Error
     ? [error, error.message]
     : [String(error), String(error)];
+
+/**
+ * Extracts the layer path from the route arguments
+ *
+ * @param args - Arguments of the route
+ * @returns The layer path
+ */
+export const getLayerPath = (
+  args: [LayerPathSegment | LayerPathSegment[], ...unknown[]]
+): string | undefined => {
+  if (Array.isArray(args[0])) {
+    return args[0].map(arg => extractLayerPathSegment(arg) || '').join(',');
+  }
+
+  return extractLayerPathSegment(args[0]);
+};
+
+const extractLayerPathSegment = (arg: LayerPathSegment) => {
+  if (typeof arg === 'string') {
+    return arg;
+  }
+
+  if (arg instanceof RegExp || typeof arg === 'number') {
+    return arg.toString();
+  }
+
+  return;
+};
diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts
@@ -566,4 +566,168 @@ describe('ExpressInstrumentation', () => {
       },
     });
   });
+
+  it('should set a correct transaction name for routes specified in RegEx', async () => {
+    await testUtils.runTestFixture({
+      cwd: __dirname,
+      argv: ['fixtures/use-express-regex.mjs'],
+      env: {
+        NODE_OPTIONS:
+          '--experimental-loader=@opentelemetry/instrumentation/hook.mjs',
+        NODE_NO_WARNINGS: '1',
+        TEST_REGEX_ROUTE: '/test/regex',
+      },
+      checkResult: (err, stdout, stderr) => {
+        assert.ifError(err);
+      },
+      checkCollector: (collector: testUtils.TestCollector) => {
+        const spans = collector.sortedSpans;
+
+        assert.strictEqual(spans[0].name, 'GET /\\/test\\/regex/');
+        assert.strictEqual(spans[0].kind, SpanKind.CLIENT);
+        assert.strictEqual(spans[1].name, 'middleware - query');
+        assert.strictEqual(spans[1].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[1].parentSpanId, spans[0].spanId);
+        assert.strictEqual(spans[2].name, 'middleware - expressInit');
+        assert.strictEqual(spans[2].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[2].parentSpanId, spans[0].spanId);
+        assert.strictEqual(spans[3].name, 'middleware - simpleMiddleware');
+        assert.strictEqual(spans[3].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[3].parentSpanId, spans[0].spanId);
+        assert.strictEqual(
+          spans[4].name,
+          'request handler - /\\/test\\/regex/'
+        );
+        assert.strictEqual(spans[4].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[4].parentSpanId, spans[0].spanId);
+      },
+    });
+  });
+
+  it('should set a correct transaction name for routes consisting of array including numbers', async () => {
+    await testUtils.runTestFixture({
+      cwd: __dirname,
+      argv: ['fixtures/use-express-regex.mjs'],
+      env: {
+        NODE_OPTIONS:
+          '--experimental-loader=@opentelemetry/instrumentation/hook.mjs',
+        NODE_NO_WARNINGS: '1',
+        TEST_REGEX_ROUTE: '/test/6/test',
+      },
+      checkResult: err => {
+        assert.ifError(err);
+      },
+      checkCollector: (collector: testUtils.TestCollector) => {
+        const spans = collector.sortedSpans;
+
+        assert.strictEqual(spans[0].name, 'GET /test,6,/test/');
+        assert.strictEqual(spans[0].kind, SpanKind.CLIENT);
+        assert.strictEqual(spans[1].name, 'middleware - query');
+        assert.strictEqual(spans[1].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[1].parentSpanId, spans[0].spanId);
+        assert.strictEqual(spans[2].name, 'middleware - expressInit');
+        assert.strictEqual(spans[2].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[2].parentSpanId, spans[0].spanId);
+        assert.strictEqual(spans[3].name, 'middleware - simpleMiddleware');
+        assert.strictEqual(spans[3].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[3].parentSpanId, spans[0].spanId);
+        assert.strictEqual(spans[4].name, 'request handler - /test,6,/test/');
+        assert.strictEqual(spans[4].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[4].parentSpanId, spans[0].spanId);
+      },
+    });
+  });
+
+  for (const segment of ['array1', 'array5']) {
+    it('should set a correct transaction name for routes consisting of arrays of routes', async () => {
+      await testUtils.runTestFixture({
+        cwd: __dirname,
+        argv: ['fixtures/use-express-regex.mjs'],
+        env: {
+          NODE_OPTIONS:
+            '--experimental-loader=@opentelemetry/instrumentation/hook.mjs',
+          NODE_NO_WARNINGS: '1',
+          TEST_REGEX_ROUTE: `/test/${segment}`,
+        },
+        checkResult: err => {
+          assert.ifError(err);
+        },
+        checkCollector: (collector: testUtils.TestCollector) => {
+          const spans = collector.sortedSpans;
+
+          assert.strictEqual(
+            spans[0].name,
+            'GET /test/array1,/\\/test\\/array[2-9]/'
+          );
+          assert.strictEqual(spans[0].kind, SpanKind.CLIENT);
+          assert.strictEqual(spans[1].name, 'middleware - query');
+          assert.strictEqual(spans[1].kind, SpanKind.SERVER);
+          assert.strictEqual(spans[1].parentSpanId, spans[0].spanId);
+          assert.strictEqual(spans[2].name, 'middleware - expressInit');
+          assert.strictEqual(spans[2].kind, SpanKind.SERVER);
+          assert.strictEqual(spans[2].parentSpanId, spans[0].spanId);
+          assert.strictEqual(spans[3].name, 'middleware - simpleMiddleware');
+          assert.strictEqual(spans[3].kind, SpanKind.SERVER);
+          assert.strictEqual(spans[3].parentSpanId, spans[0].spanId);
+          assert.strictEqual(
+            spans[4].name,
+            'request handler - /test/array1,/\\/test\\/array[2-9]/'
+          );
+          assert.strictEqual(spans[4].kind, SpanKind.SERVER);
+          assert.strictEqual(spans[4].parentSpanId, spans[0].spanId);
+        },
+      });
+    });
+  }
+
+  for (const segment of [
+    'arr/545',
+    'arr/required',
+    'arr/required',
+    'arr/requiredPath',
+    'arr/required/lastParam',
+    'arr55/required/lastParam',
+    'arr/requiredPath/optionalPath/',
+    'arr/requiredPath/optionalPath/lastParam',
+  ]) {
+    it('should handle more complex regexes in route arrays correctly', async () => {
+      await testUtils.runTestFixture({
+        cwd: __dirname,
+        argv: ['fixtures/use-express-regex.mjs'],
+        env: {
+          NODE_OPTIONS:
+            '--experimental-loader=@opentelemetry/instrumentation/hook.mjs',
+          NODE_NO_WARNINGS: '1',
+          TEST_REGEX_ROUTE: `/test/${segment}`,
+        },
+        checkResult: err => {
+          assert.ifError(err);
+        },
+        checkCollector: (collector: testUtils.TestCollector) => {
+          const spans = collector.sortedSpans;
+
+          assert.strictEqual(
+            spans[0].name,
+            'GET /test/arr/:id,/\\/test\\/arr[0-9]*\\/required(path)?(\\/optionalPath)?\\/(lastParam)?/'
+          );
+          assert.strictEqual(spans[0].kind, SpanKind.CLIENT);
+          assert.strictEqual(spans[1].name, 'middleware - query');
+          assert.strictEqual(spans[1].kind, SpanKind.SERVER);
+          assert.strictEqual(spans[1].parentSpanId, spans[0].spanId);
+          assert.strictEqual(spans[2].name, 'middleware - expressInit');
+          assert.strictEqual(spans[2].kind, SpanKind.SERVER);
+          assert.strictEqual(spans[2].parentSpanId, spans[0].spanId);
+          assert.strictEqual(spans[3].name, 'middleware - simpleMiddleware');
+          assert.strictEqual(spans[3].kind, SpanKind.SERVER);
+          assert.strictEqual(spans[3].parentSpanId, spans[0].spanId);
+          assert.strictEqual(
+            spans[4].name,
+            'request handler - /test/arr/:id,/\\/test\\/arr[0-9]*\\/required(path)?(\\/optionalPath)?\\/(lastParam)?/'
+          );
+          assert.strictEqual(spans[4].kind, SpanKind.SERVER);
+          assert.strictEqual(spans[4].parentSpanId, spans[0].spanId);
+        },
+      });
+    });
+  }
 });
diff --git a/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-regex.mjs b/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-regex.mjs
@@ -0,0 +1,85 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// Use express from an ES module:
+//    node --experimental-loader=@opentelemetry/instrumentation/hook.mjs use-express-regex.mjs
+
+import { promisify } from 'util';
+import { createTestNodeSdk } from '@opentelemetry/contrib-test-utils';
+
+import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
+import { ExpressInstrumentation } from '../../build/src/index.js';
+
+const sdk = createTestNodeSdk({
+  serviceName: 'use-express-regex',
+  instrumentations: [new HttpInstrumentation(), new ExpressInstrumentation()],
+});
+sdk.start();
+
+import express from 'express';
+import * as http from 'http';
+
+const app = express();
+
+app.use(async function simpleMiddleware(req, res, next) {
+  // Wait a short delay to ensure this "middleware - ..." span clearly starts
+  // before the "router - ..." span. The test rely on a start-time-based sort
+  // of the produced spans. If they start in the same millisecond, then tests
+  // can be flaky.
+  await promisify(setTimeout)(10);
+  next();
+});
+
+app.get(
+  [
+    '/test/arr/:id',
+    /\/test\/arr[0-9]*\/required(path)?(\/optionalPath)?\/(lastParam)?/,
+  ],
+  (_req, res) => {
+    res.send({ response: 'response' });
+  }
+);
+
+app.get(/\/test\/regex/, (_req, res) => {
+  res.send({ response: 'response 2' });
+});
+
+app.get(['/test/array1', /\/test\/array[2-9]/], (_req, res) => {
+  res.send({ response: 'response 3' });
+});
+
+app.get(['/test', 6, /test/], (_req, res) => {
+  res.send({ response: 'response 4' });
+});
+
+const server = http.createServer(app);
+await new Promise(resolve => server.listen(0, resolve));
+const port = server.address().port;
+
+await new Promise(resolve => {
+  http.get(
+    `http://localhost:${port}${process.env.TEST_REGEX_ROUTE}`,
+    res => {
+      res.resume();
+      res.on('end', () => {
+        resolve();
+      });
+    }
+  );
+});
+
+await new Promise(resolve => server.close(resolve));
+await sdk.shutdown();
diff --git a/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts
@@ -165,4 +165,28 @@ describe('Utils', () => {
       assert.strictEqual(message, '2');
     });
   });
+
+  describe('getLayerPath', () => {
+    it('should return path for a string route definition', () => {
+      assert.strictEqual(utils.getLayerPath(['/test']), '/test');
+    });
+
+    it('should return path for a regex route definition', () => {
+      assert.strictEqual(utils.getLayerPath([/^\/test$/]), '/^\\/test$/');
+    });
+
+    it('should return path for an array of route definitions', () => {
+      assert.strictEqual(
+        utils.getLayerPath([[/^\/test$/, '/test']]),
+        '/^\\/test$/,/test'
+      );
+    });
+
+    it('should return path for a mixed array of route definitions', () => {
+      assert.strictEqual(
+        utils.getLayerPath([[/^\/test$/, '/test', /^\/test$/]]),
+        '/^\\/test$/,/test,/^\\/test$/'
+      );
+    });
+  });
 });