From d99418be8e29e80f735bcdf8fbb1024a6411c3ec Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Wed, 30 Nov 2022 09:51:17 -0500 Subject: [PATCH 1/5] fix(instrumentation): add back support for absolute paths via `require-in-the-middle` --- .../src/platform/node/instrumentation.ts | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index d80985431c..e11e76d0c1 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -21,6 +21,7 @@ import { InstrumentationAbstract } from '../../instrumentation'; import { RequireInTheMiddleSingleton, Hooked } from './RequireInTheMiddleSingleton'; import { InstrumentationModuleDefinition } from './types'; import { diag } from '@opentelemetry/api'; +import * as RequireInTheMiddle from 'require-in-the-middle'; /** * Base abstract class for instrumenting node plugins @@ -29,7 +30,7 @@ export abstract class InstrumentationBase extends InstrumentationAbstract implements types.Instrumentation { private _modules: InstrumentationModuleDefinition[]; - private _hooks: Hooked[] = []; + private _hooks: (Hooked | RequireInTheMiddle.Hooked)[] = []; private _requireInTheMiddleSingleton: RequireInTheMiddleSingleton = RequireInTheMiddleSingleton.getInstance(); private _enabled = false; @@ -160,21 +161,24 @@ export abstract class InstrumentationBase this._warnOnPreloadedModules(); for (const module of this._modules) { - this._hooks.push( - this._requireInTheMiddleSingleton.register( - module.name, - (exports, name, baseDir) => { - return this._onRequire( - (module as unknown) as InstrumentationModuleDefinition< - typeof exports - >, - exports, - name, - baseDir - ); - } - ) - ); + const onRequire: RequireInTheMiddle.OnRequireFn = (exports, name, baseDir) => { + return this._onRequire( + (module as unknown) as InstrumentationModuleDefinition< + typeof exports + >, + exports, + name, + baseDir + ); + }; + + // `RequireInTheMiddleSingleton` does not support absolute paths. + // For an absolute paths, we must create a separate instance of `RequireInTheMiddle`. + const hook = path.isAbsolute(module.name) + ? RequireInTheMiddle([module.name], { internals: true }, onRequire) + : this._requireInTheMiddleSingleton.register(module.name, onRequire); + + this._hooks.push(hook); } } From b4d2b5618a08e4e34c1cb902b1877c82f0a47b24 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Wed, 30 Nov 2022 10:20:03 -0500 Subject: [PATCH 2/5] chore: add changelog entry --- experimental/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index b28553e51b..51175c9e67 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): add back support for absolute paths via `require-in-the-middle` [#3457](https://github.com/open-telemetry/opentelemetry-js/pull/3457) @mhassan1 ### :books: (Refine Doc) From a309ca7c5b75e84cf6c518b70e4c1d7d9534a0ff Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Wed, 30 Nov 2022 12:58:31 -0500 Subject: [PATCH 3/5] chore(instrumentation): fix indentation --- .../src/platform/node/instrumentation.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index e11e76d0c1..1585c1fa17 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -164,8 +164,8 @@ export abstract class InstrumentationBase const onRequire: RequireInTheMiddle.OnRequireFn = (exports, name, baseDir) => { return this._onRequire( (module as unknown) as InstrumentationModuleDefinition< - typeof exports - >, + typeof exports + >, exports, name, baseDir From 64b9bb8392f64510e051a0dcf96a630fae193fb9 Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Wed, 30 Nov 2022 13:39:28 -0500 Subject: [PATCH 4/5] chore(instrumentation): add enable/disable tests for `InstrumentationBase` --- .../test/node/InstrumentationBase.test.ts | 92 ++++++++++++++++++- .../node/fixtures/absolutePathTestFixture.js | 17 ++++ 2 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/fixtures/absolutePathTestFixture.js diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts index 062088886b..84e3c8efc1 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts @@ -16,7 +16,8 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; -import { InstrumentationBase, InstrumentationModuleDefinition } from '../../src'; +import { InstrumentationBase, InstrumentationModuleDefinition, InstrumentationNodeModuleDefinition, InstrumentationNodeModuleFile } from '../../src'; +import * as path from 'path'; const MODULE_NAME = 'test-module'; const MODULE_FILE_NAME = 'test-module-file'; @@ -254,4 +255,93 @@ describe('InstrumentationBase', () => { }); }); }); + + describe('enable/disable', () => { + describe('AND a normal module name', () => { + type Exports = Record; + type ExportsPatched = Exports & { __patched?: boolean }; + const moduleName = 'net'; + class TestInstrumentation extends InstrumentationBase { + constructor() { + super('@opentelemetry/instrumentation-net-test', '0.0.0', { enabled: false }); + } + init(): InstrumentationNodeModuleDefinition[] { + return [ + new InstrumentationNodeModuleDefinition( + moduleName, + ['*'], + (exports: ExportsPatched) => { + exports.__patched = true; + return exports; + }, + (exports: ExportsPatched) => { + exports.__patched = false; + return exports; + } + ) + ]; + } + } + + const instrumentation = new TestInstrumentation(); + + it('should patch the module', () => { + instrumentation.enable(); + const exportsPatched = require(moduleName); + assert.equal(exportsPatched.__patched, true, 'after enable'); + instrumentation.disable(); + assert.equal(exportsPatched.__patched, false, 'after disable'); + instrumentation.enable(); + assert.equal(exportsPatched.__patched, true, 'after re-enable'); + }); + }); + + describe('AND an absolute path module name', () => { + type Exports = Record; + type ExportsPatched = Exports & { __patched?: boolean }; + const moduleName = 'absolutePathTestFixture'; + const fileName = path.join(__dirname, 'fixtures', `${moduleName}.js`); + class TestInstrumentation extends InstrumentationBase { + constructor() { + super('@opentelemetry/instrumentation-absolute-path-test', '0.0.0', { enabled: false }); + } + init(): InstrumentationNodeModuleDefinition[] { + return [ + new InstrumentationNodeModuleDefinition( + fileName, + ['*'], + undefined, + undefined, + [ + new InstrumentationNodeModuleFile( + moduleName, + ['*'], + (exports: ExportsPatched) => { + exports.__patched = true; + return exports; + }, + (exports?: ExportsPatched) => { + if (exports) exports.__patched = false; + return exports; + } + ) + ], + ) + ]; + } + } + + const instrumentation = new TestInstrumentation(); + + it('should patch the module', () => { + instrumentation.enable(); + const exportsPatched = require(fileName); + assert.equal(exportsPatched.__patched, true, 'after enable'); + instrumentation.disable(); + assert.equal(exportsPatched.__patched, false, 'after disable'); + instrumentation.enable(); + assert.equal(exportsPatched.__patched, true, 'after re-enable'); + }); + }); + }); }); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/fixtures/absolutePathTestFixture.js b/experimental/packages/opentelemetry-instrumentation/test/node/fixtures/absolutePathTestFixture.js new file mode 100644 index 0000000000..0f50219616 --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/fixtures/absolutePathTestFixture.js @@ -0,0 +1,17 @@ +/* + * 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. + */ + +module.exports = {}; From f11f7031afa8912ecf35142a2359a24656565a4e Mon Sep 17 00:00:00 2001 From: mhassan1 Date: Mon, 5 Dec 2022 10:28:45 -0500 Subject: [PATCH 5/5] chore(instrumentation): fix linting errors --- .../src/platform/node/instrumentation.ts | 10 ++++++---- .../test/node/InstrumentationBase.test.ts | 18 +++++++++++------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index c76ee844db..93fbb4c176 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -167,11 +167,13 @@ export abstract class InstrumentationBase this._warnOnPreloadedModules(); for (const module of this._modules) { - const onRequire: RequireInTheMiddle.OnRequireFn = (exports, name, baseDir) => { + const onRequire: RequireInTheMiddle.OnRequireFn = ( + exports, + name, + baseDir + ) => { return this._onRequire( - module as unknown as InstrumentationModuleDefinition< - typeof exports - >, + module as unknown as InstrumentationModuleDefinition, exports, name, baseDir diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts index 1842799440..b9597c65d8 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts @@ -21,7 +21,7 @@ import { InstrumentationBase, InstrumentationModuleDefinition, InstrumentationNodeModuleDefinition, - InstrumentationNodeModuleFile + InstrumentationNodeModuleFile, } from '../../src'; const MODULE_NAME = 'test-module'; @@ -295,7 +295,9 @@ describe('InstrumentationBase', () => { const moduleName = 'net'; class TestInstrumentation extends InstrumentationBase { constructor() { - super('@opentelemetry/instrumentation-net-test', '0.0.0', { enabled: false }); + super('@opentelemetry/instrumentation-net-test', '0.0.0', { + enabled: false, + }); } init(): InstrumentationNodeModuleDefinition[] { return [ @@ -310,7 +312,7 @@ describe('InstrumentationBase', () => { exports.__patched = false; return exports; } - ) + ), ]; } } @@ -335,7 +337,9 @@ describe('InstrumentationBase', () => { const fileName = path.join(__dirname, 'fixtures', `${moduleName}.js`); class TestInstrumentation extends InstrumentationBase { constructor() { - super('@opentelemetry/instrumentation-absolute-path-test', '0.0.0', { enabled: false }); + super('@opentelemetry/instrumentation-absolute-path-test', '0.0.0', { + enabled: false, + }); } init(): InstrumentationNodeModuleDefinition[] { return [ @@ -356,9 +360,9 @@ describe('InstrumentationBase', () => { if (exports) exports.__patched = false; return exports; } - ) - ], - ) + ), + ] + ), ]; } }