fix: respect sampled flag in Span Processors, fix associated tests (#…

…2396) Co-authored-by: Daniel Dyla <[email protected]> Co-authored-by: Valentin Marchaud <[email protected]>
open-telemetry · Aug 7, 2021 · 46a42a1 · 46a42a1
1 parent f180870
commit 46a42a1
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 5 deletions.
diff --git a/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts b/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { context } from '@opentelemetry/api';
+import { context, TraceFlags } from '@opentelemetry/api';
 import {
   ExportResultCode,
   getEnv,
@@ -77,6 +77,11 @@ export abstract class BatchSpanProcessorBase<T extends BufferConfig> implements
     if (this._isShutdown) {
       return;
     }
+
+    if ((span.spanContext().traceFlags & TraceFlags.SAMPLED) === 0) {
+      return;
+    }
+
     this._addToBuffer(span);
   }
 

diff --git a/packages/opentelemetry-sdk-trace-base/src/export/SimpleSpanProcessor.ts b/packages/opentelemetry-sdk-trace-base/src/export/SimpleSpanProcessor.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { context } from '@opentelemetry/api';
+import { context, TraceFlags } from '@opentelemetry/api';
 import {
   ExportResultCode,
   globalErrorHandler,
@@ -50,6 +50,10 @@ export class SimpleSpanProcessor implements SpanProcessor {
       return;
     }
 
+    if ((span.spanContext().traceFlags & TraceFlags.SAMPLED) === 0) {
+      return;
+    }
+
     // prevent downstream exporter calls from generating spans
     context.with(suppressTracing(context.active()), () => {
       this._exporter.export([span], result => {

diff --git a/packages/opentelemetry-sdk-trace-base/test/common/export/BatchSpanProcessorBase.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/export/BatchSpanProcessorBase.test.ts
@@ -25,6 +25,7 @@ import * as assert from 'assert';
 import * as sinon from 'sinon';
 import { BasicTracerProvider, BufferConfig, InMemorySpanExporter, Span } from '../../../src';
 import { context } from '@opentelemetry/api';
+import { TestRecordOnlySampler } from './TestRecordOnlySampler';
 import { TestTracingSpanExporter } from './TestTracingSpanExporter';
 import { TestStackContextManager } from './TestStackContextManager';
 import { BatchSpanProcessorBase } from '../../../src/export/BatchSpanProcessorBase';
@@ -38,6 +39,15 @@ function createSampledSpan(spanName: string): Span {
   return span as Span;
 }
 
+function createUnsampledSpan(spanName: string): Span {
+  const tracer = new BasicTracerProvider({
+    sampler: new TestRecordOnlySampler(),
+  }).getTracer('default');
+  const span = tracer.startSpan(spanName);
+  span.end();
+  return span as Span;
+}
+
 class BatchSpanProcessor extends BatchSpanProcessorBase<BufferConfig> {
   onInit() {}
   onShutdown() {}
@@ -121,12 +131,14 @@ describe('BatchSpanProcessorBase', () => {
 
       const span = createSampledSpan(`${name}_0`);
 
+      processor.onStart(span);
       processor.onEnd(span);
       assert.strictEqual(processor['_finishedSpans'].length, 1);
 
       await processor.forceFlush();
       assert.strictEqual(exporter.getFinishedSpans().length, 1);
 
+      processor.onStart(span);
       processor.onEnd(span);
       assert.strictEqual(processor['_finishedSpans'].length, 1);
 
@@ -135,12 +147,28 @@ describe('BatchSpanProcessorBase', () => {
       assert.strictEqual(spy.args.length, 2);
       assert.strictEqual(exporter.getFinishedSpans().length, 0);
 
+      processor.onStart(span);
       processor.onEnd(span);
       assert.strictEqual(spy.args.length, 2);
       assert.strictEqual(processor['_finishedSpans'].length, 0);
       assert.strictEqual(exporter.getFinishedSpans().length, 0);
     });
 
+    it('should not export unsampled spans', async () => {
+      const processor = new BatchSpanProcessor(exporter, defaultBufferConfig);
+      const spy: sinon.SinonSpy = sinon.spy(exporter, 'export') as any;
+
+      const span = createUnsampledSpan(`${name}_0`);
+
+      processor.onStart(span);
+      processor.onEnd(span);
+
+      await processor.forceFlush();
+      assert.strictEqual(processor['_finishedSpans'].length, 0);
+      assert.strictEqual(exporter.getFinishedSpans().length, 0);
+      assert.strictEqual(spy.args.length, 0);
+    });
+
     it('should export the sampled spans with buffer size reached', done => {
       const clock = sinon.useFakeTimers();
       const processor = new BatchSpanProcessor(exporter, defaultBufferConfig);
@@ -153,6 +181,7 @@ describe('BatchSpanProcessorBase', () => {
         assert.strictEqual(exporter.getFinishedSpans().length, 0);
       }
       const span = createSampledSpan(`${name}_6`);
+      processor.onStart(span);
       processor.onEnd(span);
 
       setTimeout(async () => {
@@ -170,6 +199,7 @@ describe('BatchSpanProcessorBase', () => {
       const processor = new BatchSpanProcessor(exporter, defaultBufferConfig);
       for (let i = 0; i < defaultBufferConfig.maxExportBatchSize; i++) {
         const span = createSampledSpan(`${name}_${i}`);
+        processor.onStart(span);
         processor.onEnd(span);
         assert.strictEqual(exporter.getFinishedSpans().length, 0);
       }
@@ -188,6 +218,7 @@ describe('BatchSpanProcessorBase', () => {
       const processor = new BatchSpanProcessor(exporter, defaultBufferConfig);
       for (let i = 0; i < defaultBufferConfig.maxExportBatchSize; i++) {
         const span = createSampledSpan(`${name}_${i}`);
+        processor.onStart(span);
         processor.onEnd(span);
       }
       assert.strictEqual(exporter.getFinishedSpans().length, 0);
@@ -235,9 +266,11 @@ describe('BatchSpanProcessorBase', () => {
         const totalSpans = defaultBufferConfig.maxExportBatchSize * 2;
         for (let i = 0; i < totalSpans; i++) {
           const span = createSampledSpan(`${name}_${i}`);
+          processor.onStart(span);
           processor.onEnd(span);
         }
         const span = createSampledSpan(`${name}_last`);
+        processor.onStart(span);
         processor.onEnd(span);
         clock.tick(defaultBufferConfig.scheduledDelayMillis + 10);
 

diff --git a/packages/opentelemetry-sdk-trace-base/test/common/export/SimpleSpanProcessor.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/export/SimpleSpanProcessor.test.ts
@@ -38,8 +38,13 @@ import { TestStackContextManager } from './TestStackContextManager';
 import { TestTracingSpanExporter } from './TestTracingSpanExporter';
 
 describe('SimpleSpanProcessor', () => {
-  const provider = new BasicTracerProvider();
-  const exporter = new InMemorySpanExporter();
+  let provider: BasicTracerProvider;
+  let exporter: InMemorySpanExporter;
+
+  beforeEach(() => {
+    provider = new BasicTracerProvider();
+    exporter = new InMemorySpanExporter();
+  });
 
   describe('constructor', () => {
     it('should create a SimpleSpanProcessor instance', () => {
@@ -103,7 +108,7 @@ describe('SimpleSpanProcessor', () => {
       const spanContext: SpanContext = {
         traceId: 'a3cda95b652f4a1592b449d5929fda1b',
         spanId: '5e0c63257de34c92',
-        traceFlags: TraceFlags.NONE,
+        traceFlags: TraceFlags.SAMPLED,
       };
       const span = new Span(
         provider.getTracer('default'),
@@ -112,6 +117,7 @@ describe('SimpleSpanProcessor', () => {
         spanContext,
         SpanKind.CLIENT
       );
+      processor.onStart(span);
 
       sinon.stub(exporter, 'export').callsFake((_, callback) => {
         setTimeout(() => {
@@ -189,6 +195,7 @@ describe('SimpleSpanProcessor', () => {
         SpanKind.CLIENT
       );
 
+      processor.onStart(span);
       processor.onEnd(span);
 
       const exporterCreatedSpans = testTracingExporter.getExporterCreatedSpans();

diff --git a/packages/opentelemetry-sdk-trace-base/test/common/export/TestRecordOnlySampler.ts b/packages/opentelemetry-sdk-trace-base/test/common/export/TestRecordOnlySampler.ts
@@ -0,0 +1,30 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Sampler, SamplingDecision, SamplingResult } from '@opentelemetry/api';
+
+/** Sampler that always records but doesn't sample spans. */
+export class TestRecordOnlySampler implements Sampler {
+  shouldSample(): SamplingResult {
+    return {
+      decision: SamplingDecision.RECORD,
+    };
+  }
+
+  toString(): string {
+    return 'TestRecordOnlySampler';
+  }
+}