Add a flag for firehose mode #419
Conversation
Add 0b100 as a constant for Firehose mode and allow setting and checking this on the span context. fixes jaegertracing#417 ref #1731 Signed-off-by: Prithvi Raj <p.r@uber.com>
context.go
Outdated
| flagDebug = byte(2) | ||
| flagSampled = byte(1) | ||
| flagDebug = byte(2) | ||
| flagFirehose = byte(4) |
There was a problem hiding this comment.
Maybe we could define is as the 4th bit (byte(8)), not the 3rd. Zipkin supports "undecided" sampling decision, which we might need to implement in the future (for edge proxy use cases), and it's nicer to group sampling bits together.
There was a problem hiding this comment.
Also, should we add a bit of docs here to what firehose means?
There was a problem hiding this comment.
Yes, we can use the fourth bit.
I think we should add docs on a centralized place like the website would be better, or we would be repeating the semantic meaning in every client. (I don't think we have docs for debug flag in the clients either).
span.go
Outdated
| @@ -318,3 +318,8 @@ func setSamplingPriority(s *Span, value interface{}) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // SetFirehose sets the firehose tag on the span context | |||
| func SetFirehose(s *Span) { | |||
There was a problem hiding this comment.
EnableFirehose might be better name, as set() usually implies a value argument
span_test.go
Outdated
| @@ -126,6 +126,19 @@ func TestSetTag_SamplingPriority(t *testing.T) { | |||
| assert.False(t, sp1.context.IsDebug(), "debug should not be allowed by the throttler") | |||
| } | |||
|
|
|||
| func TestSetFirehoseMode(t *testing.T) { | |||
| tracer, closer := NewTracer("DOOP", NewConstSampler(true), NewNullReporter(), | |||
| TracerOptions.DebugThrottler(throttler.DefaultThrottler{})) | |||
There was a problem hiding this comment.
throttler seems redundant here
Signed-off-by: Prithvi Raj <p.r@uber.com>
span.go
Outdated
| @@ -318,3 +318,8 @@ func setSamplingPriority(s *Span, value interface{}) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // EnableFirehose sets the firehose tag on the span context | |||
There was a problem hiding this comment.
// EnableFirehose enables firehose mode on the span context
Signed-off-by: Prithvi Raj <p.r@uber.com>
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
==========================================
+ Coverage 88.44% 88.46% +0.01%
==========================================
Files 55 55
Lines 3107 3111 +4
==========================================
+ Hits 2748 2752 +4
Misses 255 255
Partials 104 104
Continue to review full report at Codecov.
|
Add
0b100as a constant for Firehose mode and allow setting and checkingthis on the span context.
fixes #417
ref jaegertracing/jaeger#1731
Signed-off-by: Prithvi Raj p.r@uber.com