From b1883182145dcea10dec07aacf068e4af1d81b12 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 13 Mar 2024 12:55:27 +0000 Subject: [PATCH 1/7] feat(instrumentation-express): Support multiple routes with RegExp. --- .../src/instrumentation.ts | 7 +- .../src/utils.ts | 21 ++++++ .../test/express.test.ts | 35 ++++++++++ .../test/fixtures/use-express-regex.mjs | 67 +++++++++++++++++++ 4 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-regex.mjs diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 3df15aa699..013650327c 100644 --- 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'; @@ -125,7 +126,7 @@ export class ExpressInstrumentation extends InstrumentationBase< const layer = this.stack[this.stack.length - 1] as ExpressLayer; instrumentation._applyPatch( layer, - typeof args[0] === 'string' ? args[0] : undefined + getLayerPath(args) ); return route; }; @@ -146,7 +147,7 @@ export class ExpressInstrumentation extends InstrumentationBase< const layer = this.stack[this.stack.length - 1] as ExpressLayer; instrumentation._applyPatch( layer, - typeof args[0] === 'string' ? args[0] : undefined + getLayerPath(args) ); return route; }; @@ -168,7 +169,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/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index 7e51b84124..fb7446ba76 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -145,3 +145,24 @@ 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: unknown[]) => { + if (typeof args[0] === 'string') { + return args[0]; + } + + if (Array.isArray(args[0])) { + return args[0].map((arg) => ( + typeof arg === 'string' || arg instanceof RegExp ? arg : '') + ).join(','); + } + + return; +} diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index f573a4a669..241f35bec4 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -566,4 +566,39 @@ describe('ExpressInstrumentation', () => { }, }); }); + + it('should work with ESM usage and multiple routes including 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', + }, + checkResult: (err, stdout, stderr) => { + assert.ifError(err); + }, + checkCollector: (collector: testUtils.TestCollector) => { + const spans = collector.sortedSpans; + + console.debug(spans); + + 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 new file mode 100644 index 0000000000..e5aebae064 --- /dev/null +++ b/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-regex.mjs @@ -0,0 +1,67 @@ +/* + * 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' }); +}); + +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}/test/arr/requiredPath/optionalPath/lastParam`, (res) => { + res.resume(); + res.on('end', () => { + resolve(); + }); + }) +}); + +await new Promise(resolve => server.close(resolve)); +await sdk.shutdown(); From d9b35d2bae35f27d57f5052da0923385baa01a08 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 15 Mar 2024 16:04:55 +0000 Subject: [PATCH 2/7] Cover more uses. --- .../src/utils.ts | 4 + .../test/express.test.ts | 105 +++++++++++++++++- .../test/fixtures/use-express-regex.mjs | 40 ++++--- 3 files changed, 131 insertions(+), 18 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index fb7446ba76..d888115a0f 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -154,6 +154,10 @@ export const asErrorAndMessage = ( * @returns The layer path */ export const getLayerPath = (args: unknown[]) => { + if (args[0] instanceof RegExp) { + return args[0].toString(); + } + if (typeof args[0] === 'string') { return args[0]; } diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 241f35bec4..148bac3d61 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -567,7 +567,7 @@ describe('ExpressInstrumentation', () => { }); }); - it('should work with ESM usage and multiple routes including regex', async () => { + it('should set a correct transaction name for routes specified in RegEx', async () => { await testUtils.runTestFixture({ cwd: __dirname, argv: ['fixtures/use-express-regex.mjs'], @@ -575,6 +575,7 @@ describe('ExpressInstrumentation', () => { NODE_OPTIONS: '--experimental-loader=@opentelemetry/instrumentation/hook.mjs', NODE_NO_WARNINGS: '1', + TEST_REGEX_ROUTE: '/test/regex', }, checkResult: (err, stdout, stderr) => { assert.ifError(err); @@ -582,9 +583,7 @@ describe('ExpressInstrumentation', () => { checkCollector: (collector: testUtils.TestCollector) => { const spans = collector.sortedSpans; - console.debug(spans); - - assert.strictEqual(spans[0].name, 'GET /test/arr/:id,/\\\/test\\\/arr[0-9]*\\\/required(path)?(\\\/optionalPath)?\\\/(lastParam)?/'); + 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); @@ -595,10 +594,106 @@ describe('ExpressInstrumentation', () => { 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].name, + 'request handler - /\\/test\\/regex/' + ); 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 index e5aebae064..facba6b16c 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-regex.mjs +++ b/plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-regex.mjs @@ -25,11 +25,8 @@ import { ExpressInstrumentation } from '../../build/src/index.js'; const sdk = createTestNodeSdk({ serviceName: 'use-express-regex', - instrumentations: [ - new HttpInstrumentation(), - new ExpressInstrumentation() - ] -}) + instrumentations: [new HttpInstrumentation(), new ExpressInstrumentation()], +}); sdk.start(); import express from 'express'; @@ -46,8 +43,22 @@ app.use(async function simpleMiddleware(req, res, next) { next(); }); -app.get(['/test/arr/:id', /\/test\/arr[0-9]*\/required(path)?(\/optionalPath)?\/(lastParam)?/], (_req, res) => { - res.send({ response: 'response' }); +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' }); }); const server = http.createServer(app); @@ -55,12 +66,15 @@ await new Promise(resolve => server.listen(0, resolve)); const port = server.address().port; await new Promise(resolve => { - http.get(`http://localhost:${port}/test/arr/requiredPath/optionalPath/lastParam`, (res) => { - res.resume(); - res.on('end', () => { - 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)); From 3aac5e7191e41022aae7087e3b1f7d95a395a872 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 20 Mar 2024 13:57:43 +0000 Subject: [PATCH 3/7] Update plugins/node/opentelemetry-instrumentation-express/src/utils.ts Co-authored-by: Jamie Danielson --- plugins/node/opentelemetry-instrumentation-express/src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index d888115a0f..b4d35243cf 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -153,7 +153,7 @@ export const asErrorAndMessage = ( * @param args - Arguments of the route * @returns The layer path */ -export const getLayerPath = (args: unknown[]) => { +export const getLayerPath = (args: unknown[]): string | undefined => { if (args[0] instanceof RegExp) { return args[0].toString(); } From 54717b6ce7d5414a49a5ec80da775ac970aca484 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 20 Mar 2024 13:59:23 +0000 Subject: [PATCH 4/7] Fix linter errors. --- .../src/instrumentation.ts | 10 ++-------- .../opentelemetry-instrumentation-express/src/utils.ts | 9 ++++----- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index 013650327c..bd1d0fd325 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -124,10 +124,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, - getLayerPath(args) - ); + instrumentation._applyPatch(layer, getLayerPath(args)); return route; }; }; @@ -145,10 +142,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, - getLayerPath(args) - ); + instrumentation._applyPatch(layer, getLayerPath(args)); return route; }; }; diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index b4d35243cf..bdf48a0389 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts @@ -146,7 +146,6 @@ export const asErrorAndMessage = ( ? [error, error.message] : [String(error), String(error)]; - /** * Extracts the layer path from the route arguments * @@ -163,10 +162,10 @@ export const getLayerPath = (args: unknown[]): string | undefined => { } if (Array.isArray(args[0])) { - return args[0].map((arg) => ( - typeof arg === 'string' || arg instanceof RegExp ? arg : '') - ).join(','); + return args[0] + .map(arg => (typeof arg === 'string' || arg instanceof RegExp ? arg : '')) + .join(','); } return; -} +}; From 0eb793c9c1bd4a76c2dc3d90991e4502be36bcdd Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 20 Mar 2024 15:45:19 +0000 Subject: [PATCH 5/7] Add getLayerPath unit tests. --- .../test/utils.test.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/utils.test.ts index fc927a9b7f..098bab4e8c 100644 --- 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$/' + ); + }); + }); }); From 83558f02258c862ea830cf7afb8ce762803f986f Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 1 Apr 2024 16:11:23 +0100 Subject: [PATCH 6/7] Address review suggestions. --- .../src/types.ts | 2 + .../src/utils.ts | 29 +++++++++----- .../test/express.test.ts | 40 +++++++++++++++++++ .../test/fixtures/use-express-regex.mjs | 4 ++ 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/types.ts b/plugins/node/opentelemetry-instrumentation-express/src/types.ts index 50d76df514..4ccb36e6af 100644 --- 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 = { diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index bdf48a0389..7061de9bf6 100644 --- 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 index 148bac3d61..9175b4b993 100644 --- 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 index facba6b16c..8fc412841b 100644 --- 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; From e4abf2715f61a228956c3831d15727f453f2818c Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 16 Apr 2024 14:18:20 +0100 Subject: [PATCH 7/7] Fix linter errors --- .../src/utils.ts | 19 ++++++++++--------- .../test/express.test.ts | 10 ++-------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-express/src/utils.ts b/plugins/node/opentelemetry-instrumentation-express/src/utils.ts index 7061de9bf6..fa7b0ee9a0 100644 --- 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, LayerPathSegment } from './types'; +import { + IgnoreMatcher, + ExpressInstrumentationConfig, + LayerPathSegment, +} from './types'; import { ExpressLayerType } from './enums/ExpressLayerType'; import { AttributeNames } from './enums/AttributeNames'; import { @@ -152,14 +156,11 @@ export const asErrorAndMessage = ( * @param args - Arguments of the route * @returns The layer path */ -export const getLayerPath = (args: [ - LayerPathSegment | (LayerPathSegment)[], ...unknown[] -]): string | undefined => { - +export const getLayerPath = ( + args: [LayerPathSegment | LayerPathSegment[], ...unknown[]] +): string | undefined => { if (Array.isArray(args[0])) { - return args[0] - .map(arg => extractLayerPathSegment(arg) || '') - .join(','); + return args[0].map(arg => extractLayerPathSegment(arg) || '').join(','); } return extractLayerPathSegment(args[0]); @@ -175,4 +176,4 @@ const extractLayerPathSegment = (arg: LayerPathSegment) => { } return; -} +}; diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index 9175b4b993..16c4602b36 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -620,10 +620,7 @@ describe('ExpressInstrumentation', () => { checkCollector: (collector: testUtils.TestCollector) => { const spans = collector.sortedSpans; - assert.strictEqual( - spans[0].name, - 'GET /test,6,/test/' - ); + 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); @@ -634,10 +631,7 @@ describe('ExpressInstrumentation', () => { 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].name, 'request handler - /test,6,/test/'); assert.strictEqual(spans[4].kind, SpanKind.SERVER); assert.strictEqual(spans[4].parentSpanId, spans[0].spanId); },