fix: add recommendations

from markwolff
from mayurkale22

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

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;
   }
 

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);

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 */
     });
   });
 });

packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts

@@ -22,9 +22,9 @@ import { Logger } from '../../common/Logger';
 export interface Plugin<T = any> {
   /**
    * 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[];