Update to Brave 5 #29

devinsba · 2019-02-25T16:28:37Z

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

codefromthecrypt

made some comments, not all of which need to be addressed immediately

codefromthecrypt · 2019-02-27T00:43:34Z

...ave-asynchttpclient-1.9.x/src/main/java/smartthings/brave/asynchttpclient/ClientTracing.java

@@ -222,7 +222,7 @@

 private static final class HttpAdapter extends HttpClientAdapter<Request, HttpResponseStatus> {

- @Override public boolean parseServerAddress(Request req, Endpoint.Builder builder) {
+ @Override public boolean parseServerIpAndPort(Request req, Endpoint.Builder builder) {


you can remove this method entirely I think. then separately parse the IP/port like this https://github.com/openzipkin/brave/blob/master/instrumentation/httpasyncclient/src/main/java/brave/httpasyncclient/TracingHttpAsyncClientBuilder.java#L86

codefromthecrypt · 2019-02-27T00:43:45Z

...brave-asynchttpclient-2.x/src/main/java/smartthings/brave/asynchttpclient/ClientTracing.java

@@ -154,7 +154,7 @@ private ClientTracing() {}

 private static final class HttpAdapter extends HttpClientAdapter<Request, HttpResponseStatus> {

- @Override public boolean parseServerAddress(Request req, Endpoint.Builder builder) {
+ @Override public boolean parseServerIpAndPort(Request req, Endpoint.Builder builder) {


same comment

codefromthecrypt · 2019-02-27T00:44:52Z

...p-common/src/test/java/smartthings/brave/propagation/SimpleB3ContextCarrierEncodingTest.java

@@ -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


is it zero is not valid? or an unnecessarily long string of zeros :)

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

codefromthecrypt · 2019-02-27T00:45:17Z

...p-common/src/test/java/smartthings/brave/propagation/SimpleB3ContextCarrierEncodingTest.java

@@ -156,7 +159,7 @@ public void injectingZeroFlagsShouldNotSetValues() {
 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


codefromthecrypt · 2019-02-27T00:45:48Z

...p-common/src/test/java/smartthings/brave/propagation/SimpleB3ContextCarrierEncodingTest.java

@@ -124,6 +125,7 @@ public void builderShouldCopyFrom() {
 assertThat(carrier2).isEqualTo(carrier);
 }

+ @Ignore
 @Test
 public void injectingFlagsShouldNotUnsetBuilderValues() {
 SimpleB3ContextCarrier carrier = SimpleB3ContextCarrier.newBuilder()


wonder if this type needs to still exist? (ok if it does for now, just curious)

codefromthecrypt · 2019-02-27T00:46:20Z

brave-kafka/pom.xml

@@ -57,7 +57,7 @@
 <artifactId>brave</artifactId>
 </dependency>
 <dependency>
- <groupId>io.zipkin.java</groupId>
+ <groupId>io.zipkin.zipkin2</groupId>


I don't think you'll need this dep except maybe in testing as we don't access endpoint directly anymore

codefromthecrypt · 2019-02-27T00:48:09Z

brave-kafka/src/main/java/smartthings/brave/kafka/consumers/BaseTracingConsumerInterceptor.java

@@ -74,11 +87,8 @@
 TracedConsumerRecord<K, byte[]> tracedConsumerRecord = getTracedConsumerRecord(record);
 TraceContextOrSamplingFlags traceContextOrSamplingFlags =
 tracedConsumerRecord.traceContextOrSamplingFlags;
- TraceContext ctx = traceContextOrSamplingFlags.context();


https://github.com/openzipkin/brave/blob/master/instrumentation/kafka-clients/src/main/java/brave/kafka/clients/KafkaTracing.java#L118

though this file itself possible is delete worthy, an aside to consider what needs to happen now vs later

codefromthecrypt · 2019-02-27T00:49:34Z

...kafka/src/main/java/smartthings/brave/kafka/consumers/DefaultTracingConsumerInterceptor.java

@@ -44,7 +57,7 @@
 if (envelope.hasParentId()) {
 builder.parentId(envelope.getParentId().getValue());
 }
- TraceContextOrSamplingFlags traceContextOrSamplingFlags = TraceContextOrSamplingFlags.create(builder);
+ TraceContextOrSamplingFlags traceContextOrSamplingFlags = TraceContextOrSamplingFlags.create(builder.build());


make a single-message version of this eventually, if consumer interceptor are still needed.

https://github.com/openzipkin/brave/blob/master/instrumentation/kafka-clients/src/main/java/brave/kafka/clients/TracingConsumer.java#L60

codefromthecrypt · 2019-02-27T00:50:03Z

brave-sns/pom.xml

@@ -46,7 +46,7 @@
 <artifactId>brave</artifactId>
 </dependency>
 <dependency>
- <groupId>io.zipkin.java</groupId>
+ <groupId>io.zipkin.zipkin2</groupId>
 <artifactId>zipkin</artifactId>
 <optional>true</optional>


same comment on scope.. try to remove these or set as test dep

codefromthecrypt · 2019-02-27T00:50:40Z

brave-sqs/src/main/java/smartthings/brave/sqs/TracingAmazonSQSClient.java

- Span oneWay = withEndpoint((ctx != null)
- ? tracing.tracer().joinSpan(ctx)
- : tracing.tracer().newTrace(traceContextOrSamplingFlags.samplingFlags()));
+ Span oneWay;


one way isn't the better choice anymore. this pre-dated messaging model. not sure what's required to fix now vs later

devinsba · 2019-02-28T16:49:41Z

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 · 2019-03-01T01:06:56Z

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 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In brave-http-common/src/test/java/smartthings/brave/propagation/SimpleB3ContextCarrierEncodingTest.java <#29 (comment)> : > @@ -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 <#29 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61__Hn2YNICYAt7GS3HztJcqUp74Aks5vR_9XgaJpZM4bQQ6X> .

llinder · 2019-03-05T02:52:50Z

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

YasinK-IW · 2019-03-08T09:53:21Z

Would love to use this library once its all updated.

YasinK-IW · 2019-03-19T16:10:22Z

Any update on this?

codefromthecrypt · 2019-03-19T23:00:11Z

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

Update to brave 5 and fix most of the tests

6e4a73c

devinsba mentioned this pull request Feb 25, 2019

Not compatible with Brave 5.x #28

Open

codefromthecrypt reviewed Feb 27, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Brave 5 #29

Update to Brave 5 #29

devinsba commented Feb 25, 2019

codefromthecrypt left a comment

codefromthecrypt Feb 27, 2019

codefromthecrypt Feb 27, 2019

codefromthecrypt Feb 27, 2019

devinsba Feb 28, 2019

codefromthecrypt Feb 27, 2019

codefromthecrypt Feb 27, 2019

codefromthecrypt Feb 27, 2019

codefromthecrypt Feb 27, 2019

codefromthecrypt Feb 27, 2019

codefromthecrypt Feb 27, 2019

codefromthecrypt Feb 27, 2019

devinsba commented Feb 28, 2019

codefromthecrypt commented Mar 1, 2019 via email

llinder commented Mar 5, 2019

YasinK-IW commented Mar 8, 2019

YasinK-IW commented Mar 19, 2019

codefromthecrypt commented Mar 19, 2019

Update to Brave 5 #29

Are you sure you want to change the base?

Update to Brave 5 #29

Conversation

devinsba commented Feb 25, 2019

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinsba commented Feb 28, 2019

codefromthecrypt commented Mar 1, 2019 via email

llinder commented Mar 5, 2019

YasinK-IW commented Mar 8, 2019

YasinK-IW commented Mar 19, 2019

codefromthecrypt commented Mar 19, 2019