-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Decouples from io.zipkin.zipkin2:zipkin #234
Conversation
Sorry for silly question @codefromthecrypt , I understood the purpose of the change but the overall picture is not entirely clear to me: there would be 2 sets of largely similar classes |
@@ -54,6 +54,12 @@ | |||
<artifactId>zipkin-reporter</artifactId> | |||
<version>${project.version}</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exclusion here (as in brave/pom.xml
)?
<exclusions>
<exclusion>
<groupId>io.zipkin.zipkin2</groupId>
<artifactId>zipkin</artifactId>
</exclusion>
</exclusions>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good eye, I was about 5 minutes from finding this out myself ;)
@reta good question. Actually, the amount of types reproduced were not big. In hindsight, we shouldn't have shared any base types here. I noticed this in #182. The generic type parameter on zipkin2.Reporter was thrwarted by an unremovable dependency on zipkin types. A couple years ago we made a more pliable MutableSpan type in brave, which has its own json encoder, so doesn't need the zipkin types. So, the changes here are:
The main point is the second, as I am finishing an update that will remove deprecated methods on Brave, and completely decouple it from zipkin types. Doing that and not also here would still drag them in. |
Another side point is that when we do this decoupling, it is easier to eventually remove the floor Java 6 situation on zipkin's core jar. While this doesn't remove the problem entirely, it does for almost all Brave users, which is a large percentage of uses of zipkin-reporter |
I am testing with a branch of both brave now (still zipkin incidental zipkin dep, but only for deprecated functions) and brave 6 (zipkin free due to deprecation removal). I'll test both with brave-example, then ready to merge if nothing breaks in those two. |
This copies types from zipkin, so that reporter can be used without a dependency on it. In practice, many still will because encoders will be present. However, those using Brave and only JSON or alternative backends like Wavefront can now avoid a dependency on Zipkin. Fixes #182 Signed-off-by: Adrian Cole <[email protected]>
4aac342
to
beeccbd
Compare
Signed-off-by: Adrian Cole <[email protected]>
status update: I became distracted after discovering our bytecode versions weren't correct before this change. That's all sorted. What's next is I have a branch of brave which uses no zipkin2 types at all. I want to test that with this branch in brave-example, if that works, then test this branch on existing brave, as the intent is that this could drop in without breaking most users. Regardless, this would be a major version bump on zipkin-reporter, just hoping that it is mostly compatible, and completely for end users not writing their own reporters/senders etc. |
activemq-client/src/main/java/zipkin2/reporter/activemq/ActiveMQSender.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Adrian Cole <[email protected]>
I did some more dependency sweeping mostly about Brave 6, not this, and nearly ready to commit to this. I still need to test brave examples both with this on 5.18 and also 6.0. Then, we'll cut this dep and it to brave 5.18 and cut it, then also release Brave 6.0. Getting closer now! |
It is hard to test this properly with jitpack. I will merge this and use sonatype snapshot repo to test with brave before releasing. Will still mention here the status. |
Things are working in expected ways, so cutting 3.0. Thanks for all the help here! This might be a good comment to bookmark @reta @shakuzen as basically brave-example tests run an end-to-end including running a trace through. If the docker test passes, basically it works. Before $ docker run --entrypoint /bin/sh --rm ghcr.io/openzipkin/brave-example:webmvc25-jetty -c 'find . -name \*jar|sort'
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
./root/WEB-INF/lib/aopalliance-1.0.jar
./root/WEB-INF/lib/brave-5.17.1.jar
./root/WEB-INF/lib/brave-context-log4j12-5.17.1.jar
./root/WEB-INF/lib/brave-instrumentation-http-5.17.1.jar
./root/WEB-INF/lib/brave-instrumentation-httpclient-5.17.1.jar
./root/WEB-INF/lib/brave-instrumentation-messaging-5.17.1.jar
./root/WEB-INF/lib/brave-instrumentation-rpc-5.17.1.jar
./root/WEB-INF/lib/brave-instrumentation-servlet-5.17.1.jar
./root/WEB-INF/lib/brave-instrumentation-spring-webmvc-5.17.1.jar
./root/WEB-INF/lib/brave-spring-beans-5.17.1.jar
./root/WEB-INF/lib/commons-codec-1.6.jar
./root/WEB-INF/lib/commons-logging-1.1.1.jar
./root/WEB-INF/lib/httpclient-4.3.6.jar
./root/WEB-INF/lib/httpcore-4.3.3.jar
./root/WEB-INF/lib/log4j-1.2.17.jar
./root/WEB-INF/lib/spring-beans-2.5.6.jar
./root/WEB-INF/lib/spring-context-2.5.6.jar
./root/WEB-INF/lib/spring-context-support-2.5.6.jar
./root/WEB-INF/lib/spring-core-2.5.6.jar
./root/WEB-INF/lib/spring-web-2.5.6.jar
./root/WEB-INF/lib/spring-webmvc-2.5.6.jar
./root/WEB-INF/lib/zipkin-2.27.0.jar
./root/WEB-INF/lib/zipkin-reporter-2.17.2.jar
./root/WEB-INF/lib/zipkin-reporter-brave-2.17.2.jar
./root/WEB-INF/lib/zipkin-reporter-spring-beans-2.17.2.jar
./root/WEB-INF/lib/zipkin-sender-urlconnection-2.17.2.jar Now (no zipkin-N.N.N.jar)
|
This copies types from zipkin, so that reporter can be used without a dependency on it. In practice, many still will because encoders will be present. However, those using Brave and only JSON or alternative backends like Wavefront can now avoid a dependency on Zipkin.
Fixes #182
See micrometer-metrics/tracing#509