feat(baseplugin): add internalfilesloader (#240)

* feat(baseplugin): add internalfilesloader

* refactor: remove new ctor

* refactor: baseplugin internal file loader

* test: don't browser test internal files loading

* chore: add todo issue

* refactor: cast modules as unknown isntead of any

* chore: fix linting

packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts

@@ -14,15 +14,30 @@
  * limitations under the License.
  */
 
-import { Tracer, Plugin, Logger, PluginConfig } from '@opentelemetry/types';
+import {
+  Tracer,
+  Plugin,
+  Logger,
+  PluginConfig,
+  PluginInternalFiles,
+  PluginInternalFilesVersion,
+} from '@opentelemetry/types';
+import * as semver from 'semver';
+import * as path from 'path';
 
 /** This class represent the base to patch plugin. */
 export abstract class BasePlugin<T> implements Plugin<T> {
+  supportedVersions?: string[];
+  readonly moduleName?: string; // required for internalFilesExports
+  readonly version?: string; // required for internalFilesExports
+  protected readonly _basedir?: string; // required for internalFilesExports
+
   protected _moduleExports!: T;
   protected _tracer!: Tracer;
   protected _logger!: Logger;
+  protected _internalFilesExports!: { [module: string]: unknown }; // output for internalFilesExports
+  protected readonly _internalFilesList?: PluginInternalFiles; // required for internalFilesExports
   protected _config!: PluginConfig;
-  supportedVersions?: string[];
 
   enable(
     moduleExports: T,
@@ -33,6 +48,7 @@ export abstract class BasePlugin<T> implements Plugin<T> {
     this._moduleExports = moduleExports;
     this._tracer = tracer;
     this._logger = logger;
+    this._internalFilesExports = this._loadInternalFilesExports();
     if (config) this._config = config;
     return this.patch();
   }
@@ -41,6 +57,86 @@ export abstract class BasePlugin<T> implements Plugin<T> {
     this.unpatch();
   }
 
+  /**
+   * @TODO: To avoid circular dependencies, internal file loading functionality currently
+   * lives in BasePlugin. It is not meant to work in the browser and so this logic
+   * should eventually be moved somewhere else where it makes more sense.
+   * https://github.com/open-telemetry/opentelemetry-js/issues/285
+   */
+  private _loadInternalFilesExports(): PluginInternalFiles {
+    if (!this._internalFilesList) return {};
+    if (!this.version || !this.moduleName || !this._basedir) {
+      // log here because internalFilesList was provided, so internal file loading
+      // was expected to be working
+      this._logger.debug(
+        'loadInternalFiles failed because one of the required fields was missing: moduleName=%s, version=%s, basedir=%s',
+        this.moduleName,
+        this.version,
+        this._basedir
+      );
+      return {};
+    }
+    let extraModules: PluginInternalFiles = {};
+    this._logger.debug('loadInternalFiles %o', this._internalFilesList);
+    Object.keys(this._internalFilesList).forEach(versionRange => {
+      this._loadInternalModule(versionRange, extraModules);
+    });
+    if (Object.keys(extraModules).length === 0) {
+      this._logger.debug(
+        'No internal files could be loaded for %s@%s',
+        this.moduleName,
+        this.version
+      );
+    }
+    return extraModules;
+  }
+
+  private _loadInternalModule(
+    versionRange: string,
+    outExtraModules: PluginInternalFiles
+  ): void {
+    if (semver.satisfies(this.version!, versionRange)) {
+      if (Object.keys(outExtraModules).length > 0) {
+        this._logger.warn(
+          'Plugin for %s@%s, has overlap version range (%s) for internal files: %o',
+          this.moduleName,
+          this.version,
+          versionRange,
+          this._internalFilesList
+        );
+      }
+      this._requireInternalFiles(
+        this._internalFilesList![versionRange],
+        this._basedir!,
+        outExtraModules
+      );
+    }
+  }
+
+  private _requireInternalFiles(
+    extraModulesList: PluginInternalFilesVersion,
+    basedir: string,
+    outExtraModules: PluginInternalFiles
+  ): void {
+    if (!extraModulesList) return;
+    Object.keys(extraModulesList).forEach(moduleName => {
+      try {
+        this._logger.debug('loading File %s', extraModulesList[moduleName]);
+        outExtraModules[moduleName] = require(path.join(
+          basedir,
+          extraModulesList[moduleName]
+        ));
+      } catch (e) {
+        this._logger.error(
+          'Could not load internal file %s of module %s. Error: %s',
+          path.join(basedir, extraModulesList[moduleName]),
+          this.moduleName,
+          e.message
+        );
+      }
+    });
+  }
+
   protected abstract patch(): T;
   protected abstract unpatch(): void;
 }

packages/opentelemetry-core/test/trace/BasePlugin.test.ts

@@ -0,0 +1,79 @@
+/**
+ * Copyright 2019, 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.
+ */
+
+import { BasePlugin, NoopTracer, NoopLogger } from '../../src';
+import * as assert from 'assert';
+import * as path from 'path';
+import * as types from './fixtures/test-package/foo/bar/internal';
+
+const tracer = new NoopTracer();
+const logger = new NoopLogger();
+
+describe('BasePlugin', () => {
+  describe('internalFilesLoader', () => {
+    it('should load internally exported files', () => {
+      const testPackage = require('./fixtures/test-package');
+      const plugin = new TestPlugin();
+      assert.doesNotThrow(() => {
+        plugin.enable(testPackage, tracer, logger);
+      });
+
+      // @TODO: https://github.com/open-telemetry/opentelemetry-js/issues/285
+      if (typeof process !== 'undefined' && process.release.name === 'node') {
+        assert.ok(plugin['_internalFilesExports']);
+        assert.strictEqual(
+          (plugin['_internalFilesExports']
+            .internal as typeof types).internallyExportedFunction(),
+          true
+        );
+        assert.strictEqual(
+          plugin['_internalFilesExports'].expectUndefined,
+          undefined
+        );
+        assert.strictEqual(
+          (plugin['_moduleExports']![
+            'externallyExportedFunction'
+          ] as Function)(),
+          true
+        );
+      } else {
+        assert.ok(true, 'Internal file loading is not tested in the browser');
+      }
+    });
+  });
+});
+
+class TestPlugin extends BasePlugin<{ [key: string]: Function }> {
+  readonly moduleName = 'test-package';
+  readonly version = '0.0.1';
+  readonly _basedir = basedir;
+
+  protected readonly _internalFilesList = {
+    '0.0.1': {
+      internal: 'foo/bar/internal.js',
+    },
+    '^1.0.0': {
+      expectUndefined: 'foo/bar/internal.js',
+    },
+  };
+
+  protected patch(): { [key: string]: Function } {
+    return this._moduleExports;
+  }
+  protected unpatch(): void {}
+}
+
+const basedir = path.dirname(require.resolve('./fixtures/test-package'));

packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.d.ts

@@ -0,0 +1 @@
+export declare function internallyExportedFunction(): boolean;

packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.js

@@ -0,0 +1,4 @@
+Object.defineProperty(exports, "__esModule", { value: true });
+exports.internallyExportedFunction = function internallyExportedFunction() {
+  return true;
+}

packages/opentelemetry-core/test/trace/fixtures/test-package/index.js

@@ -0,0 +1,4 @@
+Object.defineProperty(exports, "__esModule", { value: true });
+exports.externallyExportedFunction = function externallyExportedFunction() {
+  return true;
+}

packages/opentelemetry-core/test/trace/fixtures/test-package/package.json

@@ -0,0 +1,11 @@
+{
+  "name": "test-package",
+  "version": "0.0.1",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"
+  },
+  "author": "",
+  "license": "ISC"
+}

packages/opentelemetry-plugin-grpc/package.json

@@ -59,7 +59,6 @@
     "@opentelemetry/node-sdk": "^0.0.1",
     "@opentelemetry/scope-async-hooks": "^0.0.1",
     "@opentelemetry/types": "^0.0.1",
-    "shimmer": "^1.2.1",
-    "semver": "^6.3.0"
+    "shimmer": "^1.2.1"
   }
 }

