chore: create NoopSpan instead reusing NOOP_SPAN (#1899)

Flarna · web-flow · commit 000a8ac099a3 · 2021-02-10T10:58:46.000-05:00
diff --git a/packages/opentelemetry-api/src/context/context.ts b/packages/opentelemetry-api/src/context/context.ts
@@ -15,7 +15,8 @@
  */
 
 import { Context, createContextKey } from '@opentelemetry/context-base';
-import { Baggage, NoopSpan, Span, SpanContext } from '../';
+import { Baggage, Span, SpanContext } from '../';
+import { NoopSpan } from '../trace/NoopSpan';
 
 /**
  * span key
diff --git a/packages/opentelemetry-api/src/index.ts b/packages/opentelemetry-api/src/index.ts
@@ -26,7 +26,6 @@ export * from './trace/Event';
 export * from './trace/link_context';
 export * from './trace/link';
 export * from './trace/NoopLogger';
-export * from './trace/NoopSpan';
 export * from './trace/NoopTracer';
 export * from './trace/NoopTracerProvider';
 export * from './trace/ProxyTracer';
diff --git a/packages/opentelemetry-api/src/trace/NoopSpan.ts b/packages/opentelemetry-api/src/trace/NoopSpan.ts
@@ -73,5 +73,3 @@ export class NoopSpan implements Span {
   // By default does nothing
   recordException(_exception: Exception, _time?: TimeInput): void {}
 }
-
-export const NOOP_SPAN = new NoopSpan();
diff --git a/packages/opentelemetry-api/src/trace/NoopTracer.ts b/packages/opentelemetry-api/src/trace/NoopTracer.ts
@@ -16,7 +16,7 @@
 
 import { Span, SpanOptions, Tracer, SpanContext } from '..';
 import { Context } from '@opentelemetry/context-base';
-import { NoopSpan, NOOP_SPAN } from './NoopSpan';
+import { NoopSpan } from './NoopSpan';
 import { isSpanContextValid } from './spancontext-utils';
 import { getSpanContext } from '../context/context';
 
@@ -28,7 +28,7 @@ export class NoopTracer implements Tracer {
   startSpan(name: string, options?: SpanOptions, context?: Context): Span {
     const root = Boolean(options?.root);
     if (root) {
-      return NOOP_SPAN;
+      return new NoopSpan();
     }
 
     const parentFromContext = context && getSpanContext(context);
@@ -39,7 +39,7 @@ export class NoopTracer implements Tracer {
     ) {
       return new NoopSpan(parentFromContext);
     } else {
-      return NOOP_SPAN;
+      return new NoopSpan();
     }
   }
 }
diff --git a/packages/opentelemetry-api/test/api/api.test.ts b/packages/opentelemetry-api/test/api/api.test.ts
@@ -17,7 +17,6 @@
 import * as assert from 'assert';
 import api, {
   TraceFlags,
-  NoopSpan,
   NoopTracerProvider,
   NoopTracer,
   SpanOptions,
@@ -33,6 +32,7 @@ import api, {
   defaultTextMapSetter,
   defaultTextMapGetter,
 } from '../../src';
+import { NoopSpan } from '../../src/trace/NoopSpan';
 
 describe('API', () => {
   it('should expose a tracer provider via getTracerProvider', () => {
diff --git a/packages/opentelemetry-api/test/noop-implementations/noop-span.test.ts b/packages/opentelemetry-api/test/noop-implementations/noop-span.test.ts
@@ -19,9 +19,9 @@ import {
   SpanStatusCode,
   INVALID_SPANID,
   INVALID_TRACEID,
-  NoopSpan,
   TraceFlags,
 } from '../../src';
+import { NoopSpan } from '../../src/trace/NoopSpan';
 
 describe('NoopSpan', () => {
   it('do not crash', () => {
diff --git a/packages/opentelemetry-api/test/noop-implementations/noop-tracer.test.ts b/packages/opentelemetry-api/test/noop-implementations/noop-tracer.test.ts
@@ -17,28 +17,26 @@
 import * as assert from 'assert';
 import {
   NoopTracer,
-  NOOP_SPAN,
   SpanContext,
   SpanKind,
   TraceFlags,
   context,
   setSpanContext,
 } from '../../src';
+import { NoopSpan } from '../../src/trace/NoopSpan';
 
 describe('NoopTracer', () => {
   it('should not crash', () => {
     const tracer = new NoopTracer();
 
-    assert.deepStrictEqual(tracer.startSpan('span-name'), NOOP_SPAN);
-    assert.deepStrictEqual(
-      tracer.startSpan('span-name1', { kind: SpanKind.CLIENT }),
-      NOOP_SPAN
+    assert.ok(tracer.startSpan('span-name') instanceof NoopSpan);
+    assert.ok(
+      tracer.startSpan('span-name1', { kind: SpanKind.CLIENT }) instanceof
+        NoopSpan
     );
-    assert.deepStrictEqual(
-      tracer.startSpan('span-name2', {
-        kind: SpanKind.CLIENT,
-      }),
-      NOOP_SPAN
+    assert.ok(
+      tracer.startSpan('span-name2', { kind: SpanKind.CLIENT }) instanceof
+        NoopSpan
     );
   });
 
diff --git a/packages/opentelemetry-api/test/proxy-implementations/proxy-tracer.test.ts b/packages/opentelemetry-api/test/proxy-implementations/proxy-tracer.test.ts
@@ -17,8 +17,6 @@
 import * as assert from 'assert';
 import * as sinon from 'sinon';
 import {
-  NoopSpan,
-  NOOP_SPAN,
   ProxyTracerProvider,
   SpanKind,
   TracerProvider,
@@ -29,6 +27,7 @@ import {
   ROOT_CONTEXT,
   SpanOptions,
 } from '../../src';
+import { NoopSpan } from '../../src/trace/NoopSpan';
 
 describe('ProxyTracer', () => {
   let provider: ProxyTracerProvider;
@@ -52,16 +51,14 @@ describe('ProxyTracer', () => {
     it('startSpan should return Noop Spans', () => {
       const tracer = provider.getTracer('test');
 
-      assert.deepStrictEqual(tracer.startSpan('span-name'), NOOP_SPAN);
-      assert.deepStrictEqual(
-        tracer.startSpan('span-name1', { kind: SpanKind.CLIENT }),
-        NOOP_SPAN
+      assert.ok(tracer.startSpan('span-name') instanceof NoopSpan);
+      assert.ok(
+        tracer.startSpan('span-name1', { kind: SpanKind.CLIENT }) instanceof
+          NoopSpan
       );
-      assert.deepStrictEqual(
-        tracer.startSpan('span-name2', {
-          kind: SpanKind.CLIENT,
-        }),
-        NOOP_SPAN
+      assert.ok(
+        tracer.startSpan('span-name2', { kind: SpanKind.CLIENT }) instanceof
+          NoopSpan
       );
     });
   });
diff --git a/packages/opentelemetry-instrumentation-http/src/http.ts b/packages/opentelemetry-instrumentation-http/src/http.ts
@@ -25,7 +25,7 @@ import {
   ROOT_CONTEXT,
   getSpan,
   suppressInstrumentation,
-  NoopSpan,
+  NOOP_TRACER,
 } from '@opentelemetry/api';
 import type * as http from 'http';
 import type * as https from 'https';
@@ -587,7 +587,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
     if (requireParent === true && currentSpan === undefined) {
       // TODO: Refactor this when a solution is found in
       // https://github.com/open-telemetry/opentelemetry-specification/issues/530
-      span = new NoopSpan();
+      span = NOOP_TRACER.startSpan(name, options);
     } else if (requireParent === true && currentSpan?.context().isRemote) {
       span = currentSpan;
     } else {
diff --git a/packages/opentelemetry-instrumentation-http/test/functionals/http-disable.test.ts b/packages/opentelemetry-instrumentation-http/test/functionals/http-disable.test.ts
@@ -57,7 +57,7 @@ describe('HttpInstrumentation', () => {
     });
 
     beforeEach(() => {
-      NOOP_TRACER.startSpan = sinon.spy();
+      sinon.spy(NOOP_TRACER, 'startSpan');
     });
 
     afterEach(() => {
diff --git a/packages/opentelemetry-instrumentation-http/test/functionals/https-disable.test.ts b/packages/opentelemetry-instrumentation-http/test/functionals/https-disable.test.ts
@@ -66,7 +66,7 @@ describe('HttpsInstrumentation', () => {
     });
 
     beforeEach(() => {
-      NOOP_TRACER.startSpan = sinon.spy();
+      sinon.spy(NOOP_TRACER, 'startSpan');
     });
 
     afterEach(() => {
diff --git a/packages/opentelemetry-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-node/test/NodeTracerProvider.test.ts
@@ -21,7 +21,6 @@ import {
   setSpan,
   setSpanContext,
   getSpan,
-  NoopSpan,
 } from '@opentelemetry/api';
 import { AlwaysOnSampler, AlwaysOffSampler } from '@opentelemetry/core';
 import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
@@ -121,7 +120,6 @@ describe('NodeTracerProvider', () => {
         logger: new NoopLogger(),
       });
       const span = provider.getTracer('default').startSpan('my-span');
-      assert.ok(span instanceof NoopSpan);
       assert.strictEqual(span.context().traceFlags, TraceFlags.NONE);
       assert.strictEqual(span.isRecording(), false);
     });
diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts
@@ -25,7 +25,7 @@ import {
   ROOT_CONTEXT,
   getSpan,
   suppressInstrumentation,
-  NoopSpan,
+  NOOP_TRACER,
 } from '@opentelemetry/api';
 import { BasePlugin } from '@opentelemetry/core';
 import type {
@@ -446,7 +446,7 @@ export class HttpPlugin extends BasePlugin<Http> {
     if (requireParent === true && currentSpan === undefined) {
       // TODO: Refactor this when a solution is found in
       // https://github.com/open-telemetry/opentelemetry-specification/issues/530
-      span = new NoopSpan();
+      span = NOOP_TRACER.startSpan(name, options);
     } else if (requireParent === true && currentSpan?.context().isRemote) {
       span = currentSpan;
     } else {
diff --git a/packages/opentelemetry-plugin-http/test/functionals/http-disable.test.ts b/packages/opentelemetry-plugin-http/test/functionals/http-disable.test.ts
@@ -51,7 +51,7 @@ describe('HttpPlugin', () => {
     });
 
     beforeEach(() => {
-      NOOP_TRACER.startSpan = sinon.spy();
+      sinon.spy(NOOP_TRACER, 'startSpan');
     });
 
     afterEach(() => {
diff --git a/packages/opentelemetry-tracing/src/Tracer.ts b/packages/opentelemetry-tracing/src/Tracer.ts
@@ -67,7 +67,7 @@ export class Tracer implements api.Tracer {
   ): api.Span {
     if (api.isInstrumentationSuppressed(context)) {
       this.logger.debug('Instrumentation suppressed, returning Noop Span');
-      return api.NOOP_SPAN;
+      return api.NOOP_TRACER.startSpan(name, options, context);
     }
 
     const parentContext = getParent(options, context);
@@ -103,7 +103,11 @@ export class Tracer implements api.Tracer {
     const spanContext = { traceId, spanId, traceFlags, traceState };
     if (samplingResult.decision === api.SamplingDecision.NOT_RECORD) {
       this.logger.debug('Recording is off, starting no recording span');
-      return new api.NoopSpan(spanContext);
+      return api.NOOP_TRACER.startSpan(
+        name,
+        options,
+        api.setSpanContext(context, spanContext)
+      );
     }
 
     const span = new Span(
diff --git a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts
@@ -20,7 +20,6 @@ import {
   TraceFlags,
   ROOT_CONTEXT,
   NoopLogger,
-  NoopSpan,
   setSpan,
   setSpanContext,
   getSpan,
@@ -258,13 +257,13 @@ describe('BasicTracerProvider', () => {
       assert.deepStrictEqual(context.traceState, undefined);
     });
 
-    it('should return a no recording span when never sampling', () => {
+    it('should return a non recording span when never sampling', () => {
       const tracer = new BasicTracerProvider({
         sampler: new AlwaysOffSampler(),
         logger: new NoopLogger(),
       }).getTracer('default');
       const span = tracer.startSpan('my-span');
-      assert.ok(span instanceof NoopSpan);
+      assert.ok(!span.isRecording());
       const context = span.context();
       assert.ok(context.traceId.match(/[a-f0-9]{32}/));
       assert.ok(context.spanId.match(/[a-f0-9]{16}/));
diff --git a/packages/opentelemetry-tracing/test/Tracer.test.ts b/packages/opentelemetry-tracing/test/Tracer.test.ts
@@ -16,10 +16,8 @@
 
 import * as assert from 'assert';
 import {
-  NoopSpan,
   Sampler,
   SamplingDecision,
-  NOOP_SPAN,
   TraceFlags,
   ROOT_CONTEXT,
   suppressInstrumentation,
@@ -79,7 +77,7 @@ describe('Tracer', () => {
       tracerProvider
     );
     const span = tracer.startSpan('span1');
-    assert.ok(span instanceof NoopSpan);
+    assert.ok(!span.isRecording());
     span.end();
   });
 
@@ -90,7 +88,7 @@ describe('Tracer', () => {
       tracerProvider
     );
     const span = tracer.startSpan('span2');
-    assert.ok(!(span instanceof NoopSpan));
+    assert.ok(span.isRecording());
     span.end();
   });
 
@@ -130,7 +128,7 @@ describe('Tracer', () => {
 
       const span = tracer.startSpan('span3', undefined, context);
 
-      assert.equal(span, NOOP_SPAN);
+      assert.ok(!span.isRecording());
       span.end();
 
       done();

Original file line number	Diff line number	Diff line change
`@@ -73,5 +73,3 @@ export class NoopSpan implements Span {`
`73`	`73`	`// By default does nothing`
`74`	`74`	`recordException(_exception: Exception, _time?: TimeInput): void {}`
`75`	`75`	`}`
`76`		`-`
`77`		`-export const NOOP_SPAN = new NoopSpan();`
Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@`
`16`	`16`
`17`	`17`	`import { Span, SpanOptions, Tracer, SpanContext } from '..';`
`18`	`18`	`import { Context } from '@opentelemetry/context-base';`
`19`		`-import { NoopSpan, NOOP_SPAN } from './NoopSpan';`
	`19`	`+import { NoopSpan } from './NoopSpan';`
`20`	`20`	`import { isSpanContextValid } from './spancontext-utils';`
`21`	`21`	`import { getSpanContext } from '../context/context';`
`22`	`22`
`@@ -28,7 +28,7 @@ export class NoopTracer implements Tracer {`
`28`	`28`	`startSpan(name: string, options?: SpanOptions, context?: Context): Span {`
`29`	`29`	`const root = Boolean(options?.root);`
`30`	`30`	`if (root) {`
`31`		`- return NOOP_SPAN;`
	`31`	`+ return new NoopSpan();`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`const parentFromContext = context && getSpanContext(context);`
`@@ -39,7 +39,7 @@ export class NoopTracer implements Tracer {`
`39`	`39`	`) {`
`40`	`40`	`return new NoopSpan(parentFromContext);`
`41`	`41`	`} else {`
`42`		`- return NOOP_SPAN;`
	`42`	`+ return new NoopSpan();`
`43`	`43`	`}`
`44`	`44`	`}`
`45`	`45`	`}`