From 41112ddc690385fc6fff7cb4ddeab30557b1fd6e Mon Sep 17 00:00:00 2001 From: Peter Marton Date: Wed, 17 Jul 2019 19:23:33 -0700 Subject: [PATCH] feat(basic-tracer): make scope mamnager required --- .../src/BasicTracer.ts | 24 ++------ .../opentelemetry-basic-tracer/src/types.ts | 2 +- .../test/BasicTracer.test.ts | 61 +++++++++++++------ packages/opentelemetry-core/src/index.ts | 2 +- 4 files changed, 48 insertions(+), 41 deletions(-) diff --git a/packages/opentelemetry-basic-tracer/src/BasicTracer.ts b/packages/opentelemetry-basic-tracer/src/BasicTracer.ts index 484a0c07e12..be8dd9858f8 100644 --- a/packages/opentelemetry-basic-tracer/src/BasicTracer.ts +++ b/packages/opentelemetry-basic-tracer/src/BasicTracer.ts @@ -26,6 +26,7 @@ import { NEVER_SAMPLER, } from '@opentelemetry/core'; import { BasicTracerConfig } from '../src/types'; +import { BinaryFormat, HttpTextFormat } from '@opentelemetry/types'; /** * This class represents a basic tracer. @@ -38,9 +39,7 @@ export class BasicTracer implements types.Tracer { private _binaryFormat: types.BinaryFormat; private _httpTextFormat: types.HttpTextFormat; private _sampler: types.Sampler; - // TODO: discuss default ScopeManager (undefined, NoopScopeManager etc.) - // TODO: discuss behaviour without scope manager / with NoopScopeManager: log, throw, swallow etc. - private _scopeManager?: ScopeManager; + private _scopeManager: ScopeManager; // TODO: consume invalid span context from `SpanContext.INVALID` static defaultSpan = new NoopSpan({ @@ -53,7 +52,7 @@ export class BasicTracer implements types.Tracer { /** * Constructs a new Tracer instance. */ - constructor(config: BasicTracerConfig = {}) { + constructor(config: BasicTracerConfig) { this._binaryFormat = config.binaryFormat || new BinaryTraceContext(); this._defaultAttributes = config.defaultAttributes || {}; this._httpTextFormat = config.httpTextFormat || new HttpTraceContext(); @@ -108,14 +107,6 @@ export class BasicTracer implements types.Tracer { * Returns the current Span from the current context. */ getCurrentSpan(): types.Span { - // Return with defaultSpan if no scope manager provided. - if (!this._scopeManager) { - this._logger.warn( - 'getCurrentSpan() returns an invalid default span without a scopeManager' - ); - return BasicTracer.defaultSpan; - } - // Get the current Span from the context. return this._scopeManager.active() as types.Span; } @@ -127,11 +118,6 @@ export class BasicTracer implements types.Tracer { span: types.Span, fn: T ): ReturnType { - if (!this._scopeManager) { - this._logger.warn('withSpan(...) has no effect without a scopeManager'); - return fn(); - } - // Set given span to context. return this._scopeManager.with(span, fn); } @@ -146,14 +132,14 @@ export class BasicTracer implements types.Tracer { /** * Returns the binary format interface which can serialize/deserialize Spans. */ - getBinaryFormat(): unknown { + getBinaryFormat(): BinaryFormat { return this._binaryFormat; } /** * Returns the HTTP text format interface which can inject/extract Spans. */ - getHttpTextFormat(): unknown { + getHttpTextFormat(): HttpTextFormat { return this._httpTextFormat; } } diff --git a/packages/opentelemetry-basic-tracer/src/types.ts b/packages/opentelemetry-basic-tracer/src/types.ts index d763f691c32..b4e7bdfad24 100644 --- a/packages/opentelemetry-basic-tracer/src/types.ts +++ b/packages/opentelemetry-basic-tracer/src/types.ts @@ -56,5 +56,5 @@ export interface BasicTracerConfig { /** * Scope manager keeps context across in-process operations. */ - scopeManager?: ScopeManager; + scopeManager: ScopeManager; } diff --git a/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts b/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts index 68667e02483..5eae852423a 100644 --- a/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts +++ b/packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts @@ -23,17 +23,21 @@ import { NEVER_SAMPLER, NoopLogger, } from '@opentelemetry/core'; +import { NoopScopeManager } from '@opentelemetry/scope-base'; describe('BasicTracer', () => { describe('constructor', () => { - it('should construct an instance without options', () => { - const tracer = new BasicTracer(); + it('should construct an instance with required only options', () => { + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); assert.ok(tracer instanceof BasicTracer); }); it('should construct an instance with binary format', () => { const tracer = new BasicTracer({ binaryFormat: new BinaryTraceContext(), + scopeManager: new NoopScopeManager(), }); assert.ok(tracer instanceof BasicTracer); }); @@ -41,22 +45,27 @@ describe('BasicTracer', () => { it('should construct an instance with http text format', () => { const tracer = new BasicTracer({ httpTextFormat: new HttpTraceContext(), + scopeManager: new NoopScopeManager(), }); assert.ok(tracer instanceof BasicTracer); }); it('should construct an instance with logger', () => { - const tracer = new BasicTracer({ logger: new NoopLogger() }); + const tracer = new BasicTracer({ + logger: new NoopLogger(), + scopeManager: new NoopScopeManager(), + }); assert.ok(tracer instanceof BasicTracer); }); it('should construct an instance with sampler', () => { - const tracer = new BasicTracer({ sampler: ALWAYS_SAMPLER }); + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + sampler: ALWAYS_SAMPLER, + }); assert.ok(tracer instanceof BasicTracer); }); - it('should construct an instance with scope manager'); - it('should construct an instance with propagation'); it('should construct an instance with default attributes', () => { @@ -65,6 +74,7 @@ describe('BasicTracer', () => { region: 'eu-west', asg: 'my-asg', }, + scopeManager: new NoopScopeManager(), }); assert.ok(tracer instanceof BasicTracer); }); @@ -72,19 +82,26 @@ describe('BasicTracer', () => { describe('#startSpan', () => { it('should start a span with name only', () => { - const tracer = new BasicTracer(); + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); const span = tracer.startSpan('my-span'); assert.ok(span); }); it('should start a span with name and options', () => { - const tracer = new BasicTracer(); + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); const span = tracer.startSpan('my-span', {}); assert.ok(span); }); it('should return a default span with no sampling', () => { - const tracer = new BasicTracer({ sampler: NEVER_SAMPLER }); + const tracer = new BasicTracer({ + sampler: NEVER_SAMPLER, + scopeManager: new NoopScopeManager(), + }); const span = tracer.startSpan('my-span'); assert.deepStrictEqual(span, BasicTracer.defaultSpan); }); @@ -95,25 +112,25 @@ describe('BasicTracer', () => { }); describe('#getCurrentSpan', () => { - it('should return default span without scope manager', () => { - const tracer = new BasicTracer(); + it('should return null with NoopScopeManager', () => { + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); const currentSpan = tracer.getCurrentSpan(); - assert.deepStrictEqual(currentSpan, BasicTracer.defaultSpan); + assert.deepStrictEqual(currentSpan, null); }); - - it('should return a span with scope manager'); }); describe('#withSpan', () => { - it('should run scope without scope manager', done => { - const tracer = new BasicTracer(); + it('should run scope with NoopScopeManager scope manager', done => { + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); const span = tracer.startSpan('my-span'); tracer.withSpan(span, () => { return done(); }); }); - - it('should run and set scope with scope manager'); }); describe('#recordSpanData', () => { @@ -122,14 +139,18 @@ describe('BasicTracer', () => { describe('#getBinaryFormat', () => { it('should get default binary formatter', () => { - const tracer = new BasicTracer(); + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); assert.ok(tracer.getBinaryFormat() instanceof BinaryTraceContext); }); }); describe('#getHttpTextFormat', () => { it('should get default HTTP text formatter', () => { - const tracer = new BasicTracer(); + const tracer = new BasicTracer({ + scopeManager: new NoopScopeManager(), + }); assert.ok(tracer.getHttpTextFormat() instanceof HttpTraceContext); }); }); diff --git a/packages/opentelemetry-core/src/index.ts b/packages/opentelemetry-core/src/index.ts index 7a61430775d..b1ee5f38d68 100644 --- a/packages/opentelemetry-core/src/index.ts +++ b/packages/opentelemetry-core/src/index.ts @@ -14,9 +14,9 @@ * limitations under the License. */ +export * from './common/NoopLogger'; export * from './context/propagation/BinaryTraceContext'; export * from './context/propagation/HttpTraceContext'; -export * from './common/NoopLogger'; export * from './platform'; export * from './resources/Resource'; export * from './trace/NoopSpan';