Updates to latest zipkin and switches to 128-bit trace IDs #10
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
| // zipkin conditionally encodes its id as 16 chars if the high bits are zero | ||
| // This unconditionally encodes as 32 chars as Stackdriver trace IDs are fixed length. | ||
| char[] result = new char[32]; | ||
| Util.writeHexLong(result, 0, zipkinSpan.traceIdHigh); |
There was a problem hiding this comment.
ps if you don't like this, I can switch back to the other way, np
There was a problem hiding this comment.
I'm OK with this if you're OK with this depending on zipkin.internal.Util.
We may want to have this write zipkinSpan.traceId for both the high and low bits if zipkinSpan.traceIdHigh isn't set. Some users call our API and order results by traceId to get a "random sample" of traces. Duplicating the 64-bit ID instead of padding the ID with zeros will remove some sampling bias for any projects that contain both Zipkin and Stackdriver traces. Do you think that would cause any problems?
There was a problem hiding this comment.
the only problem with duplicating or otherwise is that the id won't match what's put in the logging context. Interesting use case on the "random sample" this will clash on that, though curious why they don't shuffle instead? Is this tool OSS (ex can we open an issue to discuss that?)
There was a problem hiding this comment.
incidentally, we can always change this later.. I'll do this.. will revert to the original impl, but add a comment on why we are duplicating. Later it can be followed-up as necessary.
There was a problem hiding this comment.
No, it's not an OSS tool. However, it is a pretty small use case, and I'm not sure if there are many projects that will have a mix of Stackdriver and Zipkin traces. The mismatch with the logging context may be a bigger issue if you think Zipkin users reference that frequently. We can pad with 0's if you think that would be better. Anyway, this problem won't exist once Zipkin trace ID's are 128-bit. :)
There was a problem hiding this comment.
However, if you do pad with 0's, TraceTranslatorTest:testTranslateTrace needs to be updated.
| @@ -41,6 +41,10 @@ | |||
| <tag>HEAD</tag> | |||
| </scm> | |||
|
|
|||
| <properties> | |||
| <zipkin.version>1.18.0</zipkin.version> | |||
There was a problem hiding this comment.
also ps 1.18.0 is syncing to maven central, and not actually needed. probably good to use it (or 1.17.1) though
This changes to the latest version of Zipkin and adjusts the code to use 128-bit trace ids as defined by Zipkin. This fills high bits with zero when the trace doesn't have high bits set. This would occur when 64-bit trace IDs are in use.
|
PTAL I think all good now?
|
|
CLAs look good, thanks! |
|
made some notes on the next commit. PS most major tracers are 128-bit now,
just that they don't issue them by default
openzipkin/b3-propagation#6
|
|
padding 0s left would be better I think.. it switches the explanation from
why the trace id doesn't match to why lex sorting on trace id doesn't end
up with random sample.
|
Doing so helps remove confusion with 64-bit trace ids, which are encoded by Zipkin with 16 characters (implicitly padded left with zeros).
|
okie split into two commits.. lemme know if you want one (or squash). ttfn!
|
This changes to the latest version of Zipkin and adjusts the code to use
128-bit trace ids as defined by Zipkin. This fills high bits with zero
when the trace doesn't have high bits set. This would occur when 64-bit
trace IDs are in use.