From 83480b76c66ca8f655c1f7ec90a923175b9d7e9f Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Wed, 7 Aug 2019 08:52:24 -0700 Subject: [PATCH] refactor: move traceId generation in Tracer instead of Span (#154) * refactor: move traceId generation in Tracer instead of Span * fix: pass SpanContext to span instead of destructuring * fix: pass actual spanContext to Span --- .../src/BasicTracer.ts | 44 ++++++++++++--- .../opentelemetry-basic-tracer/src/Span.ts | 45 ++++------------ .../test/BasicTracer.test.ts | 54 +++++++++++++++++++ .../test/Span.test.ts | 46 +++++++++------- packages/opentelemetry-core/src/index.ts | 1 + 5 files changed, 127 insertions(+), 63 deletions(-) diff --git a/packages/opentelemetry-basic-tracer/src/BasicTracer.ts b/packages/opentelemetry-basic-tracer/src/BasicTracer.ts index 811f183904..bf8d0d1a28 100644 --- a/packages/opentelemetry-basic-tracer/src/BasicTracer.ts +++ b/packages/opentelemetry-basic-tracer/src/BasicTracer.ts @@ -20,8 +20,15 @@ import { BinaryTraceContext, HttpTraceContext, NOOP_SPAN, + randomTraceId, + isValid, + randomSpanId, } from '@opentelemetry/core'; -import { BinaryFormat, HttpTextFormat } from '@opentelemetry/types'; +import { + BinaryFormat, + HttpTextFormat, + TraceOptions, +} from '@opentelemetry/types'; import { BasicTracerConfig } from '../src/types'; import { ScopeManager } from '@opentelemetry/scope-base'; import { Span } from './Span'; @@ -52,19 +59,39 @@ export class BasicTracer implements types.Tracer { * decision. */ startSpan(name: string, options: types.SpanOptions = {}): types.Span { - const parentSpanContext = this._getParentSpanContext(options.parent); + const parentContext = this._getParentSpanContext(options.parent); // make sampling decision - if (!this._sampler.shouldSample(parentSpanContext)) { + const samplingDecision = this._sampler.shouldSample(parentContext); + const spanId = randomSpanId(); + let traceId; + let traceState; + if (!parentContext || !isValid(parentContext)) { + // New root span. + traceId = randomTraceId(); + } else { + // New child span. + traceId = parentContext.traceId; + traceState = parentContext.traceState; + } + const traceOptions = samplingDecision + ? TraceOptions.SAMPLED + : TraceOptions.UNSAMPLED; + const spanContext = { traceId, spanId, traceOptions, traceState }; + + if (!samplingDecision) { // TODO: propagate SpanContext, for more information see // https://github.com/open-telemetry/opentelemetry-js/pull/99#issuecomment-513325536 return NOOP_SPAN; } - const spanOptions = Object.assign({}, options, { - parent: parentSpanContext, - }); - const span = new Span(this, name, spanOptions); - + const span = new Span( + this, + name, + spanContext, + options.kind || types.SpanKind.INTERNAL, + parentContext ? parentContext.spanId : undefined, + options.startTime + ); // Set default attributes span.setAttributes(this._defaultAttributes); return span; @@ -92,6 +119,7 @@ export class BasicTracer implements types.Tracer { /** * Records a SpanData. */ + /* c8 ignore next 3 */ recordSpanData(span: types.Span): void { // TODO: notify exporter } diff --git a/packages/opentelemetry-basic-tracer/src/Span.ts b/packages/opentelemetry-basic-tracer/src/Span.ts index a421004853..10420931a1 100644 --- a/packages/opentelemetry-basic-tracer/src/Span.ts +++ b/packages/opentelemetry-basic-tracer/src/Span.ts @@ -15,20 +15,14 @@ */ import * as types from '@opentelemetry/types'; -import { - randomSpanId, - randomTraceId, - INVALID_SPAN_CONTEXT, - isValid, -} from '@opentelemetry/core'; import { performance } from 'perf_hooks'; -import { TraceOptions } from '@opentelemetry/types'; +import { SpanKind, SpanContext } from '@opentelemetry/types'; /** * This class represents a span. */ export class Span implements types.Span { - private readonly _spanContext: types.SpanContext = INVALID_SPAN_CONTEXT; + private readonly _spanContext: types.SpanContext; private readonly _tracer: types.Tracer; private readonly _parentId?: string; private readonly _kind: types.SpanKind; @@ -47,24 +41,17 @@ export class Span implements types.Span { constructor( parentTracer: types.Tracer, spanName: string, - options: types.SpanOptions + spanContext: SpanContext, + kind: SpanKind, + parentSpanId?: string, + startTime?: number ) { this._tracer = parentTracer; this._name = spanName; - this._spanContext.spanId = randomSpanId(); - this._spanContext.traceOptions = TraceOptions.SAMPLED; - const parentSpanContext = this._getParentSpanContext(options.parent); - if (parentSpanContext && isValid(parentSpanContext)) { - // New child span. - this._spanContext.traceId = parentSpanContext.traceId; - this._spanContext.traceState = parentSpanContext.traceState; - this._parentId = parentSpanContext.spanId; - } else { - // This is a root span so no remote or local parent. - this._spanContext.traceId = randomTraceId(); - } - this._kind = options.kind || types.SpanKind.INTERNAL; - this._startTime = options.startTime || performance.now(); + this._spanContext = spanContext; + this._parentId = parentSpanId; + this._kind = kind; + this._startTime = startTime || performance.now(); } tracer(): types.Tracer { @@ -137,18 +124,6 @@ export class Span implements types.Span { return `Span${json}`; } - 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; - } - return (parent as Span).context(); - } - private _isSpanEnded(): boolean { if (this._ended) { // @todo: log a warning diff --git a/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts b/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts index 6b15e2dd0c..5643713b5b 100644 --- a/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts +++ b/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts @@ -22,10 +22,12 @@ import { NEVER_SAMPLER, NOOP_SPAN, NoopLogger, + TraceState, } from '@opentelemetry/core'; import { BasicTracer } from '../src/BasicTracer'; import { NoopScopeManager } from '@opentelemetry/scope-base'; import { Span } from '../src/Span'; +import { TraceOptions } from '@opentelemetry/types'; describe('BasicTracer', () => { describe('constructor', () => { @@ -97,6 +99,58 @@ describe('BasicTracer', () => { const span = tracer.startSpan('my-span', {}); assert.ok(span); assert.ok(span instanceof Span); + const context = span.context(); + assert.ok(context.traceId.match(/[a-f0-9]{32}/)); + assert.ok(context.spanId.match(/[a-f0-9]{16}/)); + assert.strictEqual(context.traceOptions, TraceOptions.SAMPLED); + assert.deepStrictEqual(context.traceState, undefined); + }); + + it('should start a span with name and parent spancontext', () => { + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); + const state = new TraceState('a=1,b=2'); + const span = tracer.startSpan('my-span', { + parent: { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceState: state, + }, + }); + assert.ok(span instanceof Span); + const context = span.context(); + assert.strictEqual(context.traceId, 'd4cda95b652f4a1592b449d5929fda1b'); + assert.strictEqual(context.traceOptions, TraceOptions.SAMPLED); + assert.deepStrictEqual(context.traceState, state); + }); + + it('should start a span with name and parent span', () => { + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); + const span = tracer.startSpan('my-span'); + const childSpan = tracer.startSpan('child-span', { + parent: span, + }); + const context = childSpan.context(); + assert.strictEqual(context.traceId, span.context().traceId); + assert.strictEqual(context.traceOptions, TraceOptions.SAMPLED); + }); + + it('should start a span with name and with invalid spancontext', () => { + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); + const span = tracer.startSpan('my-span', { + parent: { traceId: '0', spanId: '0' }, + }); + assert.ok(span instanceof Span); + const context = span.context(); + assert.ok(context.traceId.match(/[a-f0-9]{32}/)); + assert.ok(context.spanId.match(/[a-f0-9]{16}/)); + assert.strictEqual(context.traceOptions, TraceOptions.SAMPLED); + assert.deepStrictEqual(context.traceState, undefined); }); it('should return a default span with no sampling', () => { diff --git a/packages/opentelemetry-basic-tracer/test/Span.test.ts b/packages/opentelemetry-basic-tracer/test/Span.test.ts index 6fae8a4663..4d412038fd 100644 --- a/packages/opentelemetry-basic-tracer/test/Span.test.ts +++ b/packages/opentelemetry-basic-tracer/test/Span.test.ts @@ -18,47 +18,44 @@ import * as assert from 'assert'; import { Span } from '../src/Span'; import { SpanKind, - SpanContext, - TraceOptions, CanonicalCode, + TraceOptions, + SpanContext, } from '@opentelemetry/types'; import { NoopTracer } from '@opentelemetry/core'; describe('Span', () => { const tracer = new NoopTracer(); + const name = 'span1'; const spanContext: SpanContext = { traceId: 'd4cda95b652f4a1592b449d5929fda1b', spanId: '6e0c63257de34c92', traceOptions: TraceOptions.SAMPLED, }; - const name = 'span1'; it('should create a Span instance', () => { - const span = new Span(tracer, name, { kind: SpanKind.SERVER }); + const span = new Span(tracer, name, spanContext, SpanKind.SERVER); assert.ok(span instanceof Span); assert.strictEqual(span.tracer(), tracer); }); it('should get the span context of span', () => { - const span = new Span(tracer, name, { parent: spanContext }); + const span = new Span(tracer, name, spanContext, SpanKind.CLIENT); const context = span.context(); assert.strictEqual(context.traceId, spanContext.traceId); - assert.notStrictEqual(context.spanId, spanContext.spanId); - assert.strictEqual(context.traceOptions, spanContext.traceOptions); + assert.strictEqual(context.traceOptions, TraceOptions.SAMPLED); assert.strictEqual(context.traceState, undefined); assert.ok(context.spanId.match(/[a-f0-9]{16}/)); assert.ok(span.isRecordingEvents()); }); it('should return true when isRecordingEvents:true', () => { - const span = new Span(tracer, name, { isRecordingEvents: true }); + const span = new Span(tracer, name, spanContext, SpanKind.CLIENT); assert.ok(span.isRecordingEvents()); }); it('should set an attribute', () => { - const span = new Span(tracer, name, { - attributes: { service: 'service-1' }, - }); + const span = new Span(tracer, name, spanContext, SpanKind.CLIENT); ['String', 'Number', 'Boolean'].map(attType => { span.setAttribute('testKey' + attType, 'testValue' + attType); @@ -67,19 +64,24 @@ describe('Span', () => { }); it('should set an event', () => { - const span = new Span(tracer, name, {}); + const span = new Span(tracer, name, spanContext, SpanKind.CLIENT); span.addEvent('sent'); span.addEvent('rev', { attr1: 'value', attr2: 123, attr3: true }); }); it('should set a link', () => { - const span = new Span(tracer, name, {}); + const spanContext: SpanContext = { + traceId: 'a3cda95b652f4a1592b449d5929fda1b', + spanId: '5e0c63257de34c92', + traceOptions: TraceOptions.SAMPLED, + }; + const span = new Span(tracer, name, spanContext, SpanKind.CLIENT); span.addLink(spanContext); span.addLink(spanContext, { attr1: 'value', attr2: 123, attr3: true }); }); it('should set an error status', () => { - const span = new Span(tracer, name, {}); + const span = new Span(tracer, name, spanContext, SpanKind.CLIENT); span.setStatus({ code: CanonicalCode.PERMISSION_DENIED, message: 'This is an error', @@ -87,15 +89,19 @@ describe('Span', () => { }); it('should return toString', () => { - const span = new Span(tracer, name, { - kind: SpanKind.SERVER, - startTime: 100, - }); + const parentId = '5c1c63257de34c67'; + const span = new Span( + tracer, + name, + spanContext, + SpanKind.SERVER, + parentId, + 100 + ); const context = span.context(); - assert.strictEqual( span.toString(), - `Span{"traceId":"${context.traceId}","spanId":"${context.spanId}","name":"${name}","kind":1,"status":{"code":0},"startTime":100,"endTime":0}` + `Span{"traceId":"${context.traceId}","spanId":"${context.spanId}","parentId":"${parentId}","name":"${name}","kind":1,"status":{"code":0},"startTime":100,"endTime":0}` ); }); }); diff --git a/packages/opentelemetry-core/src/index.ts b/packages/opentelemetry-core/src/index.ts index ebf801b6c7..b7c48af6d0 100644 --- a/packages/opentelemetry-core/src/index.ts +++ b/packages/opentelemetry-core/src/index.ts @@ -26,3 +26,4 @@ export * from './trace/NoopTracer'; export * from './trace/sampler/ProbabilitySampler'; export * from './trace/spancontext-utils'; export * from './trace/TracerDelegate'; +export * from './trace/TraceState';