Binary format implementation #613
Conversation
Signed-off-by: Colin Sullivan <colin@synadia.com>
Codecov Report
@@ Coverage Diff @@
## master #613 +/- ##
============================================
+ Coverage 89.29% 89.76% +0.46%
- Complexity 543 563 +20
============================================
Files 68 69 +1
Lines 1953 2061 +108
Branches 253 261 +8
============================================
+ Hits 1744 1850 +106
- Misses 131 132 +1
- Partials 78 79 +1
Continue to review full report at Codecov.
|
| // Java defaults to big endian (network order), but enforce it just | ||
| // in case the carrier set the wrong byte order before passing it in. | ||
| if (buf.order() != ByteOrder.BIG_ENDIAN) { | ||
| throw new IllegalStateException("Carrier byte order must be big endian."); |
There was a problem hiding this comment.
Out of curiosity: did you run into this in practice? This check seems strangely too specific :-)
There was a problem hiding this comment.
I didn't in this case, although did notice that ByteBuffer supports specifying byte order. I figured it'd be an extremely difficult issue to debug if someone someday implemented a carrier that set it to little endian, so figured this might save someone some time in the future. ;)
| long spanId = 5678L; | ||
|
|
||
| TestBinaryCarrier buffer = new TestBinaryCarrier(); | ||
| JaegerSpanContext spanContext = new JaegerSpanContext(0, traceIdLow, spanId, 0, (byte)0); |
There was a problem hiding this comment.
Do we have boundary tests here? If so, it would be nice to have them for the binary codec as well.
There was a problem hiding this comment.
Do you mean boundaries as in max values for trace ids, spanIds, etc? I didn't see any elsewhere (may have missed them), but can add them to the binary codec if you'd like. Do you need them for the PR?
There was a problem hiding this comment.
Don't worry about this test for now: if we get an ID which is bigger than the max value, it would also fail for other codecs. I was also a bit concerned about parsing failures (baggage keys without values, for instance) , but I believe this first implementation is already a great improvement.
| @@ -27,6 +27,7 @@ | |||
| import io.opentracing.propagation.TextMap; | |||
| import io.opentracing.propagation.TextMapAdapter; | |||
|
|
|||
| import java.nio.ByteBuffer; | |||
There was a problem hiding this comment.
Perhaps a left-over from a local test?
There was a problem hiding this comment.
Ah, it is, I'll remove it. Thanks!
Signed-off-by: Colin Sullivan <colin@synadia.com>
|
@jpkrohling Would you have guidance on the test, or is this good to go? Thanks! |
|
Sounds good - thanks @jpkrohling! |
Hello from the NATS team!
We're adding a reference architecture for using the OpenTracing API over Jaeger in NATS. We've completed a NATS Open Tracing golang implementation, found here, and want to provide a java implementation as it's one of our more popular client languages and has been specifically requested. I noticed there was no binary format support, so added some here. It is wire compatible with the Jaeger golang client binary format.
I'd be happy to discuss on a call, zoom, or google hangouts if that would help.
Signed-off-by: Colin Sullivan colin@synadia.com
Which problem is this PR solving?
Short description of the changes