From f31ee6dc180cb5bfde6e4dde98bc88f769fdf5ca Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sun, 29 Dec 2019 22:12:51 +0100 Subject: [PATCH 1/6] feat: warn user when a instrumented package was already required #636 --- .../src/instrumentation/PluginLoader.ts | 10 ++++++++ .../test/instrumentation/PluginLoader.test.ts | 24 +++++++++++++++++++ .../already-require-module/index.js | 4 ++++ .../already-require-module/package.json | 4 ++++ 4 files changed, 42 insertions(+) create mode 100644 packages/opentelemetry-node/test/instrumentation/node_modules/already-require-module/index.js create mode 100644 packages/opentelemetry-node/test/instrumentation/node_modules/already-require-module/package.json diff --git a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts index d9d271acfc..15d5f8e58f 100644 --- a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts +++ b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts @@ -83,6 +83,16 @@ export class PluginLoader { return this; } + const alreadyRequiredModules = Object.keys(require.cache); + const requiredModulesToHook = modulesToHook.filter(name => + alreadyRequiredModules.some(cached => cached.includes(name)) + ); + if (requiredModulesToHook.length > 0) { + this.logger.warn( + `Some modules (${requiredModulesToHook}) were already required when their respective plugin were loaded, some plugins might not work. Make sure the SDK is setup before you require in other modules.` + ); + } + // Enable the require hook. hook(modulesToHook, (exports, name, baseDir) => { if (this._hookState !== HookState.ENABLED) return exports; diff --git a/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts b/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts index ad4edb7863..a9f67467ca 100644 --- a/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts +++ b/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts @@ -86,6 +86,13 @@ const notSupportedVersionPlugins: Plugins = { }, }; +const alreadyRequiredPlugins: Plugins = { + 'already-require-module': { + enabled: true, + path: '@opentelemetry/plugin-supported-module', + }, +}; + describe('PluginLoader', () => { const provider = new NoopTracerProvider(); const logger = new NoopLogger(); @@ -219,6 +226,23 @@ describe('PluginLoader', () => { assert.strictEqual(require('simple-module').value(), 0); pluginLoader.unload(); }); + + it(`should warn when module was already loaded`, callback => { + const verifyWarnLogger = { + error: logger.error, + info: logger.info, + debug: logger.debug, + warn: (message: string, ...args: unknown[]) => { + assert(message.match(/were already required when/)); + assert(message.match(/(already-require-module)/)); + return callback(); + }, + }; + require('already-require-module'); + const pluginLoader = new PluginLoader(tracer, verifyWarnLogger); + pluginLoader.load(alreadyRequiredPlugins); + pluginLoader.unload(); + }); }); describe('.unload()', () => { diff --git a/packages/opentelemetry-node/test/instrumentation/node_modules/already-require-module/index.js b/packages/opentelemetry-node/test/instrumentation/node_modules/already-require-module/index.js new file mode 100644 index 0000000000..18c0f69a3b --- /dev/null +++ b/packages/opentelemetry-node/test/instrumentation/node_modules/already-require-module/index.js @@ -0,0 +1,4 @@ +module.exports = { + name: () => 'already-module', + value: () => 0, +}; diff --git a/packages/opentelemetry-node/test/instrumentation/node_modules/already-require-module/package.json b/packages/opentelemetry-node/test/instrumentation/node_modules/already-require-module/package.json new file mode 100644 index 0000000000..7ae0ab8f09 --- /dev/null +++ b/packages/opentelemetry-node/test/instrumentation/node_modules/already-require-module/package.json @@ -0,0 +1,4 @@ +{ + "name": "already-module", + "version": "0.1.0" +} From 9f8e03a212dfb8eba0cd457034923eb68367ffca Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sun, 5 Jan 2020 13:24:04 +0100 Subject: [PATCH 2/6] chore: address PR comments --- .../opentelemetry-node/src/instrumentation/PluginLoader.ts | 4 +++- .../test/instrumentation/PluginLoader.test.ts | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts index 15d5f8e58f..2838d4590b 100644 --- a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts +++ b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts @@ -89,7 +89,9 @@ export class PluginLoader { ); if (requiredModulesToHook.length > 0) { this.logger.warn( - `Some modules (${requiredModulesToHook}) were already required when their respective plugin were loaded, some plugins might not work. Make sure the SDK is setup before you require in other modules.` + `Some modules (${requiredModulesToHook.join( + ', ' + )}) were already required when their respective plugin was loaded, some plugins might not work. Make sure the SDK is setup before you require in other modules.` ); } diff --git a/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts b/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts index a9f67467ca..153010d5f7 100644 --- a/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts +++ b/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts @@ -239,7 +239,7 @@ describe('PluginLoader', () => { }, }; require('already-require-module'); - const pluginLoader = new PluginLoader(tracer, verifyWarnLogger); + const pluginLoader = new PluginLoader(registry, verifyWarnLogger); pluginLoader.load(alreadyRequiredPlugins); pluginLoader.unload(); }); From e9f5f91077812c22580d0295ce219e21d2b307b2 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sun, 26 Jan 2020 11:37:48 +0100 Subject: [PATCH 3/6] chore: extract package name from require.cache --- .../src/instrumentation/PluginLoader.ts | 4 ++- .../src/instrumentation/utils.ts | 27 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts index 2838d4590b..f1f3dee847 100644 --- a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts +++ b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts @@ -85,7 +85,9 @@ export class PluginLoader { const alreadyRequiredModules = Object.keys(require.cache); const requiredModulesToHook = modulesToHook.filter(name => - alreadyRequiredModules.some(cached => cached.includes(name)) + alreadyRequiredModules.some( + cached => utils.packageNameFromPath(cached) === name + ) ); if (requiredModulesToHook.length > 0) { this.logger.warn( diff --git a/packages/opentelemetry-node/src/instrumentation/utils.ts b/packages/opentelemetry-node/src/instrumentation/utils.ts index 062682e3fb..33a56391f4 100644 --- a/packages/opentelemetry-node/src/instrumentation/utils.ts +++ b/packages/opentelemetry-node/src/instrumentation/utils.ts @@ -72,3 +72,30 @@ export function isSupportedVersion( export function searchPathForTest(searchPath: string) { module.paths.push(searchPath); } + +// Includes support for npm '@org/name' packages +// Regex: .*?node_modules(?!.*node_modules)\/(@[^\/]*\/[^\/]*|[^\/]*).* +// Tests: https://regex101.com/r/lW2bE3/6 +const moduleRegex = new RegExp( + [ + '.*?node_modules(?!.*node_modules)\\', + '(@[^\\', + ']*\\', + '[^\\', + ']*|[^\\', + ']*).*', + ].join(path.sep) +); + +/** + * Retrieves a package name from the full import path. + * For example: + * './node_modules/bar/index/foo.js' => 'bar' + * + * @param path The full import path. + * Extracted from https://github.com/googleapis/cloud-trace-nodejs/blob/master/src/util.ts#L214 + */ +export function packageNameFromPath(importPath: string) { + const matches = moduleRegex.exec(importPath); + return matches && matches.length > 1 ? matches[1] : null; +} From 760918e3c0e60ccbd966896c6cec4462728ea718 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sun, 26 Jan 2020 11:39:52 +0100 Subject: [PATCH 4/6] chore: use find instead of some --- .../src/instrumentation/PluginLoader.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts index f1f3dee847..d991c42d24 100644 --- a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts +++ b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts @@ -84,10 +84,11 @@ export class PluginLoader { } const alreadyRequiredModules = Object.keys(require.cache); - const requiredModulesToHook = modulesToHook.filter(name => - alreadyRequiredModules.some( - cached => utils.packageNameFromPath(cached) === name - ) + const requiredModulesToHook = modulesToHook.filter( + name => + alreadyRequiredModules.find( + cached => utils.packageNameFromPath(cached) === name + ) !== undefined ); if (requiredModulesToHook.length > 0) { this.logger.warn( From 2934a791c762ff1bf8df13ca7906b18ec0930290 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sat, 1 Feb 2020 11:33:00 +0100 Subject: [PATCH 5/6] chore: use require.resolve to find already required modules --- .../src/instrumentation/PluginLoader.ts | 2 +- .../src/instrumentation/utils.ts | 27 ------------------- 2 files changed, 1 insertion(+), 28 deletions(-) diff --git a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts index d991c42d24..22b8ed1660 100644 --- a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts +++ b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts @@ -87,7 +87,7 @@ export class PluginLoader { const requiredModulesToHook = modulesToHook.filter( name => alreadyRequiredModules.find( - cached => utils.packageNameFromPath(cached) === name + cached => require.resolve(name) === cached ) !== undefined ); if (requiredModulesToHook.length > 0) { diff --git a/packages/opentelemetry-node/src/instrumentation/utils.ts b/packages/opentelemetry-node/src/instrumentation/utils.ts index 33a56391f4..062682e3fb 100644 --- a/packages/opentelemetry-node/src/instrumentation/utils.ts +++ b/packages/opentelemetry-node/src/instrumentation/utils.ts @@ -72,30 +72,3 @@ export function isSupportedVersion( export function searchPathForTest(searchPath: string) { module.paths.push(searchPath); } - -// Includes support for npm '@org/name' packages -// Regex: .*?node_modules(?!.*node_modules)\/(@[^\/]*\/[^\/]*|[^\/]*).* -// Tests: https://regex101.com/r/lW2bE3/6 -const moduleRegex = new RegExp( - [ - '.*?node_modules(?!.*node_modules)\\', - '(@[^\\', - ']*\\', - '[^\\', - ']*|[^\\', - ']*).*', - ].join(path.sep) -); - -/** - * Retrieves a package name from the full import path. - * For example: - * './node_modules/bar/index/foo.js' => 'bar' - * - * @param path The full import path. - * Extracted from https://github.com/googleapis/cloud-trace-nodejs/blob/master/src/util.ts#L214 - */ -export function packageNameFromPath(importPath: string) { - const matches = moduleRegex.exec(importPath); - return matches && matches.length > 1 ? matches[1] : null; -} From 29e07e659be31b2dbf5b54cfc2e07eab4e75fa43 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sat, 8 Feb 2020 14:03:57 +0100 Subject: [PATCH 6/6] chore: try/catch require.resolve --- .../src/instrumentation/PluginLoader.ts | 10 +++++++--- .../test/instrumentation/PluginLoader.test.ts | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts index 22b8ed1660..e5fb3a83a7 100644 --- a/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts +++ b/packages/opentelemetry-node/src/instrumentation/PluginLoader.ts @@ -86,9 +86,13 @@ export class PluginLoader { const alreadyRequiredModules = Object.keys(require.cache); const requiredModulesToHook = modulesToHook.filter( name => - alreadyRequiredModules.find( - cached => require.resolve(name) === cached - ) !== undefined + alreadyRequiredModules.find(cached => { + try { + return require.resolve(name) === cached; + } catch (err) { + return false; + } + }) !== undefined ); if (requiredModulesToHook.length > 0) { this.logger.warn( diff --git a/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts b/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts index 153010d5f7..c762a5c287 100644 --- a/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts +++ b/packages/opentelemetry-node/test/instrumentation/PluginLoader.test.ts @@ -239,7 +239,7 @@ describe('PluginLoader', () => { }, }; require('already-require-module'); - const pluginLoader = new PluginLoader(registry, verifyWarnLogger); + const pluginLoader = new PluginLoader(provider, verifyWarnLogger); pluginLoader.load(alreadyRequiredPlugins); pluginLoader.unload(); });