From 3434ad534caed2556fb03ff2c666551ddc075360 Mon Sep 17 00:00:00 2001 From: Jun Guo Date: Fri, 6 Sep 2019 00:08:13 -0400 Subject: [PATCH] Add "sampleWithoutDebugFlag" option for tracer to decide set/not set debug span when a span is forced to be sampled (#423) Signed-off-by: jung --- config/config.go | 1 + config/config_test.go | 13 ++++++ config/options.go | 35 +++++++++++------ config/options_test.go | 2 + span.go | 7 +++- span_test.go | 89 +++++++++++++++++++++++++++++++++--------- tracer.go | 9 +++-- tracer_options.go | 6 +++ tracer_test.go | 2 + 9 files changed, 128 insertions(+), 36 deletions(-) diff --git a/config/config.go b/config/config.go index eb514553..6bce1b3b 100644 --- a/config/config.go +++ b/config/config.go @@ -235,6 +235,7 @@ func (c Configuration) NewTracer(options ...Option) (opentracing.Tracer, io.Clos jaeger.TracerOptions.PoolSpans(opts.poolSpans), jaeger.TracerOptions.ZipkinSharedRPCSpan(opts.zipkinSharedRPCSpan), jaeger.TracerOptions.MaxTagValueLength(opts.maxTagValueLength), + jaeger.TracerOptions.NoDebugFlagOnForcedSampling(opts.noDebugFlagOnForcedSampling), } for _, tag := range opts.tags { diff --git a/config/config_test.go b/config/config_test.go index 2e0c286f..d7527216 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -550,6 +550,19 @@ func TestNewTracer(t *testing.T) { assert.NoError(t, err) } +func TestNewTracerWithNoDebugFlagOnForcedSampling(t *testing.T) { + cfg := &Configuration{ServiceName: "my-service"} + tracer, closer, err := cfg.NewTracer(Metrics(metrics.NullFactory), Logger(log.NullLogger), NoDebugFlagOnForcedSampling(true)) + defer closer.Close() + + span := tracer.StartSpan("testSpan").(*jaeger.Span) + ext.SamplingPriority.Set(span, 1) + + assert.NoError(t, err) + assert.False(t, span.SpanContext().IsDebug()) + assert.True(t, span.SpanContext().IsSampled()) +} + func TestNewTracerWithoutServiceName(t *testing.T) { cfg := &Configuration{} _, _, err := cfg.NewTracer(Metrics(metrics.NullFactory), Logger(log.NullLogger)) diff --git a/config/options.go b/config/options.go index 322691be..e0e50e83 100644 --- a/config/options.go +++ b/config/options.go @@ -26,19 +26,20 @@ type Option func(c *Options) // Options control behavior of the client. type Options struct { - metrics metrics.Factory - logger jaeger.Logger - reporter jaeger.Reporter - sampler jaeger.Sampler - contribObservers []jaeger.ContribObserver - observers []jaeger.Observer - gen128Bit bool - poolSpans bool - zipkinSharedRPCSpan bool - maxTagValueLength int - tags []opentracing.Tag - injectors map[interface{}]jaeger.Injector - extractors map[interface{}]jaeger.Extractor + metrics metrics.Factory + logger jaeger.Logger + reporter jaeger.Reporter + sampler jaeger.Sampler + contribObservers []jaeger.ContribObserver + observers []jaeger.Observer + gen128Bit bool + poolSpans bool + zipkinSharedRPCSpan bool + maxTagValueLength int + noDebugFlagOnForcedSampling bool + tags []opentracing.Tag + injectors map[interface{}]jaeger.Injector + extractors map[interface{}]jaeger.Extractor } // Metrics creates an Option that initializes Metrics in the tracer, @@ -117,6 +118,14 @@ func MaxTagValueLength(maxTagValueLength int) Option { } } +// NoDebugFlagOnForcedSampling can be used to decide whether debug flag will be set or not +// when calling span.setSamplingPriority to force sample a span. +func NoDebugFlagOnForcedSampling(noDebugFlagOnForcedSampling bool) Option { + return func(c *Options) { + c.noDebugFlagOnForcedSampling = noDebugFlagOnForcedSampling + } +} + // Tag creates an option that adds a tracer-level tag. func Tag(key string, value interface{}) Option { return func(c *Options) { diff --git a/config/options_test.go b/config/options_test.go index b20a774c..62207c6b 100644 --- a/config/options_test.go +++ b/config/options_test.go @@ -41,6 +41,7 @@ func TestApplyOptions(t *testing.T) { PoolSpans(true), ZipkinSharedRPCSpan(true), MaxTagValueLength(1024), + NoDebugFlagOnForcedSampling(true), ) assert.Equal(t, jaeger.StdLogger, opts.logger) assert.Equal(t, sampler, opts.sampler) @@ -50,6 +51,7 @@ func TestApplyOptions(t *testing.T) { assert.True(t, opts.gen128Bit) assert.True(t, opts.poolSpans) assert.True(t, opts.zipkinSharedRPCSpan) + assert.True(t, opts.noDebugFlagOnForcedSampling) assert.Equal(t, 1024, opts.maxTagValueLength) } diff --git a/span.go b/span.go index d4669fc3..9df8b601 100644 --- a/span.go +++ b/span.go @@ -312,7 +312,10 @@ func setSamplingPriority(s *Span, value interface{}) bool { s.context.flags = s.context.flags & (^flagSampled) return true } - if s.tracer.isDebugAllowed(s.operationName) { + if s.tracer.options.noDebugFlagOnForcedSampling { + s.context.flags = s.context.flags | flagSampled + return true + } else if s.tracer.isDebugAllowed(s.operationName) { s.context.flags = s.context.flags | flagDebug | flagSampled return true } @@ -321,5 +324,7 @@ func setSamplingPriority(s *Span, value interface{}) bool { // EnableFirehose enables firehose flag on the span context func EnableFirehose(s *Span) { + s.Lock() + defer s.Unlock() s.context.flags |= flagFirehose } diff --git a/span_test.go b/span_test.go index d7bf9322..cb57dade 100644 --- a/span_test.go +++ b/span_test.go @@ -19,7 +19,7 @@ import ( "sync/atomic" "testing" - opentracing "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go" "github.com/opentracing/opentracing-go/ext" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -105,25 +105,78 @@ func TestSpanOperationName(t *testing.T) { } func TestSetTag_SamplingPriority(t *testing.T) { - tracer, closer := NewTracer("DOOP", NewConstSampler(true), NewNullReporter(), - TracerOptions.DebugThrottler(throttler.DefaultThrottler{})) - - sp1 := tracer.StartSpan("s1").(*Span) - ext.SamplingPriority.Set(sp1, 0) - assert.False(t, sp1.context.IsDebug()) - - ext.SamplingPriority.Set(sp1, 1) - assert.True(t, sp1.context.IsDebug()) - assert.NotNil(t, findDomainTag(sp1, "sampling.priority"), "sampling.priority tag should be added") - closer.Close() + type testCase struct { + noDebugFlagOnForcedSampling bool + throttler throttler.Throttler + samplingPriority uint16 + expDebugSpan bool + expSamplingTag bool + } - tracer, closer = NewTracer("DOOP", NewConstSampler(true), NewNullReporter(), - TracerOptions.DebugThrottler(testThrottler{allowAll: false})) - defer closer.Close() + testSuite := map[string]testCase{ + "Sampling priority 0 with debug flag and default throttler": { + noDebugFlagOnForcedSampling: false, + throttler: throttler.DefaultThrottler{}, + samplingPriority: 0, + expDebugSpan: false, + expSamplingTag: false, + }, + "Sampling priority 1 with debug flag and default throttler": { + noDebugFlagOnForcedSampling: false, + throttler: throttler.DefaultThrottler{}, + samplingPriority: 1, + expDebugSpan: true, + expSamplingTag: true, + }, + "Sampling priority 1 with debug flag and test throttler": { + noDebugFlagOnForcedSampling: false, + throttler: testThrottler{allowAll: false}, + samplingPriority: 1, + expDebugSpan: false, + expSamplingTag: false, + }, + "Sampling priority 0 without debug flag and default throttler": { + noDebugFlagOnForcedSampling: true, + throttler: throttler.DefaultThrottler{}, + samplingPriority: 0, + expDebugSpan: false, + expSamplingTag: false, + }, + "Sampling priority 1 without debug flag and default throttler": { + noDebugFlagOnForcedSampling: true, + throttler: throttler.DefaultThrottler{}, + samplingPriority: 1, + expDebugSpan: false, + expSamplingTag: true, + }, + "Sampling priority 1 without debug flag and test throttler": { + noDebugFlagOnForcedSampling: true, + throttler: testThrottler{allowAll: false}, + samplingPriority: 1, + expDebugSpan: false, + expSamplingTag: true, + }, + } - sp1 = tracer.StartSpan("s1").(*Span) - ext.SamplingPriority.Set(sp1, 1) - assert.False(t, sp1.context.IsDebug(), "debug should not be allowed by the throttler") + for name, testCase := range testSuite { + t.Run(name, func(t *testing.T) { + tracer, closer := NewTracer( + "DOOP", + NewConstSampler(true), + NewNullReporter(), + TracerOptions.DebugThrottler(testCase.throttler), + TracerOptions.NoDebugFlagOnForcedSampling(testCase.noDebugFlagOnForcedSampling), + ) + + sp1 := tracer.StartSpan("s1").(*Span) + ext.SamplingPriority.Set(sp1, testCase.samplingPriority) + assert.Equal(t, testCase.expDebugSpan, sp1.context.IsDebug()) + if testCase.expSamplingTag { + assert.NotNil(t, findDomainTag(sp1, "sampling.priority"), "sampling.priority tag should be added") + } + closer.Close() + }) + } } func TestSetFirehoseMode(t *testing.T) { diff --git a/tracer.go b/tracer.go index a4218bc4..745a0c38 100644 --- a/tracer.go +++ b/tracer.go @@ -47,10 +47,11 @@ type Tracer struct { randomNumber func() uint64 options struct { - gen128Bit bool // whether to generate 128bit trace IDs - zipkinSharedRPCSpan bool - highTraceIDGenerator func() uint64 // custom high trace ID generator - maxTagValueLength int + gen128Bit bool // whether to generate 128bit trace IDs + zipkinSharedRPCSpan bool + highTraceIDGenerator func() uint64 // custom high trace ID generator + maxTagValueLength int + noDebugFlagOnForcedSampling bool // more options to come } // allocator of Span objects diff --git a/tracer_options.go b/tracer_options.go index ecb17276..469685bb 100644 --- a/tracer_options.go +++ b/tracer_options.go @@ -126,6 +126,12 @@ func (tracerOptions) Gen128Bit(gen128Bit bool) TracerOption { } } +func (tracerOptions) NoDebugFlagOnForcedSampling(noDebugFlagOnForcedSampling bool) TracerOption { + return func(tracer *Tracer) { + tracer.options.noDebugFlagOnForcedSampling = noDebugFlagOnForcedSampling + } +} + func (tracerOptions) HighTraceIDGenerator(highTraceIDGenerator func() uint64) TracerOption { return func(tracer *Tracer) { tracer.options.highTraceIDGenerator = highTraceIDGenerator diff --git a/tracer_test.go b/tracer_test.go index d709c1e1..e31f4691 100644 --- a/tracer_test.go +++ b/tracer_test.go @@ -278,6 +278,7 @@ func TestTracerOptions(t *testing.T) { TracerOptions.RandomNumber(rnd), TracerOptions.PoolSpans(true), TracerOptions.Tag("tag_key", "tag_value"), + TracerOptions.NoDebugFlagOnForcedSampling(true), ) defer closer.Close() @@ -289,6 +290,7 @@ func TestTracerOptions(t *testing.T) { assert.Equal(t, uint64(1), tracer.randomNumber()) // always 1 assert.Equal(t, true, isPoolAllocator(tracer.spanAllocator)) assert.Equal(t, opentracing.Tag{Key: "tag_key", Value: "tag_value"}, tracer.Tags()[0]) + assert.True(t, tracer.options.noDebugFlagOnForcedSampling) } func TestInjectorExtractorOptions(t *testing.T) {