Address review suggestions.

open-telemetry · Apr 1, 2024 · 83558f0 · 83558f0
1 parent 0eb793c
commit 83558f0
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 11 deletions.
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,7 @@
  */
 
 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 {
@@ -152,20 +152,27 @@ export const asErrorAndMessage = (
  * @param args - Arguments of the route
  * @returns The layer path
  */
-export const getLayerPath = (args: unknown[]): string | undefined => {
-  if (args[0] instanceof RegExp) {
-    return args[0].toString();
-  }
-
-  if (typeof args[0] === 'string') {
-    return args[0];
-  }
+export const getLayerPath = (args: [
+  LayerPathSegment | (LayerPathSegment)[], ...unknown[]
+]): string | undefined => {
 
   if (Array.isArray(args[0])) {
     return args[0]
-      .map(arg => (typeof arg === 'string' || arg instanceof RegExp ? arg : ''))
+      .map(arg => extractLayerPathSegment(arg) || '')
       .join(',');
   }
 
-  return;
+  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
@@ -604,6 +604,46 @@ describe('ExpressInstrumentation', () => {
     });
   });
 
+  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({

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
@@ -61,6 +61,10 @@ 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;