Update to Brave 5 by devinsba · Pull Request #29 · SmartThingsOSS/smartthings-brave

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

  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

  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

    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

    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

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

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

        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

        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

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

Comments

Conversation

devinsba commented Feb 25, 2019

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devinsba commented Feb 28, 2019

Uh oh!

codefromthecrypt commented Mar 1, 2019 via email

Uh oh!

llinder commented Mar 5, 2019

Uh oh!

YasinK-IW commented Mar 8, 2019

Uh oh!

YasinK-IW commented Mar 19, 2019

Uh oh!

codefromthecrypt commented Mar 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants