From ecd1b307fb121ffb90e7157204b6f1c0ca81a899 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Thu, 11 Aug 2022 15:43:00 -0400 Subject: [PATCH 1/9] feat(instrumentation): implement `require-in-the-middle` singleton --- experimental/CHANGELOG.md | 1 + .../src/platform/node/instrumentation.ts | 9 +- .../node/requireInTheMiddleSingleton.ts | 102 +++++++++++++ .../node/requireInTheMiddleSingleton.test.ts | 141 ++++++++++++++++++ 4 files changed, 248 insertions(+), 5 deletions(-) create mode 100644 experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 9f65421cd2..1b9e5d427e 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -44,6 +44,7 @@ All notable changes to experimental packages in this project will be documented * Add `resourceDetectors` option to `NodeSDK` [#3210](https://github.com/open-telemetry/opentelemetry-js/issues/3210) * feat: add Logs API @mkuba [#3117](https://github.com/open-telemetry/opentelemetry-js/pull/3117) +* feat(instrumentation): implement `require-in-the-middle` singleton [#3161](https://github.com/open-telemetry/opentelemetry-js/pull/3161) @mhassan1 ### :bug: (Bug Fix) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 70dac85332..9b50da51fe 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -16,9 +16,9 @@ import * as types from '../../types'; import * as path from 'path'; -import * as RequireInTheMiddle from 'require-in-the-middle'; import { satisfies } from 'semver'; import { InstrumentationAbstract } from '../../instrumentation'; +import { requireInTheMiddleSingleton, Hooked } from './requireInTheMiddleSingleton'; import { InstrumentationModuleDefinition } from './types'; import { diag } from '@opentelemetry/api'; @@ -29,7 +29,7 @@ export abstract class InstrumentationBase extends InstrumentationAbstract implements types.Instrumentation { private _modules: InstrumentationModuleDefinition[]; - private _hooks: RequireInTheMiddle.Hooked[] = []; + private _hooks: Hooked[] = []; private _enabled = false; constructor( @@ -159,9 +159,8 @@ export abstract class InstrumentationBase this._warnOnPreloadedModules(); for (const module of this._modules) { this._hooks.push( - RequireInTheMiddle( - [module.name], - { internals: true }, + requireInTheMiddleSingleton.register( + module.name, (exports, name, baseDir) => { return this._onRequire( (module as unknown) as InstrumentationModuleDefinition< diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts new file mode 100644 index 0000000000..9185b95fbf --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts @@ -0,0 +1,102 @@ +/* + * 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. + */ + +import * as RequireInTheMiddle from 'require-in-the-middle'; +import * as path from 'path'; + +export type Hooked = { + moduleName: string + onRequire: RequireInTheMiddle.OnRequireFn +}; + +// The version number at the end of this symbol should be incremented whenever there are +// changes (even non-breaking) to the `RequireInTheMiddleSingleton` class's public interface +const RITM_SINGLETON_SYM = Symbol.for('OpenTelemetry.js.sdk.require-in-the-middle.v1'); + +/** + * Singleton class for `require-in-the-middle` + * Allows instrumentation plugins to patch modules with only a single `require` patch + * WARNING: Because this class will be used to create a process-global singleton, + * any change to the public interface of the class (even a non-breaking change like adding a method or argument) + * could break the integration with different versions of `InstrumentationBase`. + * When a change to the public interface of the class is made, + * we should increment the version number at the end of the `RITM_SINGLETON_SYM` symbol. + */ +class RequireInTheMiddleSingleton { + private _modulesToHook: Hooked[] = []; + + constructor() { + this._initialize(); + } + + private _initialize() { + RequireInTheMiddle( + // Intercept all `require` calls; we will filter the matching ones below + null, + { internals: true }, + (exports, name, basedir) => { + // For internal files on Windows, `name` will use backslash as the path separator + const normalizedModuleName = normalizePathSeparators(name); + const matches = this._modulesToHook.filter(({ moduleName: hookedModuleName }) => { + return shouldHook(hookedModuleName, normalizedModuleName); + }); + + for (const { onRequire } of matches) { + exports = onRequire(exports, name, basedir); + } + + return exports; + } + ); + } + + register(moduleName: string, onRequire: RequireInTheMiddle.OnRequireFn): Hooked { + const hooked = { moduleName, onRequire }; + this._modulesToHook.push(hooked); + return hooked; + } + + static getGlobalInstance(): RequireInTheMiddleSingleton { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (global as any)[RITM_SINGLETON_SYM] = (global as any)[RITM_SINGLETON_SYM] ?? new RequireInTheMiddleSingleton(); + } +} + +/** + * Determine whether a `require`d module should be hooked + * + * @param {string} hookedModuleName Hooked module name + * @param {string} requiredModuleName Required module name + * @returns {boolean} Whether to hook the required module + * @private + */ +export function shouldHook(hookedModuleName: string, requiredModuleName: string): boolean { + return requiredModuleName === hookedModuleName || requiredModuleName.startsWith(hookedModuleName + '/'); +} + +/** + * Normalize the path separators to forward slash in a module name or path + * + * @param {string} moduleNameOrPath Module name or path + * @returns {string} Normalized module name or path + */ +function normalizePathSeparators(moduleNameOrPath: string): string { + return path.sep !== '/' + ? moduleNameOrPath.split(path.sep).join('/') + : moduleNameOrPath; +} + +export const requireInTheMiddleSingleton = RequireInTheMiddleSingleton.getGlobalInstance(); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts new file mode 100644 index 0000000000..eecb05a5b8 --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts @@ -0,0 +1,141 @@ +/* + * 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. + */ + +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import * as path from 'path'; +import * as RequireInTheMiddle from 'require-in-the-middle'; +import { requireInTheMiddleSingleton, shouldHook } from '../../src/platform/node/requireInTheMiddleSingleton'; + +type AugmentedExports = { + __ritmOnRequires?: string[] +}; + +const makeOnRequiresStub = (label: string): sinon.SinonStub => sinon.stub().callsFake(((exports: AugmentedExports) => { + exports.__ritmOnRequires ??= []; + exports.__ritmOnRequires.push(label); + return exports; +}) as RequireInTheMiddle.OnRequireFn); + +describe('requireInTheMiddleSingleton', () => { + describe('register', () => { + const onRequireFsStub = makeOnRequiresStub('fs'); + const onRequireFsPromisesStub = makeOnRequiresStub('fs-promises'); + const onRequireCodecovStub = makeOnRequiresStub('codecov'); + const onRequireCodecovLibStub = makeOnRequiresStub('codecov-lib'); + const onRequireCpxStub = makeOnRequiresStub('cpx'); + const onRequireCpxLibStub = makeOnRequiresStub('cpx-lib'); + + before(() => { + requireInTheMiddleSingleton.register('fs', onRequireFsStub); + requireInTheMiddleSingleton.register('fs/promises', onRequireFsPromisesStub); + requireInTheMiddleSingleton.register('codecov', onRequireCodecovStub); + requireInTheMiddleSingleton.register('codecov/lib/codecov.js', onRequireCodecovLibStub); + requireInTheMiddleSingleton.register('cpx', onRequireCpxStub); + requireInTheMiddleSingleton.register('cpx/lib/copy-sync.js', onRequireCpxLibStub); + }); + + beforeEach(() => { + onRequireFsStub.resetHistory(); + onRequireFsPromisesStub.resetHistory(); + onRequireCodecovStub.resetHistory(); + onRequireCodecovLibStub.resetHistory(); + onRequireCpxStub.resetHistory(); + onRequireCpxLibStub.resetHistory(); + }); + + it('should return a hooked object', () => { + const moduleName = 'm'; + const onRequire = makeOnRequiresStub('m'); + const hooked = requireInTheMiddleSingleton.register(moduleName, onRequire); + assert.deepStrictEqual(hooked, { moduleName, onRequire }); + }); + + describe('core module', () => { + describe('AND module name matches', () => { + it('should call `onRequire`', () => { + const exports = require('fs'); + assert.deepStrictEqual(exports.__ritmOnRequires, ['fs']); + sinon.assert.calledOnceWithExactly(onRequireFsStub, exports, 'fs', undefined); + sinon.assert.notCalled(onRequireFsPromisesStub); + }); + }); + describe('AND module name does not match', () => { + it('should not call `onRequire`', () => { + const exports = require('crypto'); + assert.equal(exports.__ritmOnRequires, undefined); + sinon.assert.notCalled(onRequireFsStub); + }); + }); + }); + + describe('core module with sub-path', () => { + describe('AND module name matches', () => { + it('should call `onRequire`', () => { + const exports = require('fs/promises'); + assert.deepStrictEqual(exports.__ritmOnRequires, ['fs', 'fs-promises']); + sinon.assert.calledOnceWithExactly(onRequireFsPromisesStub, exports, 'fs/promises', undefined); + sinon.assert.calledOnceWithMatch(onRequireFsStub, { __ritmOnRequires: ['fs', 'fs-promises'] }, 'fs/promises', undefined); + }); + }); + }); + + describe('non-core module', () => { + describe('AND module name matches', () => { + const baseDir = path.dirname(require.resolve('codecov')); + const modulePath = path.join('codecov', 'lib', 'codecov.js'); + it('should call `onRequire`', () => { + const exports = require('codecov'); + assert.deepStrictEqual(exports.__ritmOnRequires, ['codecov']); + sinon.assert.calledWithExactly(onRequireCodecovStub, exports, 'codecov', baseDir); + sinon.assert.calledWithMatch(onRequireCodecovStub, { __ritmOnRequires: ['codecov', 'codecov-lib'] }, modulePath, baseDir); + sinon.assert.calledWithMatch(onRequireCodecovLibStub, { __ritmOnRequires: ['codecov', 'codecov-lib'] }, modulePath, baseDir); + }); + }); + }); + + describe('non-core module with sub-path', () => { + describe('AND module name matches', () => { + const baseDir = path.resolve(path.dirname(require.resolve('cpx')), '..'); + const modulePath = path.join('cpx', 'lib', 'copy-sync.js'); + it('should call `onRequire`', () => { + const exports = require('cpx/lib/copy-sync'); + assert.deepStrictEqual(exports.__ritmOnRequires, ['cpx', 'cpx-lib']); + sinon.assert.calledWithMatch(onRequireCpxStub, { __ritmOnRequires: ['cpx', 'cpx-lib'] }, modulePath, baseDir); + sinon.assert.calledWithExactly(onRequireCpxStub, exports, modulePath, baseDir); + sinon.assert.calledWithExactly(onRequireCpxLibStub, exports, modulePath, baseDir); + }); + }); + }); + }); + + describe('shouldHook', () => { + describe('module that matches', () => { + it('should be hooked', () => { + assert.equal(shouldHook('c', 'c'), true); + assert.equal(shouldHook('c', 'c/d'), true); + assert.equal(shouldHook('c.js', 'c.js'), true); + }); + }); + describe('module that does not match', () => { + it('should not be hooked', () => { + assert.equal(shouldHook('c', 'c.js'), false); + assert.equal(shouldHook('c', 'e'), false); + assert.equal(shouldHook('c.js', 'c'), false); + }); + }); + }); +}); From 7bb8c5dd7010698bf757a2ab3da8d16062b5ce41 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Fri, 16 Sep 2022 09:42:32 -0400 Subject: [PATCH 2/9] feat(instrumentation): create ritm singleton on first instrumentation --- .../src/platform/node/instrumentation.ts | 4 ++-- .../src/platform/node/requireInTheMiddleSingleton.ts | 4 +--- .../test/node/requireInTheMiddleSingleton.test.ts | 4 +++- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 9b50da51fe..bb28b8d554 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -18,7 +18,7 @@ import * as types from '../../types'; import * as path from 'path'; import { satisfies } from 'semver'; import { InstrumentationAbstract } from '../../instrumentation'; -import { requireInTheMiddleSingleton, Hooked } from './requireInTheMiddleSingleton'; +import { RequireInTheMiddleSingleton, Hooked } from './requireInTheMiddleSingleton'; import { InstrumentationModuleDefinition } from './types'; import { diag } from '@opentelemetry/api'; @@ -159,7 +159,7 @@ export abstract class InstrumentationBase this._warnOnPreloadedModules(); for (const module of this._modules) { this._hooks.push( - requireInTheMiddleSingleton.register( + RequireInTheMiddleSingleton.getGlobalInstance().register( module.name, (exports, name, baseDir) => { return this._onRequire( diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts index 9185b95fbf..391c78ef0d 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts @@ -35,7 +35,7 @@ const RITM_SINGLETON_SYM = Symbol.for('OpenTelemetry.js.sdk.require-in-the-middl * When a change to the public interface of the class is made, * we should increment the version number at the end of the `RITM_SINGLETON_SYM` symbol. */ -class RequireInTheMiddleSingleton { +export class RequireInTheMiddleSingleton { private _modulesToHook: Hooked[] = []; constructor() { @@ -98,5 +98,3 @@ function normalizePathSeparators(moduleNameOrPath: string): string { ? moduleNameOrPath.split(path.sep).join('/') : moduleNameOrPath; } - -export const requireInTheMiddleSingleton = RequireInTheMiddleSingleton.getGlobalInstance(); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts index eecb05a5b8..3eea85c6de 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts @@ -18,7 +18,9 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import * as path from 'path'; import * as RequireInTheMiddle from 'require-in-the-middle'; -import { requireInTheMiddleSingleton, shouldHook } from '../../src/platform/node/requireInTheMiddleSingleton'; +import { RequireInTheMiddleSingleton, shouldHook } from '../../src/platform/node/requireInTheMiddleSingleton'; + +const requireInTheMiddleSingleton = RequireInTheMiddleSingleton.getGlobalInstance(); type AugmentedExports = { __ritmOnRequires?: string[] From 8ed3f9a87eb375ea60a6846bb06d9e79e9913966 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Fri, 16 Sep 2022 11:01:53 -0400 Subject: [PATCH 3/9] feat(instrumentation): remove process-global ritm singleton --- .../src/platform/node/instrumentation.ts | 3 ++- .../node/requireInTheMiddleSingleton.ts | 20 ++++++++----------- .../node/requireInTheMiddleSingleton.test.ts | 2 +- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index bb28b8d554..90fc078ed5 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -30,6 +30,7 @@ export abstract class InstrumentationBase implements types.Instrumentation { private _modules: InstrumentationModuleDefinition[]; private _hooks: Hooked[] = []; + private _requireInTheMiddleSingleton: RequireInTheMiddleSingleton = RequireInTheMiddleSingleton.getInstance(); private _enabled = false; constructor( @@ -159,7 +160,7 @@ export abstract class InstrumentationBase this._warnOnPreloadedModules(); for (const module of this._modules) { this._hooks.push( - RequireInTheMiddleSingleton.getGlobalInstance().register( + this._requireInTheMiddleSingleton.register( module.name, (exports, name, baseDir) => { return this._onRequire( diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts index 391c78ef0d..34fe71fa17 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts @@ -22,21 +22,18 @@ export type Hooked = { onRequire: RequireInTheMiddle.OnRequireFn }; -// The version number at the end of this symbol should be incremented whenever there are -// changes (even non-breaking) to the `RequireInTheMiddleSingleton` class's public interface -const RITM_SINGLETON_SYM = Symbol.for('OpenTelemetry.js.sdk.require-in-the-middle.v1'); - /** * Singleton class for `require-in-the-middle` * Allows instrumentation plugins to patch modules with only a single `require` patch - * WARNING: Because this class will be used to create a process-global singleton, - * any change to the public interface of the class (even a non-breaking change like adding a method or argument) - * could break the integration with different versions of `InstrumentationBase`. - * When a change to the public interface of the class is made, - * we should increment the version number at the end of the `RITM_SINGLETON_SYM` symbol. + * WARNING: Because this class will create its own `require-in-the-middle` (RITM) instance, + * we should minimize the number of new instances of this class. + * Multiple instances of `@opentelemetry/instrumentation` (e.g. multiple versions) in a single process + * will result in multiple instances of RITM, which will have an impact + * on the performance of instrumentation hooks being applied. */ export class RequireInTheMiddleSingleton { private _modulesToHook: Hooked[] = []; + private static _instance?: RequireInTheMiddleSingleton; constructor() { this._initialize(); @@ -69,9 +66,8 @@ export class RequireInTheMiddleSingleton { return hooked; } - static getGlobalInstance(): RequireInTheMiddleSingleton { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return (global as any)[RITM_SINGLETON_SYM] = (global as any)[RITM_SINGLETON_SYM] ?? new RequireInTheMiddleSingleton(); + static getInstance(): RequireInTheMiddleSingleton { + return this._instance = this._instance ?? new RequireInTheMiddleSingleton(); } } diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts index 3eea85c6de..4f9641d1b2 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts @@ -20,7 +20,7 @@ import * as path from 'path'; import * as RequireInTheMiddle from 'require-in-the-middle'; import { RequireInTheMiddleSingleton, shouldHook } from '../../src/platform/node/requireInTheMiddleSingleton'; -const requireInTheMiddleSingleton = RequireInTheMiddleSingleton.getGlobalInstance(); +const requireInTheMiddleSingleton = RequireInTheMiddleSingleton.getInstance(); type AugmentedExports = { __ritmOnRequires?: string[] From 6280629ba36f230cba407d4242e0691cdf389a71 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Fri, 16 Sep 2022 11:20:32 -0400 Subject: [PATCH 4/9] feat(instrumentation): do not re-use ritm singleton in mocha --- .../platform/node/requireInTheMiddleSingleton.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts index 34fe71fa17..92c8011e37 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts @@ -22,6 +22,17 @@ export type Hooked = { onRequire: RequireInTheMiddle.OnRequireFn }; +/** + * Whether Mocha is running in this process + * Inspired by https://github.com/AndreasPizsa/detect-mocha + * + * @type {boolean} + */ +const isMocha = ['afterEach','after','beforeEach','before','describe','it'].every(fn => { + // @ts-expect-error TS7053: Element implicitly has an 'any' type + return typeof global[fn] === 'function'; +}); + /** * Singleton class for `require-in-the-middle` * Allows instrumentation plugins to patch modules with only a single `require` patch @@ -67,6 +78,10 @@ export class RequireInTheMiddleSingleton { } static getInstance(): RequireInTheMiddleSingleton { + // Mocha runs all test suites in the same process + // This prevents test suites from sharing a singleton + if (isMocha) return new RequireInTheMiddleSingleton(); + return this._instance = this._instance ?? new RequireInTheMiddleSingleton(); } } From 3c593a88e370f9fb52405e6f70d2f300e0158fd2 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Tue, 20 Sep 2022 09:09:16 -0400 Subject: [PATCH 5/9] fix(instrumentation): make ritm singleton constructor private --- .../src/platform/node/requireInTheMiddleSingleton.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts index 92c8011e37..45a8ca1570 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts @@ -46,7 +46,7 @@ export class RequireInTheMiddleSingleton { private _modulesToHook: Hooked[] = []; private static _instance?: RequireInTheMiddleSingleton; - constructor() { + private constructor() { this._initialize(); } From 8d275a5181cc53bf3c0dd3e26d407722bb39de99 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Tue, 20 Sep 2022 10:17:01 -0400 Subject: [PATCH 6/9] fix(instrumentation): use module name trie for ritm singleton performance --- .../src/platform/node/ModuleNameTrie.ts | 86 +++++++++++++++++++ .../node/requireInTheMiddleSingleton.ts | 26 ++---- .../test/node/ModuleNameTrie.test.ts | 68 +++++++++++++++ .../node/requireInTheMiddleSingleton.test.ts | 19 +--- 4 files changed, 162 insertions(+), 37 deletions(-) create mode 100644 experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts new file mode 100644 index 0000000000..f9af540b16 --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts @@ -0,0 +1,86 @@ +/* + * 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. + */ + +import type { Hooked } from './requireInTheMiddleSingleton'; + +export const ModuleNameSeparator = '/'; + +/** + * Node in a `ModuleNameTrie` + */ +class ModuleNameTrieNode { + hooks: Array<{ hook: Hooked, insertedId: number }> = []; + children: Map = new Map(); +} + +/** + * Trie containing nodes that represent a part of a module name (i.e. the parts separated by forward slash) + */ +export class ModuleNameTrie { + private _trie: ModuleNameTrieNode = new ModuleNameTrieNode(); + private _counter: number = 0; + + /** + * Insert a module hook into the trie + * + * @param {Hooked} hook Hook + */ + insert(hook: Hooked) { + let trieNode = this._trie; + + for (const moduleNamePart of hook.moduleName.split(ModuleNameSeparator)) { + let nextNode = trieNode.children.get(moduleNamePart); + if (!nextNode) { + nextNode = new ModuleNameTrieNode(); + trieNode.children.set(moduleNamePart, nextNode); + } + trieNode = nextNode; + } + trieNode.hooks.push({ hook, insertedId: this._counter++ }); + } + + /** + * Search for matching hooks in the trie + * + * @param {string} moduleName Module name + * @param {boolean} maintainInsertionOrder Whether to return the results in insertion order + * @returns {Hooked[]} Matching hooks + */ + search(moduleName: string, { maintainInsertionOrder }: { maintainInsertionOrder?: boolean } = {}): Hooked[] { + let trieNode = this._trie; + const results: ModuleNameTrieNode['hooks'] = []; + + for (const moduleNamePart of moduleName.split(ModuleNameSeparator)) { + const nextNode = trieNode.children.get(moduleNamePart); + if (!nextNode) { + break; + } + results.push(...nextNode.hooks); + trieNode = nextNode; + } + + if (results.length === 0) { + return []; + } + if (results.length === 1) { + return [results[0].hook]; + } + if (maintainInsertionOrder) { + results.sort((a, b) => a.insertedId - b.insertedId); + } + return results.map(({ hook }) => hook); + } +} diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts index 45a8ca1570..5980726b01 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts @@ -16,6 +16,7 @@ import * as RequireInTheMiddle from 'require-in-the-middle'; import * as path from 'path'; +import { ModuleNameTrie, ModuleNameSeparator } from './ModuleNameTrie'; export type Hooked = { moduleName: string @@ -43,7 +44,7 @@ const isMocha = ['afterEach','after','beforeEach','before','describe','it'].ever * on the performance of instrumentation hooks being applied. */ export class RequireInTheMiddleSingleton { - private _modulesToHook: Hooked[] = []; + private _moduleNameTrie: ModuleNameTrie = new ModuleNameTrie(); private static _instance?: RequireInTheMiddleSingleton; private constructor() { @@ -58,9 +59,8 @@ export class RequireInTheMiddleSingleton { (exports, name, basedir) => { // For internal files on Windows, `name` will use backslash as the path separator const normalizedModuleName = normalizePathSeparators(name); - const matches = this._modulesToHook.filter(({ moduleName: hookedModuleName }) => { - return shouldHook(hookedModuleName, normalizedModuleName); - }); + + const matches = this._moduleNameTrie.search(normalizedModuleName, { maintainInsertionOrder: true }); for (const { onRequire } of matches) { exports = onRequire(exports, name, basedir); @@ -73,7 +73,7 @@ export class RequireInTheMiddleSingleton { register(moduleName: string, onRequire: RequireInTheMiddle.OnRequireFn): Hooked { const hooked = { moduleName, onRequire }; - this._modulesToHook.push(hooked); + this._moduleNameTrie.insert(hooked); return hooked; } @@ -86,18 +86,6 @@ export class RequireInTheMiddleSingleton { } } -/** - * Determine whether a `require`d module should be hooked - * - * @param {string} hookedModuleName Hooked module name - * @param {string} requiredModuleName Required module name - * @returns {boolean} Whether to hook the required module - * @private - */ -export function shouldHook(hookedModuleName: string, requiredModuleName: string): boolean { - return requiredModuleName === hookedModuleName || requiredModuleName.startsWith(hookedModuleName + '/'); -} - /** * Normalize the path separators to forward slash in a module name or path * @@ -105,7 +93,7 @@ export function shouldHook(hookedModuleName: string, requiredModuleName: string) * @returns {string} Normalized module name or path */ function normalizePathSeparators(moduleNameOrPath: string): string { - return path.sep !== '/' - ? moduleNameOrPath.split(path.sep).join('/') + return path.sep !== ModuleNameSeparator + ? moduleNameOrPath.split(path.sep).join(ModuleNameSeparator) : moduleNameOrPath; } diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts new file mode 100644 index 0000000000..f86e4db233 --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts @@ -0,0 +1,68 @@ +/* + * 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. + */ + +import * as assert from 'assert'; +import { Hooked } from '../../src/platform/node/requireInTheMiddleSingleton'; +import { ModuleNameTrie } from '../../src/platform/node/ModuleNameTrie'; + +describe('ModuleNameTrie', () => { + describe('search', () => { + const trie = new ModuleNameTrie(); + const inserts = [ + { moduleName: 'a', onRequire: () => {} }, + { moduleName: 'a/b', onRequire: () => {} }, + { moduleName: 'a', onRequire: () => {} }, + { moduleName: 'a/c', onRequire: () => {} }, + { moduleName: 'd', onRequire: () => {} } + ] as Hooked[]; + inserts.forEach(trie.insert.bind(trie)); + + it('should return a list of exact matches (no results)', () => { + assert.deepEqual(trie.search('e'), []); + }); + + it('should return a list of exact matches (one result)', () => { + assert.deepEqual(trie.search('d'), [inserts[4]]); + }); + + it('should return a list of exact matches (more than one result)', () => { + assert.deepEqual(trie.search('a'), [ + inserts[0], + inserts[2] + ]); + }); + + describe('maintainInsertionOrder = false', () => { + it('should return a list of matches in prefix order', () => { + assert.deepEqual(trie.search('a/b'), [ + inserts[0], + inserts[2], + inserts[1] + ]); + }); + }); + + describe('maintainInsertionOrder = true', () => { + it('should return a list of matches in insertion order', () => { + assert.deepEqual(trie.search('a/b', { maintainInsertionOrder: true }), [ + inserts[0], + inserts[1], + inserts[2] + ]); + }); + }); + }); +}); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts index 4f9641d1b2..e98d57e123 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts @@ -18,7 +18,7 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import * as path from 'path'; import * as RequireInTheMiddle from 'require-in-the-middle'; -import { RequireInTheMiddleSingleton, shouldHook } from '../../src/platform/node/requireInTheMiddleSingleton'; +import { RequireInTheMiddleSingleton } from '../../src/platform/node/requireInTheMiddleSingleton'; const requireInTheMiddleSingleton = RequireInTheMiddleSingleton.getInstance(); @@ -123,21 +123,4 @@ describe('requireInTheMiddleSingleton', () => { }); }); }); - - describe('shouldHook', () => { - describe('module that matches', () => { - it('should be hooked', () => { - assert.equal(shouldHook('c', 'c'), true); - assert.equal(shouldHook('c', 'c/d'), true); - assert.equal(shouldHook('c.js', 'c.js'), true); - }); - }); - describe('module that does not match', () => { - it('should not be hooked', () => { - assert.equal(shouldHook('c', 'c.js'), false); - assert.equal(shouldHook('c', 'e'), false); - assert.equal(shouldHook('c.js', 'c'), false); - }); - }); - }); }); From cfb2f01fc45b5e24b3fea7acfa4a63ea96a72d37 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Tue, 20 Sep 2022 10:24:17 -0400 Subject: [PATCH 7/9] fix(instrumentation): fix filename casing --- .../src/platform/node/ModuleNameTrie.ts | 2 +- ...leSingleton.ts => RequireInTheMiddleSingleton.ts} | 12 ++++++++++++ .../src/platform/node/instrumentation.ts | 2 +- .../test/node/ModuleNameTrie.test.ts | 2 +- ...n.test.ts => RequireInTheMiddleSingleton.test.ts} | 4 ++-- 5 files changed, 17 insertions(+), 5 deletions(-) rename experimental/packages/opentelemetry-instrumentation/src/platform/node/{requireInTheMiddleSingleton.ts => RequireInTheMiddleSingleton.ts} (90%) rename experimental/packages/opentelemetry-instrumentation/test/node/{requireInTheMiddleSingleton.test.ts => RequireInTheMiddleSingleton.test.ts} (98%) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts index f9af540b16..3230fea99c 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/ModuleNameTrie.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import type { Hooked } from './requireInTheMiddleSingleton'; +import type { Hooked } from './RequireInTheMiddleSingleton'; export const ModuleNameSeparator = '/'; diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts similarity index 90% rename from experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts rename to experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts index 5980726b01..812db52b68 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/requireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts @@ -71,12 +71,24 @@ export class RequireInTheMiddleSingleton { ); } + /** + * Register a hook with `require-in-the-middle` + * + * @param {string} moduleName Module name + * @param {RequireInTheMiddle.OnRequireFn} onRequire Hook function + * @returns {Hooked} Registered hook + */ register(moduleName: string, onRequire: RequireInTheMiddle.OnRequireFn): Hooked { const hooked = { moduleName, onRequire }; this._moduleNameTrie.insert(hooked); return hooked; } + /** + * Get the `RequireInTheMiddleSingleton` singleton + * + * @returns {RequireInTheMiddleSingleton} Singleton of `RequireInTheMiddleSingleton` + */ static getInstance(): RequireInTheMiddleSingleton { // Mocha runs all test suites in the same process // This prevents test suites from sharing a singleton diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 90fc078ed5..09b66c60a8 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -18,7 +18,7 @@ import * as types from '../../types'; import * as path from 'path'; import { satisfies } from 'semver'; import { InstrumentationAbstract } from '../../instrumentation'; -import { RequireInTheMiddleSingleton, Hooked } from './requireInTheMiddleSingleton'; +import { RequireInTheMiddleSingleton, Hooked } from './RequireInTheMiddleSingleton'; import { InstrumentationModuleDefinition } from './types'; import { diag } from '@opentelemetry/api'; diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts index f86e4db233..c3d72c89d7 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/ModuleNameTrie.test.ts @@ -15,7 +15,7 @@ */ import * as assert from 'assert'; -import { Hooked } from '../../src/platform/node/requireInTheMiddleSingleton'; +import { Hooked } from '../../src/platform/node/RequireInTheMiddleSingleton'; import { ModuleNameTrie } from '../../src/platform/node/ModuleNameTrie'; describe('ModuleNameTrie', () => { diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts similarity index 98% rename from experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts rename to experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts index e98d57e123..795c62f616 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/requireInTheMiddleSingleton.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts @@ -18,7 +18,7 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import * as path from 'path'; import * as RequireInTheMiddle from 'require-in-the-middle'; -import { RequireInTheMiddleSingleton } from '../../src/platform/node/requireInTheMiddleSingleton'; +import { RequireInTheMiddleSingleton } from '../../src/platform/node/RequireInTheMiddleSingleton'; const requireInTheMiddleSingleton = RequireInTheMiddleSingleton.getInstance(); @@ -32,7 +32,7 @@ const makeOnRequiresStub = (label: string): sinon.SinonStub => sinon.stub().call return exports; }) as RequireInTheMiddle.OnRequireFn); -describe('requireInTheMiddleSingleton', () => { +describe('RequireInTheMiddleSingleton', () => { describe('register', () => { const onRequireFsStub = makeOnRequiresStub('fs'); const onRequireFsPromisesStub = makeOnRequiresStub('fs-promises'); From f497108944caec3203cac8c4bcf35bb0b029f318 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Tue, 20 Sep 2022 11:17:20 -0400 Subject: [PATCH 8/9] fix(instrumentation): increase timeout for non-core module --- .../test/node/RequireInTheMiddleSingleton.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts index 795c62f616..724dced720 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/RequireInTheMiddleSingleton.test.ts @@ -105,7 +105,7 @@ describe('RequireInTheMiddleSingleton', () => { sinon.assert.calledWithExactly(onRequireCodecovStub, exports, 'codecov', baseDir); sinon.assert.calledWithMatch(onRequireCodecovStub, { __ritmOnRequires: ['codecov', 'codecov-lib'] }, modulePath, baseDir); sinon.assert.calledWithMatch(onRequireCodecovLibStub, { __ritmOnRequires: ['codecov', 'codecov-lib'] }, modulePath, baseDir); - }); + }).timeout(30000); }); }); From 840e80b73cd13bafe375d71e6c48fbef6b3e83ae Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Wed, 28 Sep 2022 15:44:35 -0400 Subject: [PATCH 9/9] chore(instrumentation): move ritm singleton changelog entry --- experimental/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index d3922494a6..4e6997a73f 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -8,6 +8,8 @@ All notable changes to experimental packages in this project will be documented ### :rocket: (Enhancement) +* feat(instrumentation): implement `require-in-the-middle` singleton [#3161](https://github.com/open-telemetry/opentelemetry-js/pull/3161) @mhassan1 + ### :bug: (Bug Fix) ### :books: (Refine Doc) @@ -59,7 +61,6 @@ All notable changes to experimental packages in this project will be documented * Add `resourceDetectors` option to `NodeSDK` [#3210](https://github.com/open-telemetry/opentelemetry-js/issues/3210) * feat: add Logs API @mkuba [#3117](https://github.com/open-telemetry/opentelemetry-js/pull/3117) -* feat(instrumentation): implement `require-in-the-middle` singleton [#3161](https://github.com/open-telemetry/opentelemetry-js/pull/3161) @mhassan1 ### :books: (Refine Doc)