packages/opentelemetry-plugin-grpc/src/grpc.ts

@@ -27,7 +27,6 @@ import { AttributeNames } from './enums/AttributeNames';
 import {
   grpc,
   ModuleExportsMapping,
-  ModuleNameToFilePath,
   GrpcPluginOptions,
   ServerCallWithMeta,
   SendUnaryDataCallback,
@@ -42,7 +41,6 @@ import {
 import * as events from 'events';
 import * as grpcModule from 'grpc';
 import * as shimmer from 'shimmer';
-import * as semver from 'semver';
 import * as path from 'path';
 
 /** The metadata key under which span context is stored as a binary value. */
@@ -55,91 +53,20 @@ export class GrpcPlugin extends BasePlugin<grpc> {
 
   options!: GrpcPluginOptions;
 
-  constructor(public moduleName: string, public version: string) {
+  constructor(readonly moduleName: string, readonly version: string) {
     super();
     // TODO: remove this once options will be passed
     // see https://github.com/open-telemetry/opentelemetry-js/issues/210
     this.options = {};
   }
 
-  // TODO: Delete if moving internal file loaders to BasePlugin
-  // --- Note: Incorrectly ordered: Begin internal file loader --- //
-  // tslint:disable-next-line:no-any
-  protected _internalFilesExports: { [key: string]: any } | undefined;
-  protected readonly _internalFileList: ModuleExportsMapping = {
+  protected readonly _internalFilesList: ModuleExportsMapping = {
     '0.13 - 1.6': { client: 'src/node/src/client.js' },
     '^1.7': { client: 'src/client.js' },
   };
-
-  /**
-   * Load internal files according to version range
-   */
-  private _loadInternalFiles(): ModuleExportsMapping {
-    let result: ModuleExportsMapping = {};
-    if (this._internalFileList) {
-      this._logger.debug('loadInternalFiles %o', this._internalFileList);
-      Object.keys(this._internalFileList).forEach(versionRange => {
-        if (semver.satisfies(this.version, versionRange)) {
-          if (result) {
-            this._logger.warn(
-              'Plugin for %s@%s, has overlap version range (%s) for internal files: %o',
-              this.moduleName,
-              this.version,
-              versionRange,
-              this._internalFileList
-            );
-          }
-          result = this._loadInternalModuleFiles(
-            this._internalFileList[versionRange],
-            basedir
-          );
-        }
-      });
-      if (Object.keys(result).length === 0) {
-        this._logger.debug(
-          'No internal file could be loaded for %s@%s',
-          this.moduleName,
-          this.version
-        );
-      }
-    }
-    return result;
-  }
-
-  /**
-   * Load internal files from a module and set internalFilesExports
-   */
-  private _loadInternalModuleFiles(
-    extraModulesList: ModuleNameToFilePath,
-    basedir: string
-  ): ModuleExportsMapping {
-    const extraModules: ModuleExportsMapping = {};
-    if (extraModulesList) {
-      Object.keys(extraModulesList).forEach(moduleName => {
-        try {
-          this._logger.debug('loading File %s', extraModulesList[moduleName]);
-          extraModules[moduleName] = require(path.join(
-            basedir,
-            extraModulesList[moduleName]
-          ));
-        } catch (e) {
-          this._logger.error(
-            'Could not load internal file %s of module %s. Error: %s',
-            path.join(basedir, extraModulesList[moduleName]),
-            this.moduleName,
-            e.message
-          );
-        }
-      });
-    }
-    return extraModules;
-  }
-  // --- End of internal file loader stuff --- //
+  protected readonly _basedir = basedir;
 
   protected patch(): typeof grpcModule {
-    if (!this._internalFilesExports) {
-      this._internalFilesExports = this._loadInternalFiles();
-    }
     this._logger.debug(
       'applying patch to %s@%s',
       this.moduleName,
@@ -155,8 +82,8 @@ export class GrpcPlugin extends BasePlugin<grpc> {
       );
     }
 
-    if (this._internalFilesExports && this._internalFilesExports['client']) {
-      grpcClientModule = this._internalFilesExports['client'];
+    if (this._internalFilesExports['client']) {
+      grpcClientModule = this._internalFilesExports['client'] as object;
 
       shimmer.wrap(
         grpcClientModule,