Upgrades internals to Zipkin v2 library #456

codefromthecrypt · 2018-06-19T02:20:43Z

Which problem is this PR solving?

The current zipkin integration encourages use of a dead version of a library (io.zipkin.reporter)

Short description of the changes

Recent version of io.zipkin.zipkin2:zipkin has been backported to
support thrift encoding. This updates the corresponding sender here to
use that, notably to migrate off the io.zipkin.reporter libraries which
receive no updates.

codecov · 2018-06-19T02:26:58Z

Codecov Report

Merging #456 into master will increase coverage by 0.01%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master     #456      +/-   ##
============================================
+ Coverage     88.29%   88.31%   +0.01%     
- Complexity      489      494       +5     
============================================
  Files            63       63              
  Lines          1846     1848       +2     
  Branches        240      241       +1     
============================================
+ Hits           1630     1632       +2     
  Misses          139      139              
  Partials         77       77

Impacted Files	Coverage Δ	Complexity Δ
.../java/io/jaegertracing/zipkin/V2SpanConverter.java	`87.83% <100%> (+3.02%)`	`21 <1> (+1)`	⬆️
...aegertracing/senders/zipkin/ThriftSpanEncoder.java	`80% <100%> (+3.07%)`	`4 <3> (+2)`	⬆️
.../io/jaegertracing/senders/zipkin/ZipkinSender.java	`65.67% <50%> (-2.08%)`	`17 <3> (+2)`

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 f5a1097...3cb58bc. Read the comment docs.

codefromthecrypt · 2018-06-19T02:39:33Z

ps I plan to add a test for the v2 path, but only after figuring out the following:

would this be mergable in general?
since the version is <1.0 can we remove the old factory method now? If so, there will be less test work to do to achieve the same coverage.

yurishkuro · 2018-06-19T04:22:43Z

+1 on (1)
no strong feeling on (2)

If people are running Zipkin collectors with v1 thrift model only (i.e. have not upgraded to collectors that accept v2 model), would the now-deprecated v1 factory still work for them?

codefromthecrypt · 2018-06-19T04:50:58Z

The main change here is the library that sends the messages. both the new and old factory method assume thrift. I think there is different code in the same package which can use non-thrift. That hasnt changed as that side is already using the zipkin2 reporter library. on zipkin server side, v1 POST with thrift is available for the foreseeable future and the kafka transport also still accepts thrift.

codefromthecrypt · 2018-06-19T08:27:56Z

ok tests now use both paths.

I'm keen either way, retaining the deprecated factory method for a release, or cutting it now. If we do the former, we need to remember to remove it later.

pavolloffay · 2018-06-19T08:33:24Z

As there is ZipkinV2Reporter (which can be used to sent thrift) can't we just deprecate the entire sender? Now we have to maintain two converters and sender only for thrift.

codefromthecrypt · 2018-06-19T08:36:28Z

my goal is to drop the dependency on io.zipkin.java dropping the thrift sender achieves that as long as dropping it doesn't limit folks in some way I'm not aware of.

codefromthecrypt · 2018-06-19T08:37:23Z

note even if you do drop the sender (now or later) the dependencies in this change are cleaner, especially the tests. Please retain that part if you decide to drop the sender.

pavolloffay · 2018-06-19T08:41:00Z

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

-   * sender = ZipkinSender.create(KafkaSender.create("192.168.99.100:9092"));
+   * sender = ZipkinSender.create(
+   *     KafkaSender.newBuilder()
+   *                .encoding(Encoding.THRIFT)


What happens if the encoding != thrift?

Cool there is a check. nevermind

codefromthecrypt · 2018-06-20T10:10:04Z

@yurishkuro etc I think the least tech debt way here is to remove the deprecated factory method which then removes a large part of this code which will restore coverage to where it was. This also allows a smooth transition for those who can't immediately switch off thrift (for example)

if you say go, I'll do that and hopefully we can get this merged.

yurishkuro · 2018-06-20T20:40:32Z

@adriancole I have no problem with removing the factory, my question was about functional impact. I haven't followed zipkin v2 too closely, was it a change to JSON schema only, or did thrift model changed as well? If the thrift model is the same (https://github.com/openzipkin/zipkin-api/blob/master/thrift/zipkinCore.thrift - this one?), then upgrading the library to send it makes total sense.

codefromthecrypt · 2018-06-20T23:57:01Z

thrift is the same. we backported the encoding logic, so basically it takes a v2 model and allocates it across a v1 (aka only) thrift struct when writing. will yank the old path which will be nicer I think in general.

codefromthecrypt · 2018-06-21T00:20:14Z

ok ready to roll. I can squash or you can

codefromthecrypt · 2018-06-21T00:21:30Z

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

@@ -122,14 +119,13 @@ private static void buildTags(Span span, zipkin2.Span.Builder builder) {
    }
  }

-  private static InetAddress convertIp(int ip) {


I fixed this as it was bothering me

codefromthecrypt · 2018-06-21T00:21:49Z

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

@@ -88,9 +89,6 @@ public void testAppendSpanTooLarge() throws Exception {

  @Test
  public void testAppend() throws Exception {
-    // find size of the initial span
-    AutoExpandingBufferWriteTransport memoryTransport =


codefromthecrypt · 2018-06-21T00:23:59Z

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

@@ -160,9 +175,14 @@ public int close() throws SenderException {
    return n;
  }

+  @Override
+  public String toString() {
+    return "ZipkinSender{" + delegate + "}";


previous toString looked bad as another field was forgotten to be excluded. this is a problem with auto-generated toStrings. This type is simple and this won't break even if you add new fields later.

Looks like this PR removes @ToString and implements its own toString(). Did you have problems with the resulting toString() from @ToString?

codefromthecrypt · 2018-06-21T00:33:47Z

sorry i answered wrong question. this change is about senders. no impact as they take byte arrays and dont mess with them besides bundling

…

On Thu, 21 Jun 2018, 07:56 Adrian Cole, ***@***.***> wrote: thrift is the same. we backported the encoding logic, so basically it takes a v2 model and allocates it across a v1 (aka only) thrift struct when writing. will yank the old path which will be nicer I think in general.

yurishkuro · 2018-06-21T03:51:58Z

I can squash or you can

Our (CNCF's) contributing policy requires signed commits (the DCO check), so it's best if you squash them and sign as a single commit (git commit -s ...)

codefromthecrypt · 2018-06-21T04:05:20Z

Signed with a Flourish

codefromthecrypt · 2018-06-21T04:22:29Z

actually will push a slight test update as I used the wrong exception type on invalid format

codefromthecrypt · 2018-06-21T06:08:25Z

hopefully I've now addressed the severely annoying test coverage requirements

pavolloffay · 2018-06-21T08:25:34Z

jaeger-zipkin/build.gradle


-    compile group: 'io.zipkin.java', name: 'zipkin', version: '2.8.1'
-    compile group: 'io.zipkin.reporter2', name: 'zipkin-reporter', version: '2.6.0'
+    implementation 'io.zipkin.reporter2:zipkin-reporter'


shouldn't this be api scope?

pavolloffay · 2018-06-21T08:38:02Z

jaeger-zipkin/build.gradle

-    compile group: 'io.zipkin.java', name: 'zipkin', version: '2.8.1'
-    compile group: 'io.zipkin.reporter2', name: 'zipkin-reporter', version: '2.6.0'
+    implementation 'io.zipkin.reporter2:zipkin-reporter'
+    implementation 'io.zipkin.reporter2:zipkin-sender-urlconnection'


This is new to me, it's related to https://github.com/jaegertracing/jaeger-client-java/pull/456/files#diff-88b7c47e47b8ee65263b6784b86fa0a7R1

I got this when I included it into a different project

[WARNING] The POM for io.jaegertracing:jaeger-zipkin:jar:0.29.1-SNAPSHOT is invalid, transitive dependencies (if any) will not be available, enable debug logging for more details

https://docs.gradle.org/4.6/userguide/java_library_plugin.html#sec:java_library_separation

Dependencies appearing in the api configurations will be transitively exposed to consumers of the library, and as such will appear on the compile classpath of consumers. Dependencies found in the implementation configuration will, on the other hand, not be exposed to consumers, and therefore not leak into the consumers' compile classpath. This comes with several benefits:

We basically require our clients to explicitly add dependency on zipkin-sender-urlconnection to make this work. If am correct it will fail on java.lang.NoClassDefFoundError: zipkin2/reporter/urlconnection/URLConnectionSender even if you use different sender.

codefromthecrypt · 2018-06-21T08:53:14Z

hmm maybe they should both be api I guess. Will fix!

pavolloffay · 2018-06-21T08:55:36Z

+1, or just include the reporter and let users specify sender they want. But it's a bigger breaking change. I guess url sender does not pull in anything special so it's fine to include it.

codefromthecrypt · 2018-06-21T09:02:12Z

hmm

> Could not find method api() for arguments [io.zipkin.reporter2:zipkin-reporter] on object of type org.gradle.api.internal.artifacts.dsl.dependencies.DefaultDependencyHandler.

codefromthecrypt · 2018-06-21T09:03:55Z

looks like api can work if I upgrade to gradle 4.8, adding that..

jpkrohling · 2018-06-21T09:45:51Z

gradle/wrapper/gradle-wrapper.properties

@@ -1,5 +1,5 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.8-bin.zip


It would be awesome if you could update the version specified under the Wrapper task as well: https://github.com/jaegertracing/jaeger-client-java/blob/master/build.gradle#L40

jpkrohling · 2018-06-21T09:49:28Z

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

@@ -160,9 +175,14 @@ public int close() throws SenderException {
    return n;
  }

+  @Override
+  public String toString() {
+    return "ZipkinSender{" + delegate + "}";


Looks like this PR removes @ToString and implements its own toString(). Did you have problems with the resulting toString() from @ToString?

jpkrohling · 2018-06-21T09:51:06Z

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

@@ -52,7 +53,7 @@
  Tracer tracer;

  @Before
-  public void setUp() throws Exception {


codefromthecrypt · 2018-06-21T10:10:53Z

with regards to lombok toString. it was maintenance causing as someone earlier forgot to exclude a field. I mentioned this in am earlier comment on this PR

jpkrohling · 2018-06-21T10:32:52Z

Sorry, missed your previous comment about the toString(). I'm OK with that but would prefer to keep the @ToString for consistency's sake.

codefromthecrypt · 2018-06-21T10:33:27Z

as thinga dont seem to be working like I hoped and I am spending more time than I should.. i will revert the bom thing amd repush the change. feel free to re-bom later.

codefromthecrypt · 2018-06-21T10:35:41Z

I will also revert the lombok tostring thing as that tool deserves no more time from me!

Recent version of io.zipkin.zipkin2:zipkin has been backported to support thrift encoding. This updates the corresponding sender here to use that, notably to migrate off the io.zipkin.reporter libraries which receive no updates. Signed-off-by: Adrian Cole <acole@pivotal.io>

codefromthecrypt · 2018-06-21T11:13:53Z

unless there's more karmatic debt to pay in the form of build failures, I'm done I think.

yurishkuro

lgtm

@pavolloffay @jpkrohling how does it look to you?

jpkrohling

LGTM

jpkrohling · 2018-06-21T16:53:06Z

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

@@ -48,41 +54,52 @@
 * <p>
 * See https://github.com/openzipkin/zipkin/tree/master/zipkin-server
 */
-@ToString(exclude = "spanBuffer")
+@ToString(exclude = {"spanBuffer", "encoder"})


\o/

(not that I'm a fan of Lombok, btw)

pavolloffay

thanks @adriancole

codefromthecrypt · 2018-06-23T01:42:20Z

thanks for getting this merged

pavolloffay reviewed Jun 19, 2018

View reviewed changes

codefromthecrypt commented Jun 21, 2018

View reviewed changes

pavolloffay reviewed Jun 21, 2018

View reviewed changes

jpkrohling reviewed Jun 21, 2018

View reviewed changes

codefromthecrypt changed the title ~~Upgrades internals to Zipkin v2 library deprecating v1 factory method~~ Upgrades internals to Zipkin v2 library Jun 21, 2018

yurishkuro approved these changes Jun 21, 2018

View reviewed changes

jpkrohling approved these changes Jun 21, 2018

View reviewed changes

pavolloffay approved these changes Jun 22, 2018

View reviewed changes

pavolloffay merged commit eb91a3a into jaegertracing:master Jun 22, 2018

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Upgrades internals to Zipkin v2 library #456

Upgrades internals to Zipkin v2 library #456

Conversation

codefromthecrypt commented Jun 19, 2018

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jun 19, 2018 • edited Loading

Codecov Report

codefromthecrypt commented Jun 19, 2018

yurishkuro commented Jun 19, 2018

codefromthecrypt commented Jun 19, 2018 via email

codefromthecrypt commented Jun 19, 2018

pavolloffay commented Jun 19, 2018

codefromthecrypt commented Jun 19, 2018 via email

codefromthecrypt commented Jun 19, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Jun 20, 2018

yurishkuro commented Jun 20, 2018

codefromthecrypt commented Jun 20, 2018 via email

codefromthecrypt commented Jun 21, 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

codefromthecrypt commented Jun 21, 2018 via email

yurishkuro commented Jun 21, 2018

codefromthecrypt commented Jun 21, 2018 via email

codefromthecrypt commented Jun 21, 2018

codefromthecrypt commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Jun 21, 2018 via email

pavolloffay commented Jun 21, 2018

codefromthecrypt commented Jun 21, 2018

codefromthecrypt commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Jun 21, 2018 via email

jpkrohling commented Jun 21, 2018

codefromthecrypt commented Jun 21, 2018

codefromthecrypt commented Jun 21, 2018

codefromthecrypt commented Jun 21, 2018

yurishkuro left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Jun 23, 2018 via email

codecov bot commented Jun 19, 2018 •

edited

Loading