From 9d66288c6333d25cb25eb4aa7a96224e1d5fb177 Mon Sep 17 00:00:00 2001 From: Clark Jacobsohn Date: Tue, 3 Aug 2021 18:07:49 -0400 Subject: [PATCH 1/3] fix: respect sampled flag in Span Processors, fix associated tests --- .../src/export/BatchSpanProcessorBase.ts | 7 +++++- .../src/export/SimpleSpanProcessor.ts | 6 ++++- .../export/BatchSpanProcessorBase.test.ts | 24 +++++++++++++++++++ .../common/export/SimpleSpanProcessor.test.ts | 12 +++++++--- 4 files changed, 44 insertions(+), 5 deletions(-) diff --git a/packages/opentelemetry-tracing/src/export/BatchSpanProcessorBase.ts b/packages/opentelemetry-tracing/src/export/BatchSpanProcessorBase.ts index 03efc430bd..3785a6ea28 100644 --- a/packages/opentelemetry-tracing/src/export/BatchSpanProcessorBase.ts +++ b/packages/opentelemetry-tracing/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-tracing/src/export/SimpleSpanProcessor.ts b/packages/opentelemetry-tracing/src/export/SimpleSpanProcessor.ts index c92b869bad..20c096dbbe 100644 --- a/packages/opentelemetry-tracing/src/export/SimpleSpanProcessor.ts +++ b/packages/opentelemetry-tracing/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-tracing/test/common/export/BatchSpanProcessorBase.test.ts b/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts index 7015d03595..84548e11e3 100644 --- a/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts +++ b/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts @@ -16,6 +16,7 @@ import { diag } from '@opentelemetry/api'; import { + AlwaysOffSampler, AlwaysOnSampler, ExportResultCode, loggingErrorHandler, @@ -38,6 +39,15 @@ function createSampledSpan(spanName: string): Span { return span as Span; } +function createUnsampledSpan(spanName: string): Span { + const tracer = new BasicTracerProvider({ + sampler: new AlwaysOffSampler(), + }).getTracer('default'); + const span = tracer.startSpan(spanName); + span.end(); + return span as Span; +} + class BatchSpanProcessor extends BatchSpanProcessorBase { onInit() {} onShutdown() {} @@ -141,6 +151,20 @@ describe('BatchSpanProcessorBase', () => { 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.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); diff --git a/packages/opentelemetry-tracing/test/common/export/SimpleSpanProcessor.test.ts b/packages/opentelemetry-tracing/test/common/export/SimpleSpanProcessor.test.ts index df5fa66f81..a37ee12bd6 100644 --- a/packages/opentelemetry-tracing/test/common/export/SimpleSpanProcessor.test.ts +++ b/packages/opentelemetry-tracing/test/common/export/SimpleSpanProcessor.test.ts @@ -38,8 +38,14 @@ 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 +109,7 @@ describe('SimpleSpanProcessor', () => { const spanContext: SpanContext = { traceId: 'a3cda95b652f4a1592b449d5929fda1b', spanId: '5e0c63257de34c92', - traceFlags: TraceFlags.NONE, + traceFlags: TraceFlags.SAMPLED, }; const span = new Span( provider.getTracer('default'), From 0d41684d9ebe7b2a7a309a953c8330d638f56c9e Mon Sep 17 00:00:00 2001 From: Clark Jacobsohn Date: Wed, 4 Aug 2021 15:39:10 -0400 Subject: [PATCH 2/3] test: use a record-only sampler for test --- .../export/BatchSpanProcessorBase.test.ts | 4 +-- .../common/export/TestRecordOnlySampler.ts | 30 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 packages/opentelemetry-tracing/test/common/export/TestRecordOnlySampler.ts diff --git a/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts b/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts index 84548e11e3..ba13641bfb 100644 --- a/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts +++ b/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts @@ -16,7 +16,6 @@ import { diag } from '@opentelemetry/api'; import { - AlwaysOffSampler, AlwaysOnSampler, ExportResultCode, loggingErrorHandler, @@ -26,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'; @@ -41,7 +41,7 @@ function createSampledSpan(spanName: string): Span { function createUnsampledSpan(spanName: string): Span { const tracer = new BasicTracerProvider({ - sampler: new AlwaysOffSampler(), + sampler: new TestRecordOnlySampler(), }).getTracer('default'); const span = tracer.startSpan(spanName); span.end(); diff --git a/packages/opentelemetry-tracing/test/common/export/TestRecordOnlySampler.ts b/packages/opentelemetry-tracing/test/common/export/TestRecordOnlySampler.ts new file mode 100644 index 0000000000..c456780e86 --- /dev/null +++ b/packages/opentelemetry-tracing/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'; + } +} From 645a887945b8ce6daa207ff7ea3f3f499face22d Mon Sep 17 00:00:00 2001 From: Clark Jacobsohn Date: Wed, 4 Aug 2021 15:46:56 -0400 Subject: [PATCH 3/3] test: add missing calls to processor.onStart --- .../test/common/export/BatchSpanProcessorBase.test.ts | 9 +++++++++ .../test/common/export/SimpleSpanProcessor.test.ts | 2 ++ 2 files changed, 11 insertions(+) diff --git a/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts b/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts index ba13641bfb..42a7aa3fd5 100644 --- a/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts +++ b/packages/opentelemetry-tracing/test/common/export/BatchSpanProcessorBase.test.ts @@ -131,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); @@ -145,6 +147,7 @@ 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); @@ -157,6 +160,7 @@ describe('BatchSpanProcessorBase', () => { const span = createUnsampledSpan(`${name}_0`); + processor.onStart(span); processor.onEnd(span); await processor.forceFlush(); @@ -177,6 +181,7 @@ describe('BatchSpanProcessorBase', () => { assert.strictEqual(exporter.getFinishedSpans().length, 0); } const span = createSampledSpan(`${name}_6`); + processor.onStart(span); processor.onEnd(span); setTimeout(async () => { @@ -194,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); } @@ -212,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); @@ -259,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-tracing/test/common/export/SimpleSpanProcessor.test.ts b/packages/opentelemetry-tracing/test/common/export/SimpleSpanProcessor.test.ts index a37ee12bd6..03d3bbc17e 100644 --- a/packages/opentelemetry-tracing/test/common/export/SimpleSpanProcessor.test.ts +++ b/packages/opentelemetry-tracing/test/common/export/SimpleSpanProcessor.test.ts @@ -118,6 +118,7 @@ describe('SimpleSpanProcessor', () => { spanContext, SpanKind.CLIENT ); + processor.onStart(span); sinon.stub(exporter, 'export').callsFake((_, callback) => { setTimeout(() => { @@ -195,6 +196,7 @@ describe('SimpleSpanProcessor', () => { SpanKind.CLIENT ); + processor.onStart(span); processor.onEnd(span); const exporterCreatedSpans = testTracingExporter.getExporterCreatedSpans();