From e441287f5a4116a203b4c673eca0976694b1d3c6 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Tue, 29 Nov 2022 11:39:43 -0500 Subject: [PATCH 1/4] fix(instrumentation): only call `onRequire` for full matches on core modules with sub-paths --- .../src/platform/node/ModuleNameTrie.ts | 13 ++++++++-- .../node/RequireInTheMiddleSingleton.ts | 8 ++++++- .../test/node/ModuleNameTrie.test.ts | 24 +++++++++++++++++++ .../node/RequireInTheMiddleSingleton.test.ts | 4 ++-- 4 files changed, 44 insertions(+), 5 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts index 3230fea99c4..4bb5e9bebf0 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts @@ -57,21 +57,30 @@ export class ModuleNameTrie { * * @param {string} moduleName Module name * @param {boolean} maintainInsertionOrder Whether to return the results in insertion order + * @param {boolean} fullOnly Whether to return only full matches * @returns {Hooked[]} Matching hooks */ - search(moduleName: string, { maintainInsertionOrder }: { maintainInsertionOrder?: boolean } = {}): Hooked[] { + search(moduleName: string, { maintainInsertionOrder, fullOnly }: { maintainInsertionOrder?: boolean, fullOnly?: boolean } = {}): Hooked[] { let trieNode = this._trie; const results: ModuleNameTrieNode['hooks'] = []; + let foundFull = true; for (const moduleNamePart of moduleName.split(ModuleNameSeparator)) { const nextNode = trieNode.children.get(moduleNamePart); if (!nextNode) { + foundFull = false; break; } - results.push(...nextNode.hooks); + if (!fullOnly) { + results.push(...nextNode.hooks); + } trieNode = nextNode; } + if (fullOnly && foundFull) { + results.push(...trieNode.hooks); + } + if (results.length === 0) { return []; } diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts index 812db52b683..657a944f6d2 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts @@ -60,7 +60,13 @@ export class RequireInTheMiddleSingleton { // For internal files on Windows, `name` will use backslash as the path separator const normalizedModuleName = normalizePathSeparators(name); - const matches = this._moduleNameTrie.search(normalizedModuleName, { maintainInsertionOrder: true }); + const matches = this._moduleNameTrie.search(normalizedModuleName, { + maintainInsertionOrder: true, + // For core modules (e.g. `fs`), do not match on sub-paths (e.g. `fs/promises'). + // This matches the behavior of `require-in-the-middle`. + // `basedir` is always `undefined` for core modules. + fullOnly: basedir === undefined + }); for (const { onRequire } of matches) { exports = onRequire(exports, name, basedir); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts index c3d72c89d73..a6082e72c8c 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts @@ -64,5 +64,29 @@ describe('ModuleNameTrie', () => { ]); }); }); + + describe('fullOnly = false', () => { + it('should return a list of matches for prefixes', () => { + assert.deepEqual(trie.search('a/b'), [ + inserts[0], + inserts[2], + inserts[1] + ]); + }); + }); + + describe('fullOnly = true', () => { + it('should return a list of matches for full values only', () => { + assert.deepEqual(trie.search('a', { fullOnly: true }), [ + inserts[0], + inserts[2] + ]); + assert.deepEqual(trie.search('a/b', { fullOnly: true }), [ + inserts[1] + ]); + assert.deepEqual(trie.search('e', { fullOnly: true }), []); + assert.deepEqual(trie.search('a/b/e', { fullOnly: true }), []); + }); + }); }); }); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts index 724dced720f..1f82ac0a715 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts @@ -88,9 +88,9 @@ describe('RequireInTheMiddleSingleton', () => { describe('AND module name matches', () => { it('should call `onRequire`', () => { const exports = require('fs/promises'); - assert.deepStrictEqual(exports.__ritmOnRequires, ['fs', 'fs-promises']); + assert.deepStrictEqual(exports.__ritmOnRequires, ['fs-promises']); sinon.assert.calledOnceWithExactly(onRequireFsPromisesStub, exports, 'fs/promises', undefined); - sinon.assert.calledOnceWithMatch(onRequireFsStub, { __ritmOnRequires: ['fs', 'fs-promises'] }, 'fs/promises', undefined); + sinon.assert.notCalled(onRequireFsStub); }); }); }); From 0a59db8b878aeb2ace5d040c5e6a57d72c1e036f Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Tue, 29 Nov 2022 12:09:55 -0500 Subject: [PATCH 2/4] chore: add changelog entry --- experimental/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index b28553e51b2..c30753cafb6 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -15,6 +15,7 @@ All notable changes to experimental packages in this project will be documented ### :bug: (Bug Fix) * fix(instrumentation-xhr): http.url attribute should be absolute [#3200](https://github.com/open-telemetry/opentelemetry-js/pull/3200) @t2t2 +* fix(instrumentation): only call `onRequire` for full matches on core modules with sub-paths [#3451](https://github.com/open-telemetry/opentelemetry-js/pull/3451) @mhassan1 ### :books: (Refine Doc) From ec624ac77a16b4c3ec2b78bdf966849df522fc36 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Mon, 5 Dec 2022 10:21:12 -0500 Subject: [PATCH 3/4] chore(instrumentation): add `ModuleNameTrieSearchOptions` type --- .../src/platform/node/ModuleNameTrie.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts index 1e31f17af36..e971421cda0 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts @@ -26,6 +26,17 @@ class ModuleNameTrieNode { children: Map = new Map(); } +type ModuleNameTrieSearchOptions = { + /** + * Whether to return the results in insertion order + */ + maintainInsertionOrder?: boolean; + /** + * Whether to return only full matches + */ + fullOnly?: boolean; +}; + /** * Trie containing nodes that represent a part of a module name (i.e. the parts separated by forward slash) */ @@ -62,7 +73,7 @@ export class ModuleNameTrie { */ search( moduleName: string, - { maintainInsertionOrder, fullOnly }: { maintainInsertionOrder?: boolean, fullOnly?: boolean } = {} + { maintainInsertionOrder, fullOnly }: ModuleNameTrieSearchOptions = {} ): Hooked[] { let trieNode = this._trie; const results: ModuleNameTrieNode['hooks'] = []; From 92de67856d2c9fef9cd5d079bb13824a3bf09e51 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Mon, 5 Dec 2022 10:24:12 -0500 Subject: [PATCH 4/4] chore(instrumentation): fix linting errors --- .../test/node/ModuleNameTrie.test.ts | 8 +++----- .../test/node/RequireInTheMiddleSingleton.test.ts | 4 +--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts index d88a2106074..304aae996c5 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts @@ -67,7 +67,7 @@ describe('ModuleNameTrie', () => { assert.deepEqual(trie.search('a/b'), [ inserts[0], inserts[2], - inserts[1] + inserts[1], ]); }); }); @@ -76,11 +76,9 @@ describe('ModuleNameTrie', () => { it('should return a list of matches for full values only', () => { assert.deepEqual(trie.search('a', { fullOnly: true }), [ inserts[0], - inserts[2] - ]); - assert.deepEqual(trie.search('a/b', { fullOnly: true }), [ - inserts[1] + inserts[2], ]); + assert.deepEqual(trie.search('a/b', { fullOnly: true }), [inserts[1]]); assert.deepEqual(trie.search('e', { fullOnly: true }), []); assert.deepEqual(trie.search('a/b/e', { fullOnly: true }), []); }); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts index 2689630a3e2..df7059987cb 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts @@ -106,9 +106,7 @@ describe('RequireInTheMiddleSingleton', () => { describe('AND module name matches', () => { it('should call `onRequire`', () => { const exports = require('fs/promises'); - assert.deepStrictEqual(exports.__ritmOnRequires, [ - 'fs-promises', - ]); + assert.deepStrictEqual(exports.__ritmOnRequires, ['fs-promises']); sinon.assert.calledOnceWithExactly( onRequireFsPromisesStub, exports,