refactor(fix): changes to address review comments (#50)

Author: eduardoemery

packages/opencensus-core/src/trace/model/tracer.ts

@@ -88,7 +88,7 @@ export class Tracer implements types.Tracer {
     return this;
   }
 
-  /** Gets the list of event listners. */
+  /** Gets the list of event listeners. */
   get eventListeners(): types.SpanEventListener[] {
     return this.eventListenersLocal;
   }
@@ -171,8 +171,19 @@ export class Tracer implements types.Tracer {
    * Registers an end span event listener.
    * @param listener The listener to register.
    */
-  registerSpanEventListener(listner: types.SpanEventListener) {
-    this.eventListenersLocal.push(listner);
+  registerSpanEventListener(listener: types.SpanEventListener) {
+    this.eventListenersLocal.push(listener);
+  }
+
+  /**
+   * Unregisters an end span event listener.
+   * @param listener The listener to unregister.
+   */
+  unregisterSpanEventListener(listener: types.SpanEventListener) {
+    const index = this.eventListenersLocal.indexOf(listener, 0);
+    if (index > -1) {
+      this.eventListeners.splice(index, 1);
+    }
   }
 
   private notifyStartSpan(root: types.RootSpan) {

packages/opencensus-core/src/trace/model/types.ts

@@ -257,6 +257,12 @@ export interface Tracer extends SpanEventListener {
    */
   registerSpanEventListener(listener: SpanEventListener): void;
 
+  /**
+   * Unregisters an end span event listener.
+   * @param listener The listener to unregister.
+   */
+  unregisterSpanEventListener(listener: SpanEventListener): void;
+
   /** Clear the currentRootSpan from tracer instance */
   clearCurrentTrace(): void;
 

packages/opencensus-core/src/trace/types.ts

@@ -28,10 +28,13 @@ export interface Tracing {
   /** Service to send collected traces to. */
   readonly exporter: exportersTypes.Exporter;
 
+  /** Gets active status  */
+  active: boolean;
+
   /**
-   * Enables the tracing process.
-   * @param userConfig A configuration object.
-   * @returns The tracing object.
+   * Starts tracing.
+   * @param userConfig A configuration object to start tracing.
+   * @returns The started Tracing instance.
    */
   start(userConfig?: configTypes.Config): Tracing;
 
@@ -44,4 +47,10 @@ export interface Tracing {
    * @returns The tracing object.
    */
   registerExporter(exporter: exportersTypes.Exporter): Tracing;
+
+  /**
+   * Unregisters an exporter.
+   * @param exporter The exporter to stop sending traces to.
+   */
+  unregisterExporter(exporter: exportersTypes.Exporter): Tracing;
 }

packages/opencensus-core/test/test-tracer.ts

@@ -89,19 +89,37 @@ describe('Tracer', () => {
       tracer.registerSpanEventListener(onEndSpan);
     });
 
-    it('should register a new OnEndSpanEventListener on listners list', () => {
-      const listner = tracer.eventListeners[0];
+    it('should register a new OnEndSpanEventListener on listeners list', () => {
+      const listener = tracer.eventListeners[0];
       assert.strictEqual(tracer.eventListeners.length, 1);
-      assert.strictEqual(listner, onEndSpan);
+      assert.strictEqual(listener, onEndSpan);
     });
 
     it('should return an OnEndSpanEventListener list', () => {
-      for (const listner of tracer.eventListeners) {
-        assert.ok(listner instanceof OnEndSpanClass);
+      for (const listener of tracer.eventListeners) {
+        assert.ok(listener instanceof OnEndSpanClass);
       }
     });
   });
 
+  /** Should unregister a OnEndSpanEventlistener */
+  describe('unregisterSpanEventListener()', () => {
+    let tracer: types.Tracer, onEndSpan: OnEndSpanClass;
+    before(() => {
+      tracer = new Tracer();
+      onEndSpan = new OnEndSpanClass();
+      tracer.registerSpanEventListener(onEndSpan);
+    });
+
+    it('should register a new OnEndSpanEventListener on listeners list', () => {
+      const listener = tracer.eventListeners[0];
+      assert.strictEqual(tracer.eventListeners.length, 1);
+      assert.strictEqual(listener, onEndSpan);
+      tracer.unregisterSpanEventListener(onEndSpan);
+      assert.strictEqual(tracer.eventListeners.length, 0);
+    });
+  });
+
   /** Should stop the trace instance */
   describe('stop()', () => {
     it('should stop the trace instance', () => {

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

@@ -50,7 +50,6 @@ export class Tracing implements types.Tracing {
     return this.singletonInstance || (this.singletonInstance = new this());
   }
 
-  // TODO: tracing interface should be updated
   /** Gets active status  */
   get active(): boolean {
     return this.activeLocal;
@@ -62,9 +61,9 @@ export class Tracing implements types.Tracing {
   }
 
   /**
-   * Starts the tracing.
-   * @param userConfig A configuration object to start the tracing.
-   * @returns The started tracing.
+   * Starts tracing.
+   * @param userConfig A configuration object to start tracing.
+   * @returns The started Tracing instance.
    */
   start(userConfig?: types.Config): types.Tracing {
     this.configLocal = extend(
@@ -96,7 +95,6 @@ export class Tracing implements types.Tracing {
     this.pluginLoader.unloadPlugins();
     this.configLocal = null;
     this.logger = null;
-    // TODO: maybe some exporter logic when stop tracing
   }
 
 
@@ -108,37 +106,30 @@ export class Tracing implements types.Tracing {
 
   /**
    * Registers an exporter to send the collected traces to.
-   * @param exporter THe exporter to send the traces to.
+   * @param exporter The exporter to send the traces to.
    */
   registerExporter(exporter: types.Exporter): types.Tracing {
     if (exporter) {
       if (this.configLocal.exporter) {
-        this.unRegisterExporter(this.configLocal.exporter);
+        this.unregisterExporter(this.configLocal.exporter);
       }
       this.configLocal.exporter = exporter;
       this.tracer.registerSpanEventListener(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);
+        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.
+   * Unregisters an exporter.
+   * @param exporter The exporter to stop sending 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);
-    }
+  unregisterExporter(exporter: types.Exporter): types.Tracing {
+    this.tracer.unregisterSpanEventListener(exporter);
     this.configLocal.exporter = null;
     return this;
   }

packages/opencensus-nodejs/test/test-tracing.ts

@@ -242,4 +242,19 @@ describe('Tracing', () => {
       assert.strictEqual(tracing.tracer.eventListeners.length, 0);
     });
   });
+
+  /** Should unregister the exporter instance */
+  describe('unregisterExporter()', () => {
+    it('should unregister the exporter on tracer', () => {
+      const tracing = new Tracing();
+      tracing.start();
+      const exporter = NOOP_EXPORTER;
+      tracing.registerExporter(exporter);
+      assert.strictEqual(tracing.config.exporter, exporter);
+      assert.strictEqual(tracing.tracer.eventListeners.length, 1);
+      tracing.unregisterExporter(exporter);
+      assert.strictEqual(tracing.config.exporter, null);
+      assert.strictEqual(tracing.tracer.eventListeners.length, 0);
+    });
+  });
 });