Unexpected handling of binary annotations containing signed long values

[tristanpenman]

I noticed after upgrading from version 1.7 to 1.8 (and later 1.8.1) of Zipkin, that the recent Codec changes may have introduced a regression in the handling of binary annotations containing negative ints or longs.

I found it easy to reproduce using the TracingBasics demo in activator-akka-tracing which records the result of Random.nextLong() as a binary annotation.

After running this demo, attempting to 'Find Traces' using the UI results in an error similar to:

Error executing query: Expected -1005656679588439279 to be unsigned

I've been able to trace this back to the checkArgument call in Buffer.java:246

Admittedly, I don't understand Zipkin well enough to know whether this was/is now intended behaviour. If it is, I can raise an issue with the authors of the akka-tracing / activator-akka-tracing projects.

[codefromthecrypt]

this probably wasn't caught because people don't use Long annotations (as
they are unqueryable and likely to be removed in the future).

That said, happy to fix the regression.. will have a go shortly!

On Mon, Sep 5, 2016 at 10:48 AM, Tristan Penman [email protected]
wrote:

I noticed after upgrading from version 1.7 to 1.8 (and later 1.8.1) of
Zipkin, that the recent Codec changes may have introduced a regression in
the handling of binary annotations containing negative ints or longs.

I found it easy to reproduce using the TracingBasics demo in
activator-akka-tracing
https://github.com/levkhomich/activator-akka-tracing which records the
result of Random.nextLong() as a binary annotation.

After running this demo, attempting to 'Find Traces' using the UI results
in an error similar to:

Error executing query: Expected -1005656679588439279 to be unsigned

I've been able to trace this back to the checkArgument call in
Buffer.java:246

Admittedly, I don't understand Zipkin well enough to know whether this
was/is now intended behaviour. If it is, I can raise an issue with the
authors of the akka-tracing / activator-akka-tracing projects.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1268, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD613oKg43fHfDA3FQkSCqB-08qW1l9ks5qm4MVgaJpZM4J0s4i
.

[codefromthecrypt]

in general, tracers toString inputs like numbers, which allows them to be
queryable.

here's the model v2 thing which hopefully will remove the niche numeric
types like i16 #939

On Mon, Sep 5, 2016 at 10:51 AM, Adrian Cole [email protected]
wrote:

this probably wasn't caught because people don't use Long annotations (as
they are unqueryable and likely to be removed in the future).

That said, happy to fix the regression.. will have a go shortly!

On Mon, Sep 5, 2016 at 10:48 AM, Tristan Penman [email protected]
wrote:

I noticed after upgrading from version 1.7 to 1.8 (and later 1.8.1) of
Zipkin, that the recent Codec changes may have introduced a regression in
the handling of binary annotations containing negative ints or longs.

I found it easy to reproduce using the TracingBasics demo in
activator-akka-tracing
https://github.com/levkhomich/activator-akka-tracing which records the
result of Random.nextLong() as a binary annotation.

After running this demo, attempting to 'Find Traces' using the UI results
in an error similar to:

Error executing query: Expected -1005656679588439279 to be unsigned

I've been able to trace this back to the checkArgument call in
Buffer.java:246

Admittedly, I don't understand Zipkin well enough to know whether this
was/is now intended behaviour. If it is, I can raise an issue with the
authors of the akka-tracing / activator-akka-tracing projects.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1268, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD613oKg43fHfDA3FQkSCqB-08qW1l9ks5qm4MVgaJpZM4J0s4i
.

[tristanpenman]

You might have set a record for fastest issue response there. Thanks for looking into it!

So it sounds like the right thing for me to do here would be to raise issues (or submit PRs) for the projects that I'm using, since string values are clearly the direction forward.

[codefromthecrypt]

oh interesting.. I think that there's an issue with B3 in that demo. Ex.
trace ids are being toString'ed as opposed to lower-hex.

you can copy/paste stuff from here until we have extracted a B3 library

https://github.com/uber/jaeger-client-java/blob/master/jaeger-zipkin/src/main/java/com/uber/jaeger/propagation/b3/B3TextMapCodec.java

On Mon, Sep 5, 2016 at 10:57 AM, Tristan Penman [email protected]
wrote:

You might have set a record for fastest issue response there. Thanks for
looking into it!

So it sounds like the right thing for me to do here would be to raise
issues (or submit PRs) for the projects that I'm using, since string values
are clearly the direction forward.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1268 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD61_QMilNmAIXpKu2oRyMFfxAaxlv9ks5qm4UpgaJpZM4J0s4i
.

[codefromthecrypt]

I can't find that line number (with checkArgument), but I can add some
tests to flush out a bug hopefully.

On Mon, Sep 5, 2016 at 10:48 AM, Tristan Penman [email protected]
wrote:

I noticed after upgrading from version 1.7 to 1.8 (and later 1.8.1) of
Zipkin, that the recent Codec changes may have introduced a regression in
the handling of binary annotations containing negative ints or longs.

I found it easy to reproduce using the TracingBasics demo in
activator-akka-tracing
https://github.com/levkhomich/activator-akka-tracing which records the
result of Random.nextLong() as a binary annotation.

After running this demo, attempting to 'Find Traces' using the UI results
in an error similar to:

Error executing query: Expected -1005656679588439279 to be unsigned

I've been able to trace this back to the checkArgument call in
Buffer.java:246

Admittedly, I don't understand Zipkin well enough to know whether this
was/is now intended behaviour. If it is, I can raise an issue with the
authors of the akka-tracing / activator-akka-tracing projects.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1268, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD613oKg43fHfDA3FQkSCqB-08qW1l9ks5qm4MVgaJpZM4J0s4i
.

[tristanpenman]

It might help if I paste the code block and path.

/zipkin/zipkin/src/main/java/zipkin/internal/Buffer.java:

Buffer writeAscii(long v) {
    if (v == 0) return writeByte('0');
    checkArgument(v > 0, "Expected %s to be unsigned", v);
    int width = asciiSizeInBytes(v);
    int pos = this.pos += width; // We write backwards from right to left.
    while (v != 0) {
      int digit = (int) (v % 10);
      buf[--pos] = HEX_DIGITS[digit];
      v /= 10;
    }
    return this;
  }

[codefromthecrypt]

just found it, but thanks for the assist!!

[codefromthecrypt]

I was on a wrong branch :)

[codefromthecrypt]

gonna take me an hour (developer hour.. so maybe a couple :) ) to fix
this, as I intentionally special-cased and forgot that this function
was used for binary annotations. stand by!

On Mon, Sep 5, 2016 at 11:15 AM, Adrian Cole [email protected] wrote:

I was on a wrong branch :)

[tristanpenman]

No problem at all. This response has already exceeded all of my expectations! 👍

[tristanpenman]

I'm pleased to say that your fix in commit 3b8d56a has resolved the issue.

Thanks again for your help!

[codefromthecrypt]

Thanks for your help identifying this! 1.8.2 will be out shortly