From 6e1595dc9e397d11dace582545beebee1bb7ecf2 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Fri, 23 Apr 2021 15:41:49 -0400 Subject: [PATCH 1/4] chore: do not export singletons --- src/api/context.ts | 3 +-- src/api/propagation.ts | 14 +++++++------- src/propagation/NoopTextMapPropagator.ts | 2 -- src/trace/NoopTracer.ts | 2 -- src/trace/NoopTracerProvider.ts | 6 ++---- src/trace/ProxyTracer.ts | 4 ++-- src/trace/ProxyTracerProvider.ts | 4 ++-- test/internal/global.test.ts | 17 ++++++++++------- 8 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/api/context.ts b/src/api/context.ts index 34a1b806..04af0489 100644 --- a/src/api/context.ts +++ b/src/api/context.ts @@ -23,7 +23,6 @@ import { } from '../internal/global-utils'; const API_NAME = 'context'; -const NOOP_CONTEXT_MANAGER = new NoopContextManager(); /** * Singleton object which represents the entry point to the OpenTelemetry Context API @@ -87,7 +86,7 @@ export class ContextAPI { } private _getContextManager(): ContextManager { - return getGlobal(API_NAME) || NOOP_CONTEXT_MANAGER; + return getGlobal(API_NAME) || new NoopContextManager(); } /** Disable and remove the global context manager */ diff --git a/src/api/propagation.ts b/src/api/propagation.ts index d83d4e7d..d3ff69d0 100644 --- a/src/api/propagation.ts +++ b/src/api/propagation.ts @@ -15,7 +15,12 @@ */ import { Context } from '../context/types'; -import { NOOP_TEXT_MAP_PROPAGATOR } from '../propagation/NoopTextMapPropagator'; +import { + getGlobal, + registerGlobal, + unregisterGlobal, +} from '../internal/global-utils'; +import { NoopTextMapPropagator } from '../propagation/NoopTextMapPropagator'; import { defaultTextMapGetter, defaultTextMapSetter, @@ -23,11 +28,6 @@ import { TextMapPropagator, TextMapSetter, } from '../propagation/TextMapPropagator'; -import { - getGlobal, - registerGlobal, - unregisterGlobal, -} from '../internal/global-utils'; const API_NAME = 'propagation'; @@ -101,6 +101,6 @@ export class PropagationAPI { } private _getGlobalPropagator(): TextMapPropagator { - return getGlobal(API_NAME) || NOOP_TEXT_MAP_PROPAGATOR; + return getGlobal(API_NAME) || new NoopTextMapPropagator(); } } diff --git a/src/propagation/NoopTextMapPropagator.ts b/src/propagation/NoopTextMapPropagator.ts index 4d15ac8d..16d90cf9 100644 --- a/src/propagation/NoopTextMapPropagator.ts +++ b/src/propagation/NoopTextMapPropagator.ts @@ -31,5 +31,3 @@ export class NoopTextMapPropagator implements TextMapPropagator { return []; } } - -export const NOOP_TEXT_MAP_PROPAGATOR = new NoopTextMapPropagator(); diff --git a/src/trace/NoopTracer.ts b/src/trace/NoopTracer.ts index 556b1d70..b24ee3c6 100644 --- a/src/trace/NoopTracer.ts +++ b/src/trace/NoopTracer.ts @@ -55,5 +55,3 @@ function isSpanContext(spanContext: any): spanContext is SpanContext { typeof spanContext['traceFlags'] === 'number' ); } - -export const NOOP_TRACER = new NoopTracer(); diff --git a/src/trace/NoopTracerProvider.ts b/src/trace/NoopTracerProvider.ts index 09c86bd7..0d23d18d 100644 --- a/src/trace/NoopTracerProvider.ts +++ b/src/trace/NoopTracerProvider.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { NOOP_TRACER } from './NoopTracer'; +import { NoopTracer } from './NoopTracer'; import { Tracer } from './tracer'; import { TracerProvider } from './tracer_provider'; @@ -26,8 +26,6 @@ import { TracerProvider } from './tracer_provider'; */ export class NoopTracerProvider implements TracerProvider { getTracer(_name?: string, _version?: string): Tracer { - return NOOP_TRACER; + return new NoopTracer(); } } - -export const NOOP_TRACER_PROVIDER = new NoopTracerProvider(); diff --git a/src/trace/ProxyTracer.ts b/src/trace/ProxyTracer.ts index 05e55f2d..6a78d747 100644 --- a/src/trace/ProxyTracer.ts +++ b/src/trace/ProxyTracer.ts @@ -15,7 +15,7 @@ */ import { Context } from '../context/types'; -import { NOOP_TRACER } from './NoopTracer'; +import { NoopTracer } from './NoopTracer'; import { ProxyTracerProvider } from './ProxyTracerProvider'; import { Span } from './span'; import { SpanOptions } from './SpanOptions'; @@ -50,7 +50,7 @@ export class ProxyTracer implements Tracer { const tracer = this._provider.getDelegateTracer(this.name, this.version); if (!tracer) { - return NOOP_TRACER; + return new NoopTracer(); } this._delegate = tracer; diff --git a/src/trace/ProxyTracerProvider.ts b/src/trace/ProxyTracerProvider.ts index e124dc95..8000f65e 100644 --- a/src/trace/ProxyTracerProvider.ts +++ b/src/trace/ProxyTracerProvider.ts @@ -17,7 +17,7 @@ import { Tracer } from './tracer'; import { TracerProvider } from './tracer_provider'; import { ProxyTracer } from './ProxyTracer'; -import { NOOP_TRACER_PROVIDER } from './NoopTracerProvider'; +import { NoopTracerProvider } from './NoopTracerProvider'; /** * Tracer provider which provides {@link ProxyTracer}s. @@ -41,7 +41,7 @@ export class ProxyTracerProvider implements TracerProvider { } getDelegate(): TracerProvider { - return this._delegate ?? NOOP_TRACER_PROVIDER; + return this._delegate ?? new NoopTracerProvider(); } /** diff --git a/test/internal/global.test.ts b/test/internal/global.test.ts index 3932722b..c8210924 100644 --- a/test/internal/global.test.ts +++ b/test/internal/global.test.ts @@ -67,20 +67,19 @@ describe('Global Utils', () => { }); it('should disable both if one is disabled', () => { - const original = api1.context['_getContextManager'](); - - api1.context.setGlobalContextManager(new NoopContextManager()); + const manager = new NoopContextManager(); + api1.context.setGlobalContextManager(manager); - assert.notStrictEqual(original, api1.context['_getContextManager']()); + assert.strictEqual(manager, api1.context['_getContextManager']()); api2.context.disable(); - assert.strictEqual(original, api1.context['_getContextManager']()); + assert.notStrictEqual(manager, api1.context['_getContextManager']()); }); it('should return the module NoOp implementation if the version is a mismatch', () => { - const original = api1.context['_getContextManager'](); const newContextManager = new NoopContextManager(); api1.context.setGlobalContextManager(newContextManager); + // ensure new context manager is returned assert.strictEqual(api1.context['_getContextManager'](), newContextManager); const globalInstance = getGlobal('context'); @@ -88,7 +87,11 @@ describe('Global Utils', () => { // @ts-expect-error we are modifying internals for testing purposes here _globalThis[Symbol.for(GLOBAL_API_SYMBOL_KEY)].version = '0.0.1'; - assert.strictEqual(api1.context['_getContextManager'](), original); + // ensure new context manager is not returned because version above is incompatible + assert.notStrictEqual( + api1.context['_getContextManager'](), + newContextManager + ); }); it('should log an error if there is a duplicate registration', () => { From 79dbb9d24e5ba87fe01bd5f3ff6f18f8abf7d70d Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 29 Apr 2021 08:11:18 -0400 Subject: [PATCH 2/4] chore: do not create new noops on every call --- src/api/context.ts | 3 ++- src/api/propagation.ts | 3 ++- src/trace/ProxyTracerProvider.ts | 6 +++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/api/context.ts b/src/api/context.ts index 04af0489..34a1b806 100644 --- a/src/api/context.ts +++ b/src/api/context.ts @@ -23,6 +23,7 @@ import { } from '../internal/global-utils'; const API_NAME = 'context'; +const NOOP_CONTEXT_MANAGER = new NoopContextManager(); /** * Singleton object which represents the entry point to the OpenTelemetry Context API @@ -86,7 +87,7 @@ export class ContextAPI { } private _getContextManager(): ContextManager { - return getGlobal(API_NAME) || new NoopContextManager(); + return getGlobal(API_NAME) || NOOP_CONTEXT_MANAGER; } /** Disable and remove the global context manager */ diff --git a/src/api/propagation.ts b/src/api/propagation.ts index d3ff69d0..a4d336a4 100644 --- a/src/api/propagation.ts +++ b/src/api/propagation.ts @@ -30,6 +30,7 @@ import { } from '../propagation/TextMapPropagator'; const API_NAME = 'propagation'; +const NOOP_TEXT_MAP_PROPAGATOR = new NoopTextMapPropagator(); /** * Singleton object which represents the entry point to the OpenTelemetry Propagation API @@ -101,6 +102,6 @@ export class PropagationAPI { } private _getGlobalPropagator(): TextMapPropagator { - return getGlobal(API_NAME) || new NoopTextMapPropagator(); + return getGlobal(API_NAME) || NOOP_TEXT_MAP_PROPAGATOR; } } diff --git a/src/trace/ProxyTracerProvider.ts b/src/trace/ProxyTracerProvider.ts index 8000f65e..439f940c 100644 --- a/src/trace/ProxyTracerProvider.ts +++ b/src/trace/ProxyTracerProvider.ts @@ -28,7 +28,7 @@ import { NoopTracerProvider } from './NoopTracerProvider'; * all tracers already provided will use the provided delegate implementation. */ export class ProxyTracerProvider implements TracerProvider { - private _delegate?: TracerProvider; + private _delegate: TracerProvider = new NoopTracerProvider(); /** * Get a {@link ProxyTracer} @@ -41,7 +41,7 @@ export class ProxyTracerProvider implements TracerProvider { } getDelegate(): TracerProvider { - return this._delegate ?? new NoopTracerProvider(); + return this._delegate; } /** @@ -52,6 +52,6 @@ export class ProxyTracerProvider implements TracerProvider { } getDelegateTracer(name: string, version?: string): Tracer | undefined { - return this._delegate?.getTracer(name, version); + return this._delegate.getTracer(name, version); } } From 15b2002edfe9ba591a9733780ab89fccd781889d Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 29 Apr 2021 08:27:23 -0400 Subject: [PATCH 3/4] Delegate is initially undefined --- src/trace/ProxyTracerProvider.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/trace/ProxyTracerProvider.ts b/src/trace/ProxyTracerProvider.ts index 439f940c..738d5dc7 100644 --- a/src/trace/ProxyTracerProvider.ts +++ b/src/trace/ProxyTracerProvider.ts @@ -19,6 +19,8 @@ import { TracerProvider } from './tracer_provider'; import { ProxyTracer } from './ProxyTracer'; import { NoopTracerProvider } from './NoopTracerProvider'; +export const NOOP_TRACER_PROVIDER = new NoopTracerProvider(); + /** * Tracer provider which provides {@link ProxyTracer}s. * @@ -28,7 +30,7 @@ import { NoopTracerProvider } from './NoopTracerProvider'; * all tracers already provided will use the provided delegate implementation. */ export class ProxyTracerProvider implements TracerProvider { - private _delegate: TracerProvider = new NoopTracerProvider(); + private _delegate?: TracerProvider; /** * Get a {@link ProxyTracer} @@ -41,7 +43,7 @@ export class ProxyTracerProvider implements TracerProvider { } getDelegate(): TracerProvider { - return this._delegate; + return this._delegate ?? NOOP_TRACER_PROVIDER; } /** @@ -52,6 +54,6 @@ export class ProxyTracerProvider implements TracerProvider { } getDelegateTracer(name: string, version?: string): Tracer | undefined { - return this._delegate.getTracer(name, version); + return this._delegate?.getTracer(name, version); } } From b9e2dccfd5d4be8d4b4a559c89406be62a7fb67e Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 29 Apr 2021 10:46:14 -0400 Subject: [PATCH 4/4] Remove noops and fix tests --- src/index.ts | 4 ---- src/trace/ProxyTracerProvider.ts | 2 +- test/api/api.test.ts | 24 +++++++++---------- test/noop-implementations/noop-tracer.test.ts | 6 ++--- .../proxy-tracer.test.ts | 12 +++++----- 5 files changed, 22 insertions(+), 26 deletions(-) diff --git a/src/index.ts b/src/index.ts index b36eaccd..22e88eb7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -18,14 +18,11 @@ export * from './baggage'; export * from './common/Exception'; export * from './common/Time'; export * from './diag'; -export * from './propagation/NoopTextMapPropagator'; export * from './propagation/TextMapPropagator'; export * from './trace/attributes'; export * from './trace/Event'; export * from './trace/link_context'; export * from './trace/link'; -export * from './trace/NoopTracer'; -export * from './trace/NoopTracerProvider'; export * from './trace/ProxyTracer'; export * from './trace/ProxyTracerProvider'; export * from './trace/Sampler'; @@ -51,7 +48,6 @@ export { } from './trace/spancontext-utils'; export * from './context/context'; -export * from './context/NoopContextManager'; export * from './context/types'; import { ContextAPI } from './api/context'; diff --git a/src/trace/ProxyTracerProvider.ts b/src/trace/ProxyTracerProvider.ts index 738d5dc7..0d2d0b0e 100644 --- a/src/trace/ProxyTracerProvider.ts +++ b/src/trace/ProxyTracerProvider.ts @@ -19,7 +19,7 @@ import { TracerProvider } from './tracer_provider'; import { ProxyTracer } from './ProxyTracer'; import { NoopTracerProvider } from './NoopTracerProvider'; -export const NOOP_TRACER_PROVIDER = new NoopTracerProvider(); +const NOOP_TRACER_PROVIDER = new NoopTracerProvider(); /** * Tracer provider which provides {@link ProxyTracer}s. diff --git a/test/api/api.test.ts b/test/api/api.test.ts index 8049f25f..7f6bce84 100644 --- a/test/api/api.test.ts +++ b/test/api/api.test.ts @@ -16,25 +16,25 @@ import * as assert from 'assert'; import api, { - TraceFlags, - NoopTracerProvider, - NoopTracer, - SpanOptions, - Span, context, - trace, - propagation, - TextMapPropagator, Context, - TextMapSetter, - TextMapGetter, - ROOT_CONTEXT, - defaultTextMapSetter, defaultTextMapGetter, + defaultTextMapSetter, diag, + propagation, + ROOT_CONTEXT, + Span, + SpanOptions, + TextMapGetter, + TextMapPropagator, + TextMapSetter, + trace, + TraceFlags, } from '../../src'; import { DiagAPI } from '../../src/api/diag'; import { NonRecordingSpan } from '../../src/trace/NonRecordingSpan'; +import { NoopTracer } from '../../src/trace/NoopTracer'; +import { NoopTracerProvider } from '../../src/trace/NoopTracerProvider'; // DiagLogger implementation const diagLoggerFunctions = [ diff --git a/test/noop-implementations/noop-tracer.test.ts b/test/noop-implementations/noop-tracer.test.ts index 7527ae7b..555fc776 100644 --- a/test/noop-implementations/noop-tracer.test.ts +++ b/test/noop-implementations/noop-tracer.test.ts @@ -16,14 +16,14 @@ import * as assert from 'assert'; import { - NoopTracer, + context, + setSpanContext, SpanContext, SpanKind, TraceFlags, - context, - setSpanContext, } from '../../src'; import { NonRecordingSpan } from '../../src/trace/NonRecordingSpan'; +import { NoopTracer } from '../../src/trace/NoopTracer'; describe('NoopTracer', () => { it('should not crash', () => { diff --git a/test/proxy-implementations/proxy-tracer.test.ts b/test/proxy-implementations/proxy-tracer.test.ts index c350913a..be7bc09a 100644 --- a/test/proxy-implementations/proxy-tracer.test.ts +++ b/test/proxy-implementations/proxy-tracer.test.ts @@ -17,17 +17,17 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { - ProxyTracerProvider, - SpanKind, - TracerProvider, ProxyTracer, - Tracer, - Span, - NoopTracer, + ProxyTracerProvider, ROOT_CONTEXT, + Span, + SpanKind, SpanOptions, + Tracer, + TracerProvider, } from '../../src'; import { NonRecordingSpan } from '../../src/trace/NonRecordingSpan'; +import { NoopTracer } from '../../src/trace/NoopTracer'; describe('ProxyTracer', () => { let provider: ProxyTracerProvider;