From 4ac1ee10195e911df0d1fdbd479332576b2b3812 Mon Sep 17 00:00:00 2001 From: Purvi Kanal Date: Wed, 5 Apr 2023 15:12:02 -0400 Subject: [PATCH 1/5] base unwrap functionality --- .../src/instrumentation.ts | 49 +++++++++++++------ .../test/node/EsmInstrumentation.test.mjs | 14 +++--- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts index 04da0e7ede..399ab471f1 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts @@ -27,6 +27,7 @@ import { import * as shimmer from 'shimmer'; import { InstrumentationModuleDefinition } from './platform/node'; import * as types from './types'; +import * as util from 'util'; /** * Base abstract internal class for instrumenting node and web plugins @@ -63,33 +64,49 @@ export abstract class InstrumentationAbstract /* Api to wrap instrumented method */ protected _wrap = ( moduleExports: any, - name: string, + name: any, wrapper: (originalFn: any) => any ) => { - try { + if (!util.types.isProxy(moduleExports)) { return shimmer.wrap(moduleExports, name, wrapper); - } catch (e) { - // shimmer doesn't handle Proxy objects well - // if there is an error from import in the middle providing - // a Proxy of a Module we have to pass it to shimmer as a regular object - const wrapped: any = shimmer.wrap( - Object.assign({}, moduleExports), - name, - wrapper - ); - Object.defineProperty(moduleExports, name, { - value: wrapped, - }); - return moduleExports; + } else { + return this._wrapEsm(moduleExports, name, wrapper); } }; /* Api to unwrap instrumented methods */ - protected _unwrap = shimmer.unwrap; + protected _unwrap = (moduleExports: any, name: any) => { + if (!util.types.isProxy(moduleExports)) { + return shimmer.unwrap(moduleExports, name); + } else { + return this._unwrapEsm(moduleExports, name); + } + }; /* Api to mass wrap instrumented method */ protected _massWrap = shimmer.massWrap; /* Api to mass unwrap instrumented methods */ protected _massUnwrap = shimmer.massUnwrap; + private _wrapEsm = ( + moduleExports: T, + name: keyof T, + wrapper: (original: ({} & T)[keyof T]) => ({} & T)[keyof T] + ): void => { + const wrapped = shimmer.wrap( + Object.assign({}, moduleExports), + name, + wrapper + ); + Object.defineProperty(moduleExports, name, { + value: wrapped, + }); + }; + + private _unwrapEsm = (moduleExports: T, name: keyof T): void => { + Object.defineProperty(moduleExports, name, { + value: moduleExports[name], + }); + }; + /* Returns meter */ protected get meter(): Meter { return this._meter; diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs index d631b0f1a2..32b0289f64 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs @@ -78,11 +78,11 @@ describe('when loading esm module', () => { assert.deepEqual(exported.testFunction(), 'patched'); }); - // it('should unwrap a patched function', async () => { - // // disable to trigger unwrap - // const exported = await import('test-esm-module'); - // instrumentationWrap.enable(); - // instrumentationWrap.disable(); - // assert.deepEqual(exported.testFunction(), 'original'); - // }); + it('should unwrap a patched function', async () => { + // disable to trigger unwrap + const exported = await import('test-esm-module'); + instrumentationWrap.enable(); + instrumentationWrap.disable(); + assert.deepEqual(exported.testFunction(), 'original'); + }); }); From a346531617623c7319e93e99e056867e210937ac Mon Sep 17 00:00:00 2001 From: Purvi Kanal Date: Wed, 5 Apr 2023 15:24:37 -0400 Subject: [PATCH 2/5] move esm specific wrap logic to node class --- .../src/instrumentation.ts | 42 +----------------- .../src/platform/node/instrumentation.ts | 43 +++++++++++++++++++ 2 files changed, 45 insertions(+), 40 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts index 399ab471f1..4b729fd439 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts @@ -27,7 +27,6 @@ import { import * as shimmer from 'shimmer'; import { InstrumentationModuleDefinition } from './platform/node'; import * as types from './types'; -import * as util from 'util'; /** * Base abstract internal class for instrumenting node and web plugins @@ -62,51 +61,14 @@ export abstract class InstrumentationAbstract } /* Api to wrap instrumented method */ - protected _wrap = ( - moduleExports: any, - name: any, - wrapper: (originalFn: any) => any - ) => { - if (!util.types.isProxy(moduleExports)) { - return shimmer.wrap(moduleExports, name, wrapper); - } else { - return this._wrapEsm(moduleExports, name, wrapper); - } - }; + protected _wrap = shimmer.wrap; /* Api to unwrap instrumented methods */ - protected _unwrap = (moduleExports: any, name: any) => { - if (!util.types.isProxy(moduleExports)) { - return shimmer.unwrap(moduleExports, name); - } else { - return this._unwrapEsm(moduleExports, name); - } - }; + protected _unwrap = shimmer.unwrap; /* Api to mass wrap instrumented method */ protected _massWrap = shimmer.massWrap; /* Api to mass unwrap instrumented methods */ protected _massUnwrap = shimmer.massUnwrap; - private _wrapEsm = ( - moduleExports: T, - name: keyof T, - wrapper: (original: ({} & T)[keyof T]) => ({} & T)[keyof T] - ): void => { - const wrapped = shimmer.wrap( - Object.assign({}, moduleExports), - name, - wrapper - ); - Object.defineProperty(moduleExports, name, { - value: wrapped, - }); - }; - - private _unwrapEsm = (moduleExports: T, name: keyof T): void => { - Object.defineProperty(moduleExports, name, { - value: moduleExports[name], - }); - }; - /* Returns meter */ protected get meter(): Meter { return this._meter; diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index b58e9c48f7..f3ea878f4c 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -27,6 +27,8 @@ import * as ImportInTheMiddle from 'import-in-the-middle'; import { InstrumentationModuleDefinition } from './types'; import { diag } from '@opentelemetry/api'; import * as RequireInTheMiddle from 'require-in-the-middle'; +import * as util from 'util'; +import * as shimmer from 'shimmer'; /** * Base abstract class for instrumenting node plugins @@ -69,6 +71,47 @@ export abstract class InstrumentationBase } } + protected override _wrap = ( + moduleExports: any, + name: any, + wrapper: (originalFn: any) => any + ) => { + if (!util.types.isProxy(moduleExports)) { + return shimmer.wrap(moduleExports, name, wrapper); + } else { + return this._wrapEsm(moduleExports, name, wrapper); + } + }; + + protected override _unwrap = (moduleExports: any, name: any) => { + if (!util.types.isProxy(moduleExports)) { + return shimmer.unwrap(moduleExports, name); + } else { + return this._unwrapEsm(moduleExports, name); + } + }; + + private _wrapEsm = ( + moduleExports: T, + name: keyof T, + wrapper: (original: ({} & T)[keyof T]) => ({} & T)[keyof T] + ): void => { + const wrapped = shimmer.wrap( + Object.assign({}, moduleExports), + name, + wrapper + ); + Object.defineProperty(moduleExports, name, { + value: wrapped, + }); + }; + + private _unwrapEsm = (moduleExports: T, name: keyof T): void => { + Object.defineProperty(moduleExports, name, { + value: moduleExports[name], + }); + }; + private _warnOnPreloadedModules(): void { this._modules.forEach((module: InstrumentationModuleDefinition) => { const { name } = module; From 1ca6b55bade953ab79828cb815092876182d56cd Mon Sep 17 00:00:00 2001 From: Purvi Kanal Date: Thu, 6 Apr 2023 14:05:25 -0400 Subject: [PATCH 3/5] fix types --- .../src/platform/node/instrumentation.ts | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index f3ea878f4c..41f28c79c3 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -16,7 +16,9 @@ import * as types from '../../types'; import * as path from 'path'; +import * as util from 'util'; import { satisfies } from 'semver'; +import * as shimmer from 'shimmer'; import { InstrumentationAbstract } from '../../instrumentation'; import { RequireInTheMiddleSingleton, @@ -27,8 +29,6 @@ import * as ImportInTheMiddle from 'import-in-the-middle'; import { InstrumentationModuleDefinition } from './types'; import { diag } from '@opentelemetry/api'; import * as RequireInTheMiddle from 'require-in-the-middle'; -import * as util from 'util'; -import * as shimmer from 'shimmer'; /** * Base abstract class for instrumenting node plugins @@ -71,10 +71,10 @@ export abstract class InstrumentationBase } } - protected override _wrap = ( - moduleExports: any, - name: any, - wrapper: (originalFn: any) => any + protected override _wrap: typeof shimmer.wrap = ( + moduleExports, + name, + wrapper ) => { if (!util.types.isProxy(moduleExports)) { return shimmer.wrap(moduleExports, name, wrapper); @@ -83,7 +83,7 @@ export abstract class InstrumentationBase } }; - protected override _unwrap = (moduleExports: any, name: any) => { + protected override _unwrap: typeof shimmer.unwrap = (moduleExports, name) => { if (!util.types.isProxy(moduleExports)) { return shimmer.unwrap(moduleExports, name); } else { @@ -91,11 +91,7 @@ export abstract class InstrumentationBase } }; - private _wrapEsm = ( - moduleExports: T, - name: keyof T, - wrapper: (original: ({} & T)[keyof T]) => ({} & T)[keyof T] - ): void => { + private _wrapEsm: typeof shimmer.wrap = (moduleExports, name, wrapper) => { const wrapped = shimmer.wrap( Object.assign({}, moduleExports), name, @@ -106,7 +102,7 @@ export abstract class InstrumentationBase }); }; - private _unwrapEsm = (moduleExports: T, name: keyof T): void => { + private _unwrapEsm: typeof shimmer.unwrap = (moduleExports, name) => { Object.defineProperty(moduleExports, name, { value: moduleExports[name], }); From 44015765098ead55294f819ee5ea780ee2fbd9d7 Mon Sep 17 00:00:00 2001 From: Purvi Kanal Date: Thu, 6 Apr 2023 14:21:41 -0400 Subject: [PATCH 4/5] add masswrap & massunwrap --- .../src/platform/node/instrumentation.ts | 23 ++++++++ .../test/node/EsmInstrumentation.test.mjs | 54 ++++++++++++++++++- .../node_modules/test-esm-module/src/index.js | 4 ++ 3 files changed, 79 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 41f28c79c3..d9cc8d9cee 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -108,6 +108,29 @@ export abstract class InstrumentationBase }); }; + protected override _massWrap: typeof shimmer.massWrap = ( + moduleExportsArray, + names, + wrapper + ) => { + moduleExportsArray.forEach(moduleExports => { + names.forEach(name => { + this._wrap(moduleExports, name, wrapper); + }); + }); + }; + + protected override _massUnwrap: typeof shimmer.massUnwrap = ( + moduleExportsArray, + names + ) => { + moduleExportsArray.forEach(moduleExports => { + names.forEach(name => { + this._unwrap(moduleExports, name); + }); + }); + }; + private _warnOnPreloadedModules(): void { this._modules.forEach((module: InstrumentationModuleDefinition) => { const { name } = module; diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs index 32b0289f64..f09097cd79 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs @@ -44,6 +44,36 @@ class TestInstrumentationWrapFn extends InstrumentationBase { } } +class TestInstrumentationMasswrapFn extends InstrumentationBase { + constructor(config) { + super('test-esm-instrumentation', '0.0.1', config); + } + init() { + console.log('test-esm-instrumentation initialized!'); + return new InstrumentationNodeModuleDefinition( + 'test-esm-module', + ['*'], + moduleExports => { + this._massWrap( + [moduleExports], + ['testFunction', 'secondTestFunction'], + () => { + return () => 'patched'; + } + ); + return moduleExports; + }, + moduleExports => { + this._massUnwrap( + [moduleExports], + ['testFunction', 'secondTestFunction'] + ); + return moduleExports; + } + ); + } +} + class TestInstrumentationSimple extends InstrumentationBase { constructor(config) { super('test-esm-instrumentation', '0.0.1', config); @@ -79,10 +109,30 @@ describe('when loading esm module', () => { }); it('should unwrap a patched function', async () => { - // disable to trigger unwrap - const exported = await import('test-esm-module'); instrumentationWrap.enable(); + // disable to trigger unwrap instrumentationWrap.disable(); assert.deepEqual(exported.testFunction(), 'original'); }); + + it('should wrap multiple functions with masswrap', () => { + const instrumentation = new TestInstrumentationMasswrapFn({ + enabled: false, + }); + + instrumentation.enable(); + assert.deepEqual(exported.testFunction(), 'patched'); + assert.deepEqual(exported.secondTestFunction(), 'patched'); + }); + + it('should unwrap multiple functions with massunwrap', () => { + const instrumentation = new TestInstrumentationMasswrapFn({ + enabled: false, + }); + + instrumentation.enable(); + instrumentation.disable(); + assert.deepEqual(exported.testFunction(), 'original'); + assert.deepEqual(exported.secondTestFunction(), 'original'); + }); }); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/test-esm-module/src/index.js b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/test-esm-module/src/index.js index f4c372bfc0..2fb6c6ed25 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/test-esm-module/src/index.js +++ b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/test-esm-module/src/index.js @@ -2,4 +2,8 @@ export const testFunction = () => { return 'original'; }; +export const secondTestFunction = () => { + return 'original'; +}; + export const testConstant = 42; From 86b68373046ed64212048a1d7516ac880cc7e874 Mon Sep 17 00:00:00 2001 From: Purvi Kanal Date: Thu, 6 Apr 2023 14:47:05 -0400 Subject: [PATCH 5/5] handle edge cases for masswrap and massunwrap --- .../src/platform/node/instrumentation.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index d9cc8d9cee..ce305c0109 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -113,6 +113,18 @@ export abstract class InstrumentationBase names, wrapper ) => { + if (!moduleExportsArray) { + diag.error('must provide one or more modules to patch'); + return; + } else if (!Array.isArray(moduleExportsArray)) { + moduleExportsArray = [moduleExportsArray]; + } + + if (!(names && Array.isArray(names))) { + diag.error('must provide one or more functions to wrap on modules'); + return; + } + moduleExportsArray.forEach(moduleExports => { names.forEach(name => { this._wrap(moduleExports, name, wrapper); @@ -124,6 +136,18 @@ export abstract class InstrumentationBase moduleExportsArray, names ) => { + if (!moduleExportsArray) { + diag.error('must provide one or more modules to patch'); + return; + } else if (!Array.isArray(moduleExportsArray)) { + moduleExportsArray = [moduleExportsArray]; + } + + if (!(names && Array.isArray(names))) { + diag.error('must provide one or more functions to wrap on modules'); + return; + } + moduleExportsArray.forEach(moduleExports => { names.forEach(name => { this._unwrap(moduleExports, name);