Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Negative hex spanIds #39

Closed
henrikno opened this issue Oct 7, 2014 · 5 comments · Fixed by #49
Closed

Negative hex spanIds #39

henrikno opened this issue Oct 7, 2014 · 5 comments · Fixed by #49

Comments

@henrikno
Copy link
Contributor

henrikno commented Oct 7, 2014

Currently you can get negative span ids like this: -140dcf65fae75f53
I don't think this is compatible with zipkin, e.g. if a zipkin/finagle client calls a brave service (tell me if I'm wrong here).

I think we should use Long.toHexString when we serialize span/traceIds, and Long.parseUnsignedLong when deserializing them, or some similar method.
See how finagle generates ids:
https://github.com/twitter/finagle/blob/master/finagle-core/src/main/scala/com/twitter/finagle/tracing/Id.scala

@joeslice
Copy link
Contributor

joeslice commented Oct 8, 2014

I believe this was fixed by @srapp with #28, released in 2.2.1. Are you using a version prior to that?

@kristofa
Copy link
Member

Hi @henrikno. There indeed was a fix as @joeslice mentions but I I guess it would be good if we would write a test that verifies and confirms if we still have an incompatibility between Finagle and Brave? Could you do that? Thanks!

@kristofa
Copy link
Member

@henrikno you are right. I wrote a test and we are incompatible with zipkin / finagle when negative span ids are generated. I'll look into you suggestions.

@kristofa
Copy link
Member

I can fix it by using following guava parsing/conversion utilities (part of com.google.common.primitives package):

From Long to String: UnsignedLongs.toString(spanId, 16)
From String to Long: UnsignedLongs.parseUnsignedLong(stringSpanId, 16)

java.lang.Long.toHexString(spanId) for converting to String also works but I prefer to stick to Guava for both conversions.

This makes it compatible with Zipkin/Finagle. I tested with positive and negative long values.
I'll fix this and close this issue when it is done.

Apparently this bug is fixed in Java 8.

@kristofa
Copy link
Member

This issue is fixed as part of brave 2.4 which is just released and should become available through Maven Central in a few hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants