Implement Zipkin 2 JSON Sender #399

keitwb · 2018-04-17T15:30:20Z

The existing ZipkinSender class only supported the Thrift v1 protocol,
but the new Zipkin2Sender supports both v1 and v2 JSON. Otherwise,
functionality is mostly the same.

There is some conceptually duplicate code in the Sender classes due to
the use of different classes between Zipkin 1/2 but it seemed overly
complicated to adapt them to a common adapter interface.

Fixes #383.

codecov · 2018-04-17T15:37:10Z

Codecov Report

Merging #399 into master will decrease coverage by 0.01%.
The diff coverage is 84.21%.

@@             Coverage Diff             @@
##             master    #399      +/-   ##
===========================================
- Coverage     82.81%   82.8%   -0.02%     
- Complexity      490     512      +22     
===========================================
  Files            66      69       +3     
  Lines          2019    2105      +86     
  Branches        247     263      +16     
===========================================
+ Hits           1672    1743      +71     
- Misses          265     273       +8     
- Partials         82      89       +7

Impacted Files	Coverage Δ	Complexity Δ
.../io/jaegertracing/senders/zipkin/ZipkinSender.java	`67.74% <ø> (ø)`	`15 <0> (ø)`	⬇️
...gertracing/senders/zipkin/ThriftSpanConverter.java	`85.55% <100%> (-0.62%)`	`21 <0> (-5)`
...egertracing/zipkin/reporters/ZipkinV2Reporter.java	`71.42% <71.42%> (ø)`	`2 <2> (?)`
...in/java/io/jaegertracing/zipkin/ConverterUtil.java	`75% <75%> (ø)`	`5 <5> (?)`
.../java/io/jaegertracing/zipkin/V2SpanConverter.java	`84.81% <84.81%> (ø)`	`20 <20> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e237f8...b497c36. Read the comment docs.

mpetazzoni · 2018-04-17T20:53:26Z

jaeger-zipkin/src/main/java/io/jaegertracing/senders/zipkin/V2SpanConverter.java

+
+  public static zipkin2.Span convertSpan(Span span) {
+    Tracer tracer = span.getTracer();
+    Endpoint host = zipkin2.Endpoint.newBuilder()


To avoid confusion I would remove the import zipkin2.Endpoint and use zipkin2.Endpoint host = ... here.

mpetazzoni · 2018-04-17T20:54:35Z

jaeger-zipkin/src/main/java/io/jaegertracing/senders/zipkin/ConverterUtil.java

+
+  static boolean isRpc(Span span) {
+    Object spanKindValue = span.getTags().get(Tags.SPAN_KIND.getKey());
+    return Tags.SPAN_KIND_CLIENT.equals(spanKindValue) || Tags.SPAN_KIND_SERVER.equals(spanKindValue);


I know you're just moving code around here, but this could simply be return isRpcServer(span) || isRpcClient(span). Also clean up the superfluous empty newlines here and at the end of the class.

mpetazzoni · 2018-04-17T20:55:46Z

jaeger-zipkin/src/main/java/io/jaegertracing/senders/zipkin/V2SpanConverter.java

+    Tracer tracer = span.getTracer();
+    Endpoint host = zipkin2.Endpoint.newBuilder()
+        .ip(convertIp(tracer.getIpv4()))
+        .port(0)


You can probably just omit that. Might be good to add a comment saying that we don't have a local endpoint port to put in at all.

mpetazzoni · 2018-04-17T20:56:37Z

jaeger-zipkin/src/main/java/io/jaegertracing/senders/zipkin/V2SpanConverter.java

+  }
+
+  private static zipkin2.Span.Kind convertKind(Object kind) {
+    if (kind == null) {


If you reverse your equals calls you don't need that initial null check and have a single location where you return null from this method

mpetazzoni · 2018-04-17T20:57:42Z

jaeger-zipkin/src/main/java/io/jaegertracing/senders/zipkin/V2SpanConverter.java

+  }
+
+  private static InetAddress convertIp(Integer ip) {
+    byte[] bytes = ByteBuffer.allocate(4).putInt(ip).array();


Can ip be null here? If so, you probably need to handle that. If not, then you can change the method parameter to be the unboxed int

mpetazzoni

LGTM, but I'll of course defer to the maintainers here.

jpkrohling · 2018-04-30T12:31:55Z

@keitwb, thank you for this PR and sorry for the delay in reviewing it. I've been AFK for most of last week and will be for most of this week again.

Perhaps @pavolloffay would have some time this week to review, otherwise, I'll review it next week.

pavolloffay · 2018-05-02T12:10:03Z

Based on #383 (comment) I think it would be better to have an adapter for zipkin reporter. It's more flexible and allows us

use any zipkin supported sender: okhttp, urlconnection, kafka...
leverage zipkin reporters
less code to maintain?

keitwb · 2018-05-02T21:13:30Z

@pavolloffay I added an abstract base class for the two Zipkin senders so that there is quite a bit less duplication now. This always allowed using any kind of Zipkin sender (http clients, kafka, etc.), and I don't understand what you mean about a Zipkin reporter adapter.

pavolloffay · 2018-05-03T11:03:42Z

I mean instead of implementing sender interface e.g. ZipkinSender we could implement ZipkinReporter

class ZipkinReporter implements io.jaegertracing.Reporter {
   private io.zipkin.Reporter delegate;

   public ZipkinReporter(io.zipkin.Reporter reporter) {
     this.delegate = reporter;
   {

   public report(io.jaegertracing.Span jaegerSpan) {
       io.zipkin.Span ipkinSpan  = convertToZipkin(span);
       delegate.report(span);
   }
}

pavolloffay · 2018-05-03T12:16:06Z

Something like 431a558.

But note that it deals only with jsonv1 and jsonv2. It should be also possible to add thrift.

One disadvantage is that flush is not supporter, but we could bind to zipkin's AsyncReporter (instead to zipkin.Reporter) which implements flush.

keitwb · 2018-05-07T15:55:45Z

@pavolloffay ok, yes I have started reimplementing this using reporters and it seems much cleaner and makes much better reuse of zipkin code instead of reinventing the wheel in this lib. My main question at this point is whether there is anything special about the use of the older com.twitter.zipkin.thriftjava.Span class vs. just converting the jaeger spans to zipkin.Span and using the Thrift encoding from the Zipkin v1 reporter.

pavolloffay · 2018-05-07T16:10:13Z

I think those two should be compatible.

keitwb · 2018-05-09T18:27:51Z

@pavolloffay ok, I made an initial attempt at reusing Zipkin reporters as much as possible. It is a bit tricky since there are Zipkin 1 and 2 reporters with a different Java interface, but I think I have simplified it as much as possible.

This does break existing users of this lib since it totally removes ZipkinSender from this repo, but I am unsure if that is important or not.

keitwb · 2018-05-09T18:30:23Z

jaeger-zipkin/build.gradle

@@ -1,12 +1,16 @@
 description = 'Integration library for Zipkin'

 dependencies {
-    compile group: 'io.zipkin.reporter', name: 'zipkin-reporter', version: '1.1.2'
-    compile group: 'io.zipkin.reporter', name: 'zipkin-sender-urlconnection', version: '1.1.2'
+    compile group: 'io.zipkin.reporter', name: 'zipkin-reporter', version: '1.0.4'


I had to downgrade this because of some nasty classpath conflicts between the version of Zipkin that 1.1.2 and 2.6.0 of the reporter were using. Is there some way to pin the version of Zipkin used by dependencies of this module?

Do you know what artifact is causing the problems?

zipkin-reporter 1.1.2 depends on Zipkin 2.2.1 and zipkin-reporter 2.6.0 depends on Zipkin 2.8.1. Between those two versions of Zipkin, an overloaded version of listSizeInBytes was added that takes an int in zipkin2.codec.Encoding. If I try and use the OkHttpSender from zipkin-reporter 2.6.0, I get the following exception:

java.lang.NoSuchMethodError: zipkin2.codec.Encoding.listSizeInBytes(I)I at zipkin2.reporter.okhttp3.OkHttpSender.messageSizeInBytes(OkHttpSender.java:211) at zipkin2.reporter.AsyncReporter$BoundedAsyncReporter.report(AsyncReporter.java:232) at io.jaegertracing.senders.zipkin.ZipkinReporter.report(ZipkinReporter.java:36) at io.jaegertracing.Tracer.reportSpan(Tracer.java:160) at io.jaegertracing.Span.finishWithDuration(Span.java:183) at io.jaegertracing.Span.finish(Span.java:160)

Because it is trying to use the new int overloaded method but zipkin 2.2.1 is getting chosen on the classpath instead of 2.8.1.

Do you know any simple way to make it always use the newer version of Zipkin?

What If you exclude the old zipkin(2.2.1) from reporter 1.1.2?

I'll see if it is still an issue after I revert the ZipkinSender and only implement the V2 Reporter.

keitwb · 2018-05-10T20:59:10Z

jaeger-zipkin/src/main/java/io/jaegertracing/senders/zipkin/V2SpanConverter.java

+        endLabel = zipkin.Constants.CLIENT_RECV;
+      }
+
+      builder.addAnnotation(span.getStart(), startLabel);


Are these special annotations necessary in v2 Zipkin?

I don't think so

cs time - is kind=client and start time
cr time - is kind=client starttime+duration

the same for server side, but the kind is server

keitwb · 2018-05-14T18:53:15Z

@pavolloffay How does it look now?

pavolloffay · 2018-05-15T09:16:31Z

I think it's better to keep the zipkin sender and just deprecate it.

pavolloffay

I have checked the convertor it looks good.

Could you please revert the old sender back? Do not deprecate it and just introduce ZipkinV2Reporter?In the appropriate package e.g. io.jaegertracing.zipkin (for convertor) and io.jaegertracing.zipkin.reporters for reporter?

In the next PR we could introduce ZipkinV1Reporter and deprecate the sender to keep the changes small and clean.

pavolloffay · 2018-05-15T12:26:13Z

jaeger-zipkin/build.gradle

    compile project(path: ':jaeger-core', configuration: 'shadow')

+    testCompile group: 'io.zipkin.reporter', name: 'zipkin-sender-urlconnection', version: '1.0.4'
+    testCompile group: 'io.zipkin.reporter2', name: 'zipkin-sender-urlconnection', version: '2.6.0'


Is this needed? If not do not include it

pavolloffay · 2018-05-15T13:50:25Z

jaeger-zipkin/src/test/java/io/jaegertracing/senders/zipkin/V2SpanConverterTest.java

+  @Before
+  public void setUp() {
+    tracer =
+        new Tracer.Builder("test-service-name", new InMemoryReporter(), new ConstSampler(true))


please do not use deprecated APIs

keitwb · 2018-05-15T20:07:57Z

@pavolloffay ok, I reverted the original ZipkinSender and rearranged things a bit. I think I figured out how to exclude Zipkin 2.2.1 from polluting the classpath. Let me know what you think.

pavolloffay · 2018-05-16T09:38:59Z

@keitwb could you please keep ThriftSpanConverter in the old package? Just keep the changes minimal

and ThriftSpanConverterTest.java too

pavolloffay · 2018-05-16T10:07:08Z

jaeger-zipkin/README.md

+import zipkin2.reporter.AsyncReporter;
+import zipkin2.reporter.urlconnection.URLConnectionSender;
+
+reporter = new Zipkin2Reporter(


nit: could you please just rename it to ZipkinV2Reporter?

pavolloffay · 2018-05-16T10:08:29Z

jaeger-zipkin/src/main/java/io/jaegertracing/zipkin/V2SpanConverter.java

+        endLabel = zipkin.Constants.CLIENT_RECV;
+      }
+
+      builder.addAnnotation(span.getStart(), startLabel);


From the previous review: these are not needed in V2

pavolloffay · 2018-05-16T10:09:42Z

@keitwb the PR looks good, I had just a couple of minor comments.

This adds a new class called Zipkin2Reporter that adapts a Zipkin v2 reporter and converts Jaeger spans to Zipkin v2 spans. This enables us to send spans to a Zipkin v2 server using the newer protocol. This also moves the existing Thrift conversion logic to a new package `io.jaegertracing.zipkin` where it exists alongside the new v2 conversion logic. Signed-off-by: Ben Keith <bkeith@signalfx.com>

keitwb · 2018-05-16T14:13:55Z

jaeger-zipkin/src/main/java/io/jaegertracing/zipkin/V2SpanConverter.java

+        componentName = span.getTracer().getServiceName();
+      }
+
+      builder.putTag(zipkin.Constants.LOCAL_COMPONENT, componentName);


What about this one? In Zipkin v2 spans, the local endpoint field contains the service name so this seems redundant to add this special tag.

I'm going to remove it for now but let me know if you think it should remain.

keitwb · 2018-05-16T14:18:55Z

@pavolloffay ok, I moved the Thrift converter back to its original location and rebased. Also, I removed some logic for the local component special tag since it was in the local endpoint object already (see comment). I put the new stuff as a separate commit so you can see it clearly. Thanks!

pavolloffay · 2018-05-16T15:30:51Z

@keitwb travis is failing

keitwb · 2018-05-16T16:20:43Z

There is some race between the zipkin reporter finishing a span and reporting it when the close method is called on the reporter. Let me see what I can do to prevent it.

Also removing Zipkin v1 special tag handling from V2SpanConverter because they are not necessary Signed-off-by: Ben Keith <bkeith@signalfx.com>

keitwb · 2018-05-16T19:50:21Z

Ok, I did the same thing the Zipkin reporter tests do and it seemed to fix it.

pavolloffay · 2018-05-17T08:16:59Z

@keitwb great work! Thanks for the PR

I will wait for 24h in case anybody else wants to review

jpkrohling

I trust that @pavolloffay checked the Zipkin semantics already, so, I had only a couple of comments, all optional.

jpkrohling · 2018-05-17T08:33:52Z

jaeger-zipkin/build.gradle

+        exclude group:'io.zipkin.java'
+    }
+    compile(group: 'io.zipkin.reporter', name: 'zipkin-sender-urlconnection', version: '1.1.2') {
+        exclude group:'io.zipkin.zipkin2'


Is this needed, if you are declaring it as a direct dependency of your project?

Tested it, and you are right, it doesn't appear to be necessary due to the explicit dependency.

jpkrohling · 2018-05-17T08:49:23Z

jaeger-zipkin/src/main/java/io/jaegertracing/zipkin/V2SpanConverter.java

+    } else if (Tags.SPAN_KIND_PRODUCER.equals(kind)) {
+      return zipkin2.Span.Kind.PRODUCER;
+    } else {
+      return null;


Is this an expected case? If not, a log statement should be produced, like, "Could not identify the span kind during conversion. Skipping."

IIRC internal spans has a null kind.

yes they have kind set to null

jpkrohling · 2018-05-17T08:51:04Z

jaeger-zipkin/src/main/java/io/jaegertracing/zipkin/V2SpanConverter.java

+    try {
+      return InetAddress.getByAddress(bytes);
+    } catch (UnknownHostException e) {
+      return null;


IIRC, this should never happen but would be worth adding a log statement here.

jpkrohling · 2018-05-17T08:52:51Z

jaeger-zipkin/src/test/java/io/jaegertracing/zipkin/V2SpanConverterTest.java

+  Tracer tracer;
+
+  @Before
+  public void setUp() {


s/setUp/setup

Signed-off-by: Ben Keith <bkeith@signalfx.com>

pavolloffay · 2018-05-18T09:44:39Z

I am just linking a similar PR in go jaegertracing/jaeger-client-go#310. I have updated the branch and I will merge on green

Expands span reporter selection by breaking out Reporter into base QueueReporter class and using reporting method-specific ThriftReporter and ZipkinV2Reporter. Also breaks LocalAgentSender into two classes including LocalAgentReader for ThriftReporter only usage of TBufferedTransport. These changes allow users to continue reporting spans oob to the local jaeger-agent but with the option of reporting to Zipkin backend. In the spirit of jaegertracing/jaeger-client-java#399 and jaegertracing/jaeger-client-go#310 Signed-off-by: Ryan Fitzpatrick <rmfitzpatrick@signalfx.com>

keitwb force-pushed the zipkin2-sender branch from b135446 to f4b4ed5 Compare April 17, 2018 15:31

mpetazzoni suggested changes Apr 17, 2018

View reviewed changes

keitwb force-pushed the zipkin2-sender branch from f4b4ed5 to 5050c73 Compare April 24, 2018 20:23

mpetazzoni approved these changes Apr 24, 2018

View reviewed changes

yurishkuro requested review from jpkrohling and pavolloffay April 24, 2018 22:49

keitwb force-pushed the zipkin2-sender branch from 8724fef to 60b8584 Compare May 2, 2018 21:18

keitwb force-pushed the zipkin2-sender branch from 60b8584 to 6d72916 Compare May 9, 2018 18:22

keitwb commented May 9, 2018

View reviewed changes

keitwb commented May 10, 2018

View reviewed changes

pavolloffay reviewed May 15, 2018

View reviewed changes

keitwb force-pushed the zipkin2-sender branch from 6d72916 to ee27972 Compare May 15, 2018 20:05

keitwb force-pushed the zipkin2-sender branch from ee27972 to 5b29db3 Compare May 15, 2018 21:09

pavolloffay reviewed May 16, 2018

View reviewed changes

keitwb commented May 16, 2018

View reviewed changes

keitwb force-pushed the zipkin2-sender branch from 5b29db3 to 3597ca7 Compare May 16, 2018 14:15

keitwb force-pushed the zipkin2-sender branch from 3597ca7 to a0499cf Compare May 16, 2018 14:20

pavolloffay mentioned this pull request May 16, 2018

Does it make sense to use gzip compression for Thrift HTTP senders? #225

Closed

keitwb force-pushed the zipkin2-sender branch from a0499cf to a039956 Compare May 16, 2018 15:32

Reverting ThriftSpanConverter to origin location

5730b13

Also removing Zipkin v1 special tag handling from V2SpanConverter because they are not necessary Signed-off-by: Ben Keith <bkeith@signalfx.com>

keitwb force-pushed the zipkin2-sender branch from a039956 to 5730b13 Compare May 16, 2018 19:37

pavolloffay approved these changes May 17, 2018

View reviewed changes

jpkrohling approved these changes May 17, 2018

View reviewed changes

Ben Keith and others added 2 commits May 17, 2018 09:39

Misc changes from code review

ec6a465

Signed-off-by: Ben Keith <bkeith@signalfx.com>

Merge branch 'master' into zipkin2-sender

b497c36

ghost assigned pavolloffay May 18, 2018

ghost added the review label May 18, 2018

pavolloffay merged commit 07df0b8 into jaegertracing:master May 18, 2018

ghost removed the review label May 18, 2018

keitwb deleted the zipkin2-sender branch May 18, 2018 17:16

rmfitzpatrick mentioned this pull request Jul 6, 2018

Add Zipkin API V2 Reporter jaegertracing/jaeger-client-python#197

Closed

black-adder mentioned this pull request Jul 10, 2018

Move zipkin reporting logic to jaeger agent/collector jaegertracing/jaeger#925

Closed

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Implement Zipkin 2 JSON Sender #399

Implement Zipkin 2 JSON Sender #399

Conversation

keitwb commented Apr 17, 2018 • edited Loading

codecov bot commented Apr 17, 2018 • edited Loading

Codecov Report

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

mpetazzoni left a comment

Choose a reason for hiding this comment

jpkrohling commented Apr 30, 2018

pavolloffay commented May 2, 2018

keitwb commented May 2, 2018

pavolloffay commented May 3, 2018

pavolloffay commented May 3, 2018

keitwb commented May 7, 2018

pavolloffay commented May 7, 2018

keitwb commented May 9, 2018

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

keitwb May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keitwb commented May 14, 2018

pavolloffay commented May 15, 2018

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keitwb commented May 15, 2018

pavolloffay commented May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented May 16, 2018

Choose a reason for hiding this comment

keitwb commented May 16, 2018

pavolloffay commented May 16, 2018

keitwb commented May 16, 2018

keitwb commented May 16, 2018

pavolloffay commented May 17, 2018

jpkrohling 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

pavolloffay commented May 18, 2018

keitwb commented Apr 17, 2018 •

edited

Loading

codecov bot commented Apr 17, 2018 •

edited

Loading

keitwb May 10, 2018 •

edited

Loading

pavolloffay commented May 16, 2018 •

edited

Loading