Add option for consistent high trace ID #219
Conversation
Signed-off-by: Isaac Hier <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 81.92% 81.99% +0.06%
==========================================
Files 51 51
Lines 3214 3226 +12
==========================================
+ Hits 2633 2645 +12
Misses 457 457
Partials 124 124
Continue to review full report at Codecov.
|
|
lgtm, but I'm a bit hesitant on landing this since it's for a very specific Uber use case. Let's keep this branch open and pin this branch for now. |
|
Sounds good to me |
|
Lol the sampling part is true for the lower 64 bits. Regardless that is a good point about overriding the random number generator. |
Signed-off-by: Isaac Hier <[email protected]>
isaachier
force-pushed
the
consistent-high-trace-id
branch
from
61a4729 to
65e5b85
Compare
tracer.go
Outdated
| @@ -61,6 +62,7 @@ type Tracer struct { | |||
|
|
|||
| baggageRestrictionManager baggage.RestrictionManager | |||
| baggageSetter *baggageSetter | |||
| serviceNameHash uint64 | |||
There was a problem hiding this comment.
don't think the tracer needs to be aware of this
There was a problem hiding this comment.
Oh missed this thanks
tracer.go
Outdated
| @@ -206,6 +208,8 @@ func (t *Tracer) startSpanWithOptions( | |||
| ctx.traceID.Low = t.randomID() | |||
| if t.options.gen128Bit { | |||
| ctx.traceID.High = t.randomID() | |||
| } else if t.options.highTraceIDGenerator != nil { | |||
There was a problem hiding this comment.
I don't think else if is appropriate - if we're not asked to generate 128bit IDs, then there's no need to invoke High ID generator even if it is provided. On the other hand, if gen128Bit == true but the external generator is not provided, it needs to be defaulted to tracer.randomID.
I would also return an error from constructor if gen128Bit==false && highTraceIDGenerator != nil
There was a problem hiding this comment.
Fair enough. I think I just didn't think about the change in logic after changing the option to a function. This was originally checking the ConsistentHighTraceID flag.
tracer_test.go
Outdated
| @@ -317,6 +319,30 @@ func TestGen128Bit(t *testing.T) { | |||
| assert.True(t, traceID.Low != 0) | |||
| } | |||
|
|
|||
| func TestHighTraceIDGenerator(t *testing.T) { | |||
| hash := sha256.Sum256([]byte("x")) | |||
| id := binary.BigEndian.Uint64(hash[0:]) | |||
There was a problem hiding this comment.
don't see the point of these manipulations - why not just define some constant and check against it? Also, if the default is random generator, then there is a chance it will produce the same constant, so the test must be checking that the generator was actually called, not just that the resulting HighID is equal to the constant.
There was a problem hiding this comment.
Good idea. Although the likelihood of the random generator coming up with the same value is effectively zero, still worth doing the ideal way. This was just where I temporarily moved the service name to hash transformation.
Signed-off-by: Isaac Hier <[email protected]>
tracer.go
Outdated
| @@ -205,7 +212,11 @@ func (t *Tracer) startSpanWithOptions( | |||
| newTrace = true | |||
| ctx.traceID.Low = t.randomID() | |||
| if t.options.gen128Bit { | |||
| ctx.traceID.High = t.randomID() | |||
| if t.options.highTraceIDGenerator != nil { | |||
There was a problem hiding this comment.
the branching is not necessary, you can default highTraceIDGenerator to randomID() in the constructor
tracer_test.go
Outdated
| assert.True(t, calledGenerator) | ||
| } | ||
|
|
||
| type mockLogger struct { |
There was a problem hiding this comment.
shouldn't do that. We have log/zap package where you can create a logger with zap, and zap has a ByteBuffer-backed logger. To make using it easier, however, we can add log/testutils package that will do the above in one call.
There was a problem hiding this comment.
incidentally, we have something like that in the main repo, jaeger/pkg/testutils/logger.go, another option would be to consolidate that in jaeger-lib (but I'd punt on that for now)
There was a problem hiding this comment.
When I try using a the log/zap package in my test case, I get this error:
# github.com/uber/jaeger-client-go
import cycle not allowed in test
package github.com/uber/jaeger-client-go (test)
imports github.com/uber/jaeger-client-go/log/zap
imports github.com/uber/jaeger-client-go
Imagine this needs to be moved to jaeger-lib or otherwise refactored (the issue is log/zap/field.go not log/zap/logger.go).
There was a problem hiding this comment.
Should I just copy the struct from log/zap/logger.go for now or should I refactor testutils into jaeger-lib as you mentioned above?
There was a problem hiding this comment.
just leave it for now, add a TODO
Signed-off-by: Isaac Hier <[email protected]>
isaachier
force-pushed
the
consistent-high-trace-id
branch
from
c1a92a4 to
0e4fac0
Compare
Signed-off-by: Isaac Hier <[email protected]>
Signed-off-by: Isaac Hier <[email protected]>
Allow high 64 bits of trace ID to be set to the first 64 bits of SHA256 of service name. This is a quick way to identify the originator of a given trace.