fix(basic-tracer): use recording/real span (open-telemetry#145)

* fix(basic-tracer): use recording/real span * fix: avoid mutate the parent span, use Object.assign()
srikanthccv · Aug 1, 2019 · 669b088 · 669b088
1 parent e20de3b
commit 669b088
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 40 deletions.
diff --git a/packages/opentelemetry-basic-tracer/src/BasicTracer.ts b/packages/opentelemetry-basic-tracer/src/BasicTracer.ts
@@ -15,18 +15,16 @@
  */
 
 import * as types from '@opentelemetry/types';
-import { ScopeManager } from '@opentelemetry/scope-base';
 import {
+  ALWAYS_SAMPLER,
   BinaryTraceContext,
   HttpTraceContext,
-  NoopSpan,
-  randomSpanId,
-  randomTraceId,
   NOOP_SPAN,
-  ALWAYS_SAMPLER,
 } from '@opentelemetry/core';
-import { BasicTracerConfig } from '../src/types';
 import { BinaryFormat, HttpTextFormat } from '@opentelemetry/types';
+import { BasicTracerConfig } from '../src/types';
+import { ScopeManager } from '@opentelemetry/scope-base';
+import { Span } from './Span';
 
 /**
  * This class represents a basic tracer.
@@ -54,42 +52,21 @@ export class BasicTracer implements types.Tracer {
    * decision.
    */
   startSpan(name: string, options: types.SpanOptions = {}): types.Span {
-    let parentSpanContext: types.SpanContext | undefined;
-
-    // parent is a SpanContext
-    if (options.parent && (options.parent as types.SpanContext).traceId) {
-      parentSpanContext = options.parent as types.SpanContext;
-    }
-    // parent is a Span
-    if (
-      options.parent &&
-      typeof (options.parent as types.Span).context === 'function'
-    ) {
-      parentSpanContext = (options.parent as types.Span).context();
-    }
-
+    const parentSpanContext = this._getParentSpanContext(options.parent);
     // make sampling decision
     if (!this._sampler.shouldSample(parentSpanContext)) {
       // TODO: propagate SpanContext, for more information see
       // https://github.com/open-telemetry/opentelemetry-js/pull/99#issuecomment-513325536
       return NOOP_SPAN;
     }
 
-    // span context
-    const traceId = parentSpanContext
-      ? parentSpanContext.traceId
-      : randomTraceId();
-    const spanId = randomSpanId();
-
-    // TODO: create a real Span
-    const span = new NoopSpan({
-      traceId,
-      spanId,
+    const spanOptions = Object.assign({}, options, {
+      parent: parentSpanContext,
     });
+    const span = new Span(this, name, spanOptions);
 
     // Set default attributes
     span.setAttributes(this._defaultAttributes);
-
     return span;
   }
 
@@ -132,4 +109,20 @@ export class BasicTracer implements types.Tracer {
   getHttpTextFormat(): HttpTextFormat {
     return this._httpTextFormat;
   }
+
+  private _getParentSpanContext(
+    parent: types.Span | types.SpanContext | undefined
+  ): types.SpanContext | undefined {
+    if (!parent) return undefined;
+
+    // parent is a SpanContext
+    if ((parent as types.SpanContext).traceId) {
+      return parent as types.SpanContext;
+    }
+
+    if (typeof (parent as types.Span).context === 'function') {
+      return (parent as Span).context();
+    }
+    return undefined;
+  }
 }
diff --git a/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts b/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts
@@ -15,16 +15,17 @@
  */
 
 import * as assert from 'assert';
-import { BasicTracer } from '../src/BasicTracer';
 import {
+  ALWAYS_SAMPLER,
   BinaryTraceContext,
   HttpTraceContext,
-  ALWAYS_SAMPLER,
   NEVER_SAMPLER,
-  NoopLogger,
   NOOP_SPAN,
+  NoopLogger,
 } from '@opentelemetry/core';
+import { BasicTracer } from '../src/BasicTracer';
 import { NoopScopeManager } from '@opentelemetry/scope-base';
+import { Span } from '../src/Span';
 
 describe('BasicTracer', () => {
   describe('constructor', () => {
@@ -79,13 +80,14 @@ describe('BasicTracer', () => {
     });
   });
 
-  describe('#startSpan', () => {
+  describe('.startSpan()', () => {
     it('should start a span with name only', () => {
       const tracer = new BasicTracer({
         scopeManager: new NoopScopeManager(),
       });
       const span = tracer.startSpan('my-span');
       assert.ok(span);
+      assert.ok(span instanceof Span);
     });
 
     it('should start a span with name and options', () => {
@@ -94,6 +96,7 @@ describe('BasicTracer', () => {
       });
       const span = tracer.startSpan('my-span', {});
       assert.ok(span);
+      assert.ok(span instanceof Span);
     });
 
     it('should return a default span with no sampling', () => {
@@ -112,7 +115,7 @@ describe('BasicTracer', () => {
     it('should set default attributes on span');
   });
 
-  describe('#getCurrentSpan', () => {
+  describe('.getCurrentSpan()', () => {
     it('should return null with NoopScopeManager', () => {
       const tracer = new BasicTracer({
         scopeManager: new NoopScopeManager(),
@@ -122,7 +125,7 @@ describe('BasicTracer', () => {
     });
   });
 
-  describe('#withSpan', () => {
+  describe('.withSpan()', () => {
     it('should run scope with NoopScopeManager scope manager', done => {
       const tracer = new BasicTracer({
         scopeManager: new NoopScopeManager(),
@@ -134,12 +137,12 @@ describe('BasicTracer', () => {
     });
   });
 
-  describe('#recordSpanData', () => {
+  describe('.recordSpanData()', () => {
     // @todo: implement
     it('should call exporters with span data');
   });
 
-  describe('#getBinaryFormat', () => {
+  describe('.getBinaryFormat()', () => {
     it('should get default binary formatter', () => {
       const tracer = new BasicTracer({
         scopeManager: new NoopScopeManager(),
@@ -148,7 +151,7 @@ describe('BasicTracer', () => {
     });
   });
 
-  describe('#getHttpTextFormat', () => {
+  describe('.getHttpTextFormat()', () => {
     it('should get default HTTP text formatter', () => {
       const tracer = new BasicTracer({
         scopeManager: new NoopScopeManager(),