search

SmartThingsOSS / smartthings-brave

Experimental extensions for the Open Zipkin Brave library
9 stars 8 forks source link

Update to Brave 5 #29

Open devinsba opened 5 years ago

devinsba commented 5 years ago

I've started the process toward Brave 5, I need to fix a couple more things in the tests. I may need some guidance from @adriancole on the HTTP ones, I'm not entirely sure how to convert those to be more idiomatic so that they pass the tests. I put notes and @Ignore on all the tests that didn't pass so that I could work through all the modules. I don't intend for this to be merged before all the @Ignore's are removed

devinsba commented 5 years ago

I'm not sure I have the time to take this over the finish line right now. I ended up using an approach based around RequestHander2, influenced by this code, in our code base which I plan to open source as soon as I get a free hour but that is questionable unless I can find some time over the weekend

codefromthecrypt commented 5 years ago

I see anyways flags zero is invalid. we only actually encode when context.debug is true. iotw this test isnt testing something we would emit.

On Fri, Mar 1, 2019, 12:09 AM Brian Devins-Suresh notifications@github.com wrote:

@devinsba commented on this pull request.

In brave-http-common/src/test/java/smartthings/brave/propagation/SimpleB3ContextCarrierEncodingTest.java https://github.com/SmartThingsOSS/smartthings-brave/pull/29#discussion_r261268153 :

@@ -135,7 +137,7 @@ public void injectingFlagsShouldNotUnsetBuilderValues() { SimpleB3ContextCarrier.Setter setter = new SimpleB3ContextCarrier.Setter(); setter.put(carrier, SimpleB3ContextCarrier.TRACE_ID_NAME, HexCodec.toLowerHex(1234)); setter.put(carrier, SimpleB3ContextCarrier.SPAN_ID_NAME, HexCodec.toLowerHex(9012));

  • setter.put(carrier, SimpleB3ContextCarrier.FLAGS_NAME, "0000000000000000");
  • setter.put(carrier, SimpleB3ContextCarrier.FLAGS_NAME, "0000000000000000"); // TODO 0 is not a valid hex anymore

The brave HexCodec class throws a NumberFormatException if you try to encode hex that ends up equaling 0

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SmartThingsOSS/smartthings-brave/pull/29#discussion_r261268153, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD61__Hn2YNICYAt7GS3HztJcqUp74Aks5vR_9XgaJpZM4bQQ6X .

llinder commented 5 years ago

Really appreciate the attempt to upgrade things. I will take a look through this ASAP and see where I can help out.

YasinK-IW commented 5 years ago

Would love to use this library once its all updated.

YasinK-IW commented 5 years ago

Any update on this?

codefromthecrypt commented 5 years ago

@YasinK-IW do you have time to pick this up and address the comments?