refactor(fix): changes to address review comments

Author: silva-fabio

packages/opencensus-nodejs/AUTHORS

@@ -5,4 +5,4 @@
 # of contributors, see the revision history in source control.
 
 Google LLC
-CESAR Team (www.cesar.org.br)
+CESAR Team (www.cesar.org.br)

packages/opencensus-nodejs/README.md

@@ -1,4 +1,4 @@
-# OpenCensus Core Node.js
+# OpenCensus Main Package
 [![Gitter chat][gitter-image]][gitter-url]
 
 OpenCensus for Node.js is an implementation of OpenCensus, a toolkit for collecting application performance and behavior monitoring data. 

packages/opencensus-nodejs/package-lock.json

@@ -6,16 +6,15 @@
   "types": "build/src/index.d.ts",
   "repository": "census-instrumentation/opencensus-node",
   "scripts": {
-    "build": "npm run compile",
     "test": "nyc -x '**/test/**' --reporter=html --reporter=text mocha 'build/test/**/*.js'",
     "clean": "rimraf build/*",
     "check": "gts check",
-    "compile": "tsc -p .; npm run cp-test-support-files",
+    "compile": "tsc -p .",
     "fix": "gts fix",
     "prepare": "npm run compile",
-    "pretest": "npm run compile",
+    "pretest": "npm run compile; npm run copy-fixtures",
     "posttest": "npm run check",
-    "cp-test-support-files": "find ./test -name 'package.json' -type f -exec rsync -R {} ./build \\;"
+    "copy-fixtures": "./scripts/copy-fixtures.sh"
   },
   "keywords": [
     "opencensus",
@@ -45,6 +44,7 @@
     "@types/node": "^9.4.7",
     "@types/shimmer": "^1.0.1",
     "gts": "^0.5.4",
+    "intercept-stdout": "^0.1.2",
     "mocha": "^5.0.4",
     "ncp": "^2.0.0",
     "nyc": "11.6.0",

packages/opencensus-nodejs/package.json

@@ -0,0 +1,4 @@
+#!/bin/bash
+
+find ./test -type d -exec mkdir -p ./build/{} \;
+find ./test -name 'package.json' -type f -exec cp {} ./build/{} \;

packages/opencensus-nodejs/scripts/copy-fixtures.sh

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-/** General pupose constants. */
+/** General purpose constants. */
 const constants = {
   /** Default maximum size of a buffer. */
   DEFAULT_BUFFER_SIZE: 100,

packages/opencensus-nodejs/src/trace/constants.ts

@@ -26,4 +26,3 @@ declare module 'require-in-the-middle' {
     function hook(onRequire: hook.OnRequireFn): void;
     export = hook;
   }
-  

packages/opencensus-nodejs/src/trace/instrumentation/ext-types.d.ts

@@ -56,7 +56,7 @@ export class PluginLoader {
 
 
   /**
-   * Returns a PlunginNames object, build from a string array of target modules
+   * Returns a PluginNames object, build from a string array of target modules
    * names, using the defaultPackageName.
    * @param modulesToPatch A list of modules to patch.
    * @returns Plugin names.
@@ -98,26 +98,33 @@ export class PluginLoader {
    * Loads a list of plugins (using a map of the target module name
    * and its instrumentation plugin package name). Each plugin module
    * should implement the core Plugin interface and export an instance
-   * named as "plugin".
+   * named as "plugin". This function will attach a hook to be called
+   * the first time the module is loaded.
    * @param pluginList A list of plugins.
    */
   loadPlugins(pluginList: types.PluginNames) {
-    const self = this;
-
     // tslint:disable:no-any
     hook(Object.keys(pluginList), (exports, name, basedir) => {
-      const version = self.getPackageVersion(name, basedir as string);
-      self.logger.info('trying loading %s.%s', name, version);
+      const version = this.getPackageVersion(name, basedir as string);
+      this.logger.info('trying loading %s.%s', name, version);
+      let result = exports;
       if (!version) {
-        return exports;
+        return result;
       } else {
-        self.logger.debug('applying patch to %s@%s module', name, version);
-        self.logger.debug(
+        this.logger.debug('applying patch to %s@%s module', name, version);
+        this.logger.debug(
             'using package %s to patch %s', pluginList[name], name);
         // Expecting a plugin from module;
-        const plugin: types.Plugin = require(pluginList[name]).plugin;
-        self.plugins.push(plugin);
-        return plugin.applyPatch(exports, self.tracer, version);
+        try {
+          const plugin: types.Plugin = require(pluginList[name]).plugin;
+          this.plugins.push(plugin);
+          result = plugin.applyPatch(exports, this.tracer, version);
+        } catch (e) {
+          this.logger.error(
+              'could not load plugin %s of module %s. Error: %s',
+              pluginList[name], name, e.message);
+        }
+        return result;
       }
     });
   }

packages/opencensus-nodejs/src/trace/instrumentation/plugin-loader.ts

@@ -24,7 +24,7 @@ import {PluginLoader} from './instrumentation/plugin-loader';
 /** Implements a Tracing. */
 export class Tracing implements types.Tracing {
   /** A tracer object */
-  private tracerLocal: types.Tracer;
+  readonly tracer: types.Tracer;
   /** A plugin loader object */
   private pluginLoader: PluginLoader;
   /** Plugin names */
@@ -34,20 +34,20 @@ export class Tracing implements types.Tracing {
   /** An object to log information to */
   private logger: types.Logger = null;
   /** Singleton instance */
-  private static sgltnInstance: types.Tracing;
+  private static singletonInstance: types.Tracing;
   /** Indicates if the tracing is active */
   private activeLocal: boolean;
 
   /** Constructs a new TracingImpl instance. */
   constructor() {
-    this.tracerLocal = new classes.Tracer();
+    this.tracer = new classes.Tracer();
     this.defaultPlugins = PluginLoader.defaultPluginsFromArray(
         Constants.DEFAULT_INSTRUMENTATION_MODULES);
   }
 
   /** Gets the trancing instance. */
   static get instance(): types.Tracing {
-    return this.sgltnInstance || (this.sgltnInstance = new this());
+    return this.singletonInstance || (this.singletonInstance = new this());
   }
 
   // TODO: tracing interface should be updated
@@ -74,7 +74,7 @@ export class Tracing implements types.Tracing {
         this.configLocal.logger || logger.logger(this.configLocal.logLevel);
     this.configLocal.logger = this.logger;
     this.logger.debug('config: %o', this.configLocal);
-    this.pluginLoader = new PluginLoader(this.logger, this.tracerLocal);
+    this.pluginLoader = new PluginLoader(this.logger, this.tracer);
     this.pluginLoader.loadPlugins(
         this.configLocal.plugins as types.PluginNames);
 
@@ -85,24 +85,20 @@ export class Tracing implements types.Tracing {
       this.registerExporter(this.configLocal.exporter);
     }
     this.activeLocal = true;
-    this.tracerLocal.start(this.configLocal);
+    this.tracer.start(this.configLocal);
     return this;
   }
 
   /** Stops the tracing. */
   stop() {
     this.activeLocal = false;
-    this.tracerLocal.stop();
+    this.tracer.stop();
     this.pluginLoader.unloadPlugins();
     this.configLocal = null;
     this.logger = null;
     // TODO: maybe some exporter logic when stop tracing
   }
 
-  /** Gets the tracer. */
-  get tracer(): types.Tracer {
-    return this.tracerLocal;
-  }
 
   /** Gets the exporter. */
   get exporter(): types.Exporter {
@@ -115,14 +111,35 @@ export class Tracing implements types.Tracing {
    * @param exporter THe exporter to send the traces to.
    */
   registerExporter(exporter: types.Exporter): types.Tracing {
-    this.configLocal.exporter = exporter;
-
-    // TODO: review this logic. Tracer.registerEndSpanListener
-    // should allow only one exporter listener
-    // an unregister method on Tracer is needed or a clear EndSpanListener
-    if (this.tracer.eventListeners.indexOf(exporter) < 0) {
+    if (exporter) {
+      if (this.configLocal.exporter) {
+        this.unRegisterExporter(this.configLocal.exporter);
+      }
+      this.configLocal.exporter = exporter;
       this.tracer.registerEndSpanListener(exporter);
+    } else {
+      // TODO: if unRegisterExporter go public, this logic may not be
+      // necessary - register a null to unRegister
+      if (this.configLocal.exporter) {
+        this.unRegisterExporter(this.configLocal.exporter);
+      }
+    }
+    return this;
+  }
+
+
+  /**
+   * Registers an exporter to send the collected traces to.
+   * @param exporter THe exporter to send the traces to.
+   */
+  // TODO: maybe this method should be added to Tracing interface
+  private unRegisterExporter(exporter: types.Exporter): types.Tracing {
+    // TODO: maybe an unRegisterEndSpanListener method should be added to Tracer
+    const index = this.tracer.eventListeners.indexOf(exporter, 0);
+    if (index > -1) {
+      this.tracer.eventListeners.splice(index, 1);
     }
+    this.configLocal.exporter = null;
     return this;
   }
 }