Code Enhancement/Refactor (#403)

* Code Enhancement/Refactor

* fix review comments

Change console instance to logger instance (ConsoleLogger).
Remove return from the logger statement.

Author: mayurkale22

packages/opencensus-core/src/trace/model/no-record/no-record-root-span.ts

@@ -87,32 +87,27 @@ export class NoRecordRootSpan extends NoRecordSpanBase implements
   }
 
   /**
-   * Starts a new child span in the noop root span.
-   * @param name Span name.
-   * @param kind Span kind.
-   * @param parentSpanId Span parent ID.
+   * Starts a new no record child span in the no record root span.
+   * @param nameOrOptions Span name string or SpanOptions object.
+   * @param kind Span kind if not using SpanOptions object.
    */
   startChildSpan(
-      nameOrOptions?: string|types.SpanOptions, kind?: types.SpanKind,
-      parentSpanId?: string): types.Span {
-    const newSpan = new NoRecordSpan(this);
-    let spanName;
-    let spanKind;
-    if (typeof nameOrOptions === 'object') {
-      spanName = nameOrOptions.name;
-      spanKind = nameOrOptions.kind;
-    } else {
-      spanName = nameOrOptions;
-      spanKind = kind;
-    }
+      nameOrOptions?: string|types.SpanOptions,
+      kind?: types.SpanKind): types.Span {
+    const noRecordChild = new NoRecordSpan(this);
 
+    const spanName =
+        typeof nameOrOptions === 'object' ? nameOrOptions.name : nameOrOptions;
+    const spanKind =
+        typeof nameOrOptions === 'object' ? nameOrOptions.kind : kind;
     if (spanName) {
-      newSpan.name = spanName;
+      noRecordChild.name = spanName;
     }
     if (spanKind) {
-      newSpan.kind = spanKind;
+      noRecordChild.kind = spanKind;
     }
-    newSpan.start();
-    return newSpan;
+
+    noRecordChild.start();
+    return noRecordChild;
   }
 }

packages/opencensus-core/src/trace/model/root-span.ts

@@ -112,13 +112,12 @@ export class RootSpan extends SpanBase implements types.RootSpan {
 
   /**
    * Starts a new child span in the root span.
-   * @param name Span name.
-   * @param kind Span kind.
-   * @param parentSpanId Span parent ID.
+   * @param nameOrOptions Span name string or SpanOptions object.
+   * @param kind Span kind if not using SpanOptions object.
    */
   startChildSpan(
-      nameOrOptions?: string|types.SpanOptions, kind?: types.SpanKind,
-      parentSpanId?: string): types.Span {
+      nameOrOptions?: string|types.SpanOptions,
+      kind?: types.SpanKind): types.Span {
     if (this.ended) {
       this.logger.debug(
           'calling %s.startSpan() on ended %s %o', this.className,
@@ -132,25 +131,22 @@ export class RootSpan extends SpanBase implements types.RootSpan {
       return null;
     }
     this.numberOfChildrenLocal++;
-    const newSpan = new Span(this);
-    let spanName;
-    let spanKind;
-    if (typeof nameOrOptions === 'object') {
-      spanName = nameOrOptions.name;
-      spanKind = nameOrOptions.kind;
-    } else {
-      spanName = nameOrOptions;
-      spanKind = kind;
-    }
 
+    const child = new Span(this);
+
+    const spanName =
+        typeof nameOrOptions === 'object' ? nameOrOptions.name : nameOrOptions;
+    const spanKind =
+        typeof nameOrOptions === 'object' ? nameOrOptions.kind : kind;
     if (spanName) {
-      newSpan.name = spanName;
+      child.name = spanName;
     }
     if (spanKind) {
-      newSpan.kind = spanKind;
+      child.kind = spanKind;
     }
-    newSpan.start();
-    this.spansLocal.push(newSpan);
-    return newSpan;
+
+    child.start();
+    this.spansLocal.push(child);
+    return child;
   }
 }

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

@@ -39,9 +39,6 @@ export class CoreTracer implements types.Tracer {
   private config: configTypes.TracerConfig;
   /** A list of end span event listeners */
   private eventListenersLocal: types.SpanEventListener[] = [];
-  /** A list of ended root spans */
-  // @ts-ignore
-  private endedTraces: types.RootSpan[] = [];
   /** Bit to represent whether trace is sampled or not. */
   private readonly IS_SAMPLED = 0x1;
   /** A sampler used to make sample decisions */
@@ -162,34 +159,31 @@ export class CoreTracer implements types.Tracer {
     });
   }
 
+  /** Notifies listeners of the span start. */
   onStartSpan(root: types.RootSpan): void {
-    if (this.active) {
-      if (!root) {
-        return this.logger.debug('cannot start trace - no active trace found');
-      }
-      if (this.currentRootSpan !== root) {
-        this.logger.debug(
-            'currentRootSpan != root on notifyStart. Need more investigation.');
-      }
-      this.notifyStartSpan(root);
+    if (!this.active) return;
+    if (!root) {
+      return this.logger.debug('cannot start trace - no active trace found');
+    }
+    if (this.currentRootSpan !== root) {
+      this.logger.debug(
+          'currentRootSpan != root on notifyStart. Need more investigation.');
     }
+    this.notifyStartSpan(root);
   }
 
-  /**
-   * Is called when a span is ended.
-   * @param root The ended span.
-   */
+  /** Notifies listeners of the span end. */
   onEndSpan(root: types.RootSpan): void {
-    if (this.active) {
-      if (!root) {
-        return this.logger.debug('cannot end trace - no active trace found');
-      }
-      if (this.currentRootSpan !== root) {
-        this.logger.debug(
-            'currentRootSpan != root on notifyEnd. Need more investigation.');
-      }
-      this.notifyEndSpan(root);
+    if (!this.active) return;
+    if (!root) {
+      this.logger.debug('cannot end trace - no active trace found');
+      return;
+    }
+    if (this.currentRootSpan !== root) {
+      this.logger.debug(
+          'currentRootSpan != root on notifyEnd. Need more investigation.');
     }
+    this.notifyEndSpan(root);
   }
 
   /**
@@ -221,15 +215,11 @@ export class CoreTracer implements types.Tracer {
   }
 
   private notifyEndSpan(root: types.RootSpan) {
-    if (this.active) {
-      this.logger.debug('starting to notify listeners the end of rootspans');
-      if (this.eventListenersLocal && this.eventListenersLocal.length > 0) {
-        for (const listener of this.eventListenersLocal) {
-          listener.onEndSpan(root);
-        }
+    this.logger.debug('starting to notify listeners the end of rootspans');
+    if (this.eventListenersLocal && this.eventListenersLocal.length > 0) {
+      for (const listener of this.eventListenersLocal) {
+        listener.onEndSpan(root);
       }
-    } else {
-      this.logger.debug('this tracer is inactivate cant notify endspan');
     }
   }
 
@@ -240,9 +230,8 @@ export class CoreTracer implements types.Tracer {
 
   /**
    * Starts a span.
-   * @param name The span name.
-   * @param kind optional The span kind.
-   * @param parentSpanId The parent span ID.
+   * @param nameOrOptions Span name string or SpanOptions object.
+   * @param kind Span kind if not using SpanOptions object.
    */
   startChildSpan(
       nameOrOptions?: string|types.SpanOptions,
@@ -277,7 +266,7 @@ export class CoreTracer implements types.Tracer {
    * This is necessary in order to create child spans correctly in event
    * handlers.
    * @param emitter An event emitter whose handlers should have
-   * the trace context binded to them.
+   *     the trace context binded to them.
    */
   wrapEmitter(emitter: NodeJS.EventEmitter): void {
     if (!this.active) {

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

@@ -464,7 +464,7 @@ export interface RootSpan extends Span {
   readonly numberOfChildren: number;
 
   /** Starts a new Span instance in the RootSpan instance */
-  startChildSpan(name?: string, kind?: SpanKind, parentSpanId?: string): Span;
+  startChildSpan(name?: string, kind?: SpanKind): Span;
   startChildSpan(options?: SpanOptions): Span;
   startChildSpan(nameOrOptions?: string|SpanOptions, kind?: SpanKind): Span;
 }
@@ -529,12 +529,11 @@ export interface Tracer extends SpanEventListener {
   /**
    * Start a new Span instance to the currentRootSpan
    * @param name Span name
-   * @param type Span type
-   * @param parentSpanId Parent SpanId
+   * @param kind Span kind
    * @param options Span Options
    * @returns The new Span instance started
    */
-  startChildSpan(name?: string, type?: SpanKind, parentSpanId?: string): Span;
+  startChildSpan(name?: string, kind?: SpanKind): Span;
   startChildSpan(options?: SpanOptions): Span;
 
   /**

packages/opencensus-core/test/test-console-exporter.ts

@@ -91,7 +91,7 @@ describe('ConsoleLogExporter', () => {
       const exporter = new ConsoleExporter(defaultBufferConfig);
       const rootSpan = new RootSpan(tracer, name, kind, traceId, parentSpanId);
       rootSpan.start();
-      rootSpan.startChildSpan('name', SpanKind.UNSPECIFIED, rootSpan.traceId);
+      rootSpan.startChildSpan('name', SpanKind.UNSPECIFIED);
       const queue: RootSpan[] = [rootSpan];
 
       return exporter.publish(queue).then(() => {

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

@@ -343,34 +343,31 @@ describe('Tracer', () => {
     });
 
     it('should start a span with SpanObject', () => {
-      let spanWithObject: types.Span;
       tracer.startRootSpan(options, (rootSpan) => {
-        spanWithObject = tracer.startChildSpan(
+        const spanWithObject = tracer.startChildSpan(
             {name: 'my-span', kind: types.SpanKind.SERVER});
+        assert.ok(spanWithObject.started);
+        assert.strictEqual(spanWithObject.name, 'my-span');
+        assert.strictEqual(spanWithObject.kind, types.SpanKind.SERVER);
       });
-      assert.ok(spanWithObject.started);
-      assert.strictEqual(spanWithObject.name, 'my-span');
-      assert.strictEqual(spanWithObject.kind, types.SpanKind.SERVER);
     });
 
     it('should start a span with SpanObject-name', () => {
-      let spanWithObject: types.Span;
       tracer.startRootSpan(options, (rootSpan) => {
-        spanWithObject = tracer.startChildSpan({name: 'my-span1'});
+        const spanWithObject = tracer.startChildSpan({name: 'my-span1'});
+        assert.ok(spanWithObject.started);
+        assert.strictEqual(spanWithObject.name, 'my-span1');
+        assert.strictEqual(spanWithObject.kind, types.SpanKind.UNSPECIFIED);
       });
-      assert.ok(spanWithObject.started);
-      assert.strictEqual(spanWithObject.name, 'my-span1');
-      assert.strictEqual(spanWithObject.kind, types.SpanKind.UNSPECIFIED);
     });
 
     it('should start a span without params', () => {
-      let spanWithObject: types.Span;
       tracer.startRootSpan(options, (rootSpan) => {
-        spanWithObject = tracer.startChildSpan();
+        const spanWithObject = tracer.startChildSpan();
+        assert.ok(spanWithObject.started);
+        assert.strictEqual(spanWithObject.name, null);
+        assert.strictEqual(spanWithObject.kind, types.SpanKind.UNSPECIFIED);
       });
-      assert.ok(spanWithObject.started);
-      assert.strictEqual(spanWithObject.name, null);
-      assert.strictEqual(spanWithObject.kind, types.SpanKind.UNSPECIFIED);
     });
   });
 

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

@@ -14,11 +14,10 @@
  * limitations under the License.
  */
 
-import {Logger, NamedPluginConfig, Plugin, PluginConfig, PluginNames, Tracer} from '@opencensus/core';
+import {Logger, Plugin, PluginConfig, PluginNames, Tracer} from '@opencensus/core';
 import * as fs from 'fs';
 import * as path from 'path';
 import * as hook from 'require-in-the-middle';
-
 import {Constants} from '../constants';
 
 enum HookState {

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

@@ -14,14 +14,13 @@
  * limitations under the License.
  */
 import * as core from '@opencensus/core';
-import {Logger, logger} from '@opencensus/core';
-
+import {logger} from '@opencensus/core';
 import * as extend from 'extend';
-
 import {defaultConfig} from './config/default-config';
 import {Constants} from './constants';
 import {PluginLoader} from './instrumentation/plugin-loader';
 
+const NOOP_EXPORTER = new core.NoopExporter();
 
 /** Implements a Tracing. */
 export class Tracing implements core.Tracing {
@@ -33,12 +32,12 @@ export class Tracing implements core.Tracing {
   private defaultPlugins: core.PluginNames;
   /** A configuration object to start the tracing */
   private configLocal: core.Config = {};
-  /** An object to log information to */
-  private logger: core.Logger = null;
+  /** An object to log information to. Logs to the JS console by default. */
+  private logger: core.Logger = logger.logger();
   /** Singleton instance */
   private static singletonInstance: core.Tracing;
   /** Indicates if the tracing is active */
-  private activeLocal: boolean;
+  private activeLocal = false;
 
   /** Constructs a new TracingImpl instance. */
   constructor() {
@@ -95,15 +94,15 @@ export class Tracing implements core.Tracing {
     this.tracer.stop();
     this.pluginLoader.unloadPlugins();
     this.configLocal = {};
-    this.logger = null;
+    this.logger = logger.logger();
   }
 
 
   /** Gets the exporter. */
   get exporter(): core.Exporter {
     return this.configLocal.exporter ?
         this.configLocal.exporter as core.Exporter :
-        null;
+        NOOP_EXPORTER;
   }
 
   /**
@@ -128,7 +127,7 @@ export class Tracing implements core.Tracing {
    */
   unregisterExporter(exporter: core.Exporter): core.Tracing {
     this.tracer.unregisterSpanEventListener(exporter);
-    this.configLocal.exporter = null;
+    this.configLocal.exporter = NOOP_EXPORTER;
     return this;
   }
 }

packages/opencensus-nodejs/test/test-plugin-loader.ts

@@ -16,12 +16,9 @@
 
 import {CoreTracer, logger} from '@opencensus/core';
 import * as assert from 'assert';
-import * as path from 'path';
-
 import {Constants} from '../src/trace/constants';
 import {PluginLoader} from '../src/trace/instrumentation/plugin-loader';
 
-
 const INSTALLED_PLUGINS_PATH = `${__dirname}/instrumentation/node_modules`;
 const TEST_MODULES = [
   'simple-module'  // this module exist and has a plugin
@@ -130,7 +127,6 @@ describe('Plugin Loader', () => {
         const pluginLoader = new PluginLoader(log, tracer);
         assert.strictEqual(pluginLoader.plugins.length, 0);
         pluginLoader.loadPlugins(plugins);
-        const http = require(TEST_MODULES[2]);
         intercept((txt: string) => {
           assert.ok(txt.indexOf('error') >= 0);
         })();