From 46a42a18570da8a0b2ae027c80018ebfb6c8096f Mon Sep 17 00:00:00 2001 From: Clark Jacobsohn Date: Sat, 7 Aug 2021 10:05:37 -0400 Subject: [PATCH] fix: respect sampled flag in Span Processors, fix associated tests (#2396) Co-authored-by: Daniel Dyla Co-authored-by: Valentin Marchaud --- .../src/export/BatchSpanProcessorBase.ts | 7 +++- .../src/export/SimpleSpanProcessor.ts | 6 +++- .../export/BatchSpanProcessorBase.test.ts | 33 +++++++++++++++++++ .../common/export/SimpleSpanProcessor.test.ts | 13 ++++++-- .../common/export/TestRecordOnlySampler.ts | 30 +++++++++++++++++ 5 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 packages/opentelemetry-sdk-trace-base/test/common/export/TestRecordOnlySampler.ts diff --git a/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts b/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts index 03efc430bd..3785a6ea28 100644 --- 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 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 index c92b869bad..20c096dbbe 100644 --- 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 index 7015d03595..42a7aa3fd5 100644 --- 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 { 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 index df5fa66f81..52fd2979ce 100644 --- 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 new file mode 100644 index 0000000000..c456780e86 --- /dev/null +++ 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'; + } +}