From 793301819b08a775e8e1f30073ea3fce8ff4ac2b Mon Sep 17 00:00:00 2001 From: Olivier Albertini Date: Thu, 5 Sep 2019 13:20:51 -0400 Subject: [PATCH 1/2] feat(plugin): add supportedVersions property Closes #132 Signed-off-by: Olivier Albertini --- .../src/trace/instrumentation/BasePlugin.ts | 1 + .../src/instrumentation/PluginLoader.ts | 7 +++- .../src/instrumentation/utils.ts | 13 ++++++ .../test/instrumentation/utils.test.ts | 41 +++++++++++++++++++ .../src/trace/instrumentation/Plugin.ts | 8 ++++ 5 files changed, 68 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts b/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts index 90631c7830b..b50f739481e 100644 --- a/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts +++ b/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts @@ -21,6 +21,7 @@ export abstract class BasePlugin implements Plugin { protected _moduleExports!: T; protected _tracer!: Tracer; protected _logger!: Logger; + supportedVersions?: string[]; enable( moduleExports: T, diff --git a/packages/opentelemetry-node-tracer/src/instrumentation/PluginLoader.ts b/packages/opentelemetry-node-tracer/src/instrumentation/PluginLoader.ts index 7137983bfae..20a680a097e 100644 --- a/packages/opentelemetry-node-tracer/src/instrumentation/PluginLoader.ts +++ b/packages/opentelemetry-node-tracer/src/instrumentation/PluginLoader.ts @@ -90,8 +90,6 @@ export class PluginLoader { `PluginLoader#load: trying loading ${name}.${version}` ); - // @todo (issues/132): Check if version and supportedVersions are - // satisfied if (!version) return exports; this.logger.debug( @@ -101,6 +99,11 @@ export class PluginLoader { // Expecting a plugin from module; try { const plugin: Plugin = require(moduleName).plugin; + + if (!utils.isSupportedVersion(version, plugin.supportedVersions)) { + return exports; + } + this._plugins.push(plugin); // Enable each supported plugin. return plugin.enable(exports, this.tracer, this.logger); diff --git a/packages/opentelemetry-node-tracer/src/instrumentation/utils.ts b/packages/opentelemetry-node-tracer/src/instrumentation/utils.ts index de9648e3024..1ea90230142 100644 --- a/packages/opentelemetry-node-tracer/src/instrumentation/utils.ts +++ b/packages/opentelemetry-node-tracer/src/instrumentation/utils.ts @@ -60,6 +60,19 @@ export function getPackageVersion( } } +export function isSupportedVersion( + moduleVersion: string, + supportedVersions?: string[] +) { + if (!Array.isArray(supportedVersions)) { + return true; + } + + return supportedVersions.some(supportedVersion => + semver.satisfies(moduleVersion, supportedVersion) + ); +} + /** * Adds a search path for plugin modules. Intended for testing purposes only. * @param searchPath The path to add. diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/utils.test.ts b/packages/opentelemetry-node-tracer/test/instrumentation/utils.test.ts index 66d25390c59..c90bb469c90 100644 --- a/packages/opentelemetry-node-tracer/test/instrumentation/utils.test.ts +++ b/packages/opentelemetry-node-tracer/test/instrumentation/utils.test.ts @@ -69,4 +69,45 @@ describe('Instrumentation#utils', () => { }); }); }); + describe('isSupportedVersion', () => { + const version = '1.0.1'; + + it('should return true when supportedVersions is not defined', () => { + assert.strictEqual(utils.isSupportedVersion('1.0.0', undefined), true); + }); + + [ + ['1.X'], + [version], + ['1.X.X', '3.X.X'], + ['^1.0.0'], + ['~1.0.0', '^0.1.0'], + ['*'], + ['>1.0.0'], + ].forEach(supportedVersion => { + it(`should return true when version is equal to ${version} and supportedVersions is equal to ${supportedVersion}`, () => { + assert.strictEqual( + utils.isSupportedVersion(version, supportedVersion), + true + ); + }); + }); + + [['0.X'], ['0.0.1'], ['0.X.X'], ['^0.1.0'], ['1.0.0'], ['<1.0.0']].forEach( + supportedVersion => { + it(`should return false when version is equal to ${version} and supportedVersions is equal to ${supportedVersion}`, () => { + assert.strictEqual( + utils.isSupportedVersion(version, supportedVersion), + false + ); + }); + } + ); + + it(`should return false when version is equal to null and supportedVersions is equal to '*'`, () => { + /* tslint:disable:no-any */ + assert.strictEqual(utils.isSupportedVersion(null as any, ['*']), false); + /* tslint:enable:no-any */ + }); + }); }); diff --git a/packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts b/packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts index 643a4d45291..0c1174d963e 100644 --- a/packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts +++ b/packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts @@ -20,6 +20,14 @@ import { Logger } from '../../common/Logger'; /** Interface Plugin to apply patch. */ // tslint:disable-next-line:no-any export interface Plugin { + /** + * Contains all supported versions. + * all versions must be compatible with [semver](https://semver.org/spec/v2.0.0.html) format. + * If the version is not supported, we won't apply instrumentation patch (see `enable` method). + * If not defined, we will apply instrumentation for every version. + */ + supportedVersions?: string[]; + /** * Method that enables the instrumentation patch. * @param moduleExports The value of the `module.exports` property that would From 8547b6ddf01b1fd8f0ee33f6a4bc6b210875e84c Mon Sep 17 00:00:00 2001 From: Olivier Albertini Date: Thu, 5 Sep 2019 18:01:12 -0400 Subject: [PATCH 2/2] fix: add recommendations from markwolff from mayurkale22 Signed-off-by: Olivier Albertini --- .../src/instrumentation/utils.ts | 7 ++++- .../test/instrumentation/PluginLoader.test.ts | 31 +++++++++++++++++++ .../plugin-notsupported-module/index.js | 5 +++ .../plugin-notsupported-module/package.json | 4 +++ .../simple-module.js | 24 ++++++++++++++ .../plugin-supported-module/index.js | 5 +++ .../plugin-supported-module/package.json | 4 +++ .../plugin-supported-module/simple-module.js | 24 ++++++++++++++ .../node_modules/notsupported-module/index.js | 4 +++ .../notsupported-module/package.json | 4 +++ .../node_modules/supported-module/index.js | 4 +++ .../supported-module/package.json | 4 +++ .../test/instrumentation/utils.test.ts | 4 +-- .../src/trace/instrumentation/Plugin.ts | 4 +-- 14 files changed, 123 insertions(+), 5 deletions(-) create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/index.js create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/package.json create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/simple-module.js create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/index.js create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/package.json create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/simple-module.js create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/notsupported-module/index.js create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/notsupported-module/package.json create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/supported-module/index.js create mode 100644 packages/opentelemetry-node-tracer/test/instrumentation/node_modules/supported-module/package.json diff --git a/packages/opentelemetry-node-tracer/src/instrumentation/utils.ts b/packages/opentelemetry-node-tracer/src/instrumentation/utils.ts index 1ea90230142..fd3cb6a2127 100644 --- a/packages/opentelemetry-node-tracer/src/instrumentation/utils.ts +++ b/packages/opentelemetry-node-tracer/src/instrumentation/utils.ts @@ -60,11 +60,16 @@ export function getPackageVersion( } } +/** + * Determines if a version is supported + * @param moduleVersion a version in [semver](https://semver.org/spec/v2.0.0.html) format. + * @param [supportedVersions] a list of supported versions ([semver](https://semver.org/spec/v2.0.0.html) format). + */ export function isSupportedVersion( moduleVersion: string, supportedVersions?: string[] ) { - if (!Array.isArray(supportedVersions)) { + if (!Array.isArray(supportedVersions) || supportedVersions.length === 0) { return true; } diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/PluginLoader.test.ts b/packages/opentelemetry-node-tracer/test/instrumentation/PluginLoader.test.ts index 66961790e68..b90b6763e11 100644 --- a/packages/opentelemetry-node-tracer/test/instrumentation/PluginLoader.test.ts +++ b/packages/opentelemetry-node-tracer/test/instrumentation/PluginLoader.test.ts @@ -63,8 +63,17 @@ describe('PluginLoader', () => { it('sanity check', () => { // Ensure that module fixtures contain values that we expect. const simpleModule = require('simple-module'); + const simpleModule001 = require('supported-module'); + const simpleModule100 = require('notsupported-module'); + assert.strictEqual(simpleModule.name(), 'simple-module'); + assert.strictEqual(simpleModule001.name(), 'supported-module'); + assert.strictEqual(simpleModule100.name(), 'notsupported-module'); + assert.strictEqual(simpleModule.value(), 0); + assert.strictEqual(simpleModule001.value(), 0); + assert.strictEqual(simpleModule100.value(), 0); + assert.throws(() => require('nonexistent-module')); }); @@ -79,6 +88,28 @@ describe('PluginLoader', () => { assert.strictEqual(simpleModule.name(), 'patched-simple-module'); pluginLoader.unload(); }); + // @TODO: simplify this test once we can load module with custom path + it('should not load the plugin when supported versions does not match', () => { + const pluginLoader = new PluginLoader(tracer, logger); + assert.strictEqual(pluginLoader['_plugins'].length, 0); + pluginLoader.load({ 'notsupported-module': true }); + // The hook is only called the first time the module is loaded. + require('notsupported-module'); + assert.strictEqual(pluginLoader['_plugins'].length, 0); + pluginLoader.unload(); + }); + // @TODO: simplify this test once we can load module with custom path + it('should load a plugin and patch the target modules when supported versions match', () => { + const pluginLoader = new PluginLoader(tracer, logger); + assert.strictEqual(pluginLoader['_plugins'].length, 0); + pluginLoader.load({ 'supported-module': true }); + // The hook is only called the first time the module is loaded. + const simpleModule = require('supported-module'); + assert.strictEqual(pluginLoader['_plugins'].length, 1); + assert.strictEqual(simpleModule.value(), 1); + assert.strictEqual(simpleModule.name(), 'patched-supported-module'); + pluginLoader.unload(); + }); it('should not load a plugin when value is false', () => { const pluginLoader = new PluginLoader(tracer, logger); diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/index.js b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/index.js new file mode 100644 index 00000000000..1b22b5ce904 --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/index.js @@ -0,0 +1,5 @@ +function __export(m) { + for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p]; +} +Object.defineProperty(exports, "__esModule", { value: true }); +__export(require("./simple-module")); diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/package.json b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/package.json new file mode 100644 index 00000000000..4db9e49b1d5 --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/package.json @@ -0,0 +1,4 @@ +{ + "name": "@opentelemetry/plugin-notsupported-module", + "version": "1.0.0" +} diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/simple-module.js b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/simple-module.js new file mode 100644 index 00000000000..86a27bc32d9 --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-notsupported-module/simple-module.js @@ -0,0 +1,24 @@ +Object.defineProperty(exports, "__esModule", { value: true }); +const core_1 = require("@opentelemetry/core"); +const shimmer = require("shimmer"); + +class SimpleModulePlugin extends core_1.BasePlugin { + constructor() { + super(); + } + + patch() { + shimmer.wrap(this._moduleExports, 'name', orig => () => 'patched-' + orig.apply()); + shimmer.wrap(this._moduleExports, 'value', orig => () => orig.apply() + 1); + return this._moduleExports; + } + + unpatch() { + shimmer.unwrap(this._moduleExports, 'name'); + shimmer.unwrap(this._moduleExports, 'value'); + } +} +exports.SimpleModulePlugin = SimpleModulePlugin; +const plugin = new SimpleModulePlugin(); +plugin.supportedVersions = ['1.0.0']; +exports.plugin = plugin; diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/index.js b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/index.js new file mode 100644 index 00000000000..1b22b5ce904 --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/index.js @@ -0,0 +1,5 @@ +function __export(m) { + for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p]; +} +Object.defineProperty(exports, "__esModule", { value: true }); +__export(require("./simple-module")); diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/package.json b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/package.json new file mode 100644 index 00000000000..ca18bafa639 --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/package.json @@ -0,0 +1,4 @@ +{ + "name": "@opentelemetry/plugin-supported-module", + "version": "0.0.1" +} diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/simple-module.js b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/simple-module.js new file mode 100644 index 00000000000..fb2a965e7f3 --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/@opentelemetry/plugin-supported-module/simple-module.js @@ -0,0 +1,24 @@ +Object.defineProperty(exports, "__esModule", { value: true }); +const core_1 = require("@opentelemetry/core"); +const shimmer = require("shimmer"); + +class SimpleModulePlugin extends core_1.BasePlugin { + constructor() { + super(); + } + + patch() { + shimmer.wrap(this._moduleExports, 'name', orig => () => 'patched-' + orig.apply()); + shimmer.wrap(this._moduleExports, 'value', orig => () => orig.apply() + 1); + return this._moduleExports; + } + + unpatch() { + shimmer.unwrap(this._moduleExports, 'name'); + shimmer.unwrap(this._moduleExports, 'value'); + } +} +exports.SimpleModulePlugin = SimpleModulePlugin; +const plugin = new SimpleModulePlugin(); +plugin.supportedVersions = ['^0.0.1']; +exports.plugin = plugin; diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/notsupported-module/index.js b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/notsupported-module/index.js new file mode 100644 index 00000000000..4fe98dae334 --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/notsupported-module/index.js @@ -0,0 +1,4 @@ +module.exports = { + name: () => 'notsupported-module', + value: () => 0, +}; diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/notsupported-module/package.json b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/notsupported-module/package.json new file mode 100644 index 00000000000..9494b2866ef --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/notsupported-module/package.json @@ -0,0 +1,4 @@ +{ + "name": "notsupported-module", + "version": "0.0.1" +} diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/supported-module/index.js b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/supported-module/index.js new file mode 100644 index 00000000000..090d0db5fbd --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/supported-module/index.js @@ -0,0 +1,4 @@ +module.exports = { + name: () => 'supported-module', + value: () => 0, +}; diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/supported-module/package.json b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/supported-module/package.json new file mode 100644 index 00000000000..ffd520afdaa --- /dev/null +++ b/packages/opentelemetry-node-tracer/test/instrumentation/node_modules/supported-module/package.json @@ -0,0 +1,4 @@ +{ + "name": "supported-module", + "version": "0.0.1" +} diff --git a/packages/opentelemetry-node-tracer/test/instrumentation/utils.test.ts b/packages/opentelemetry-node-tracer/test/instrumentation/utils.test.ts index c90bb469c90..fd1d76754ff 100644 --- a/packages/opentelemetry-node-tracer/test/instrumentation/utils.test.ts +++ b/packages/opentelemetry-node-tracer/test/instrumentation/utils.test.ts @@ -84,6 +84,7 @@ describe('Instrumentation#utils', () => { ['~1.0.0', '^0.1.0'], ['*'], ['>1.0.0'], + [], ].forEach(supportedVersion => { it(`should return true when version is equal to ${version} and supportedVersions is equal to ${supportedVersion}`, () => { assert.strictEqual( @@ -105,9 +106,8 @@ describe('Instrumentation#utils', () => { ); it(`should return false when version is equal to null and supportedVersions is equal to '*'`, () => { - /* tslint:disable:no-any */ + // tslint:disable-next-line:no-any assert.strictEqual(utils.isSupportedVersion(null as any, ['*']), false); - /* tslint:enable:no-any */ }); }); }); diff --git a/packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts b/packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts index 0c1174d963e..5a51ef8c9a3 100644 --- a/packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts +++ b/packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts @@ -22,9 +22,9 @@ import { Logger } from '../../common/Logger'; export interface Plugin { /** * Contains all supported versions. - * all versions must be compatible with [semver](https://semver.org/spec/v2.0.0.html) format. + * All versions must be compatible with [semver](https://semver.org/spec/v2.0.0.html) format. * If the version is not supported, we won't apply instrumentation patch (see `enable` method). - * If not defined, we will apply instrumentation for every version. + * If omitted, all versions of the module will be patched. */ supportedVersions?: string[];