Add "sampleWithoutDebugFlag" option for tracer to decide set/not set …

…debug span when a span is forced to be sampled (jaegertracing#423) Signed-off-by: jung <jung@uber.com>
Clarilab · Sep 6, 2019 · 3434ad5 · 3434ad5
1 parent a359b63
commit 3434ad5
Show file tree

Hide file tree

Showing 9 changed files with 128 additions and 36 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {