line
Makes armeria-brave dependencies compile ZipkinSpanHandler
Motivation:
armeria-brave was changed in a unique way in 1.27.1 to pin an old version of the library, even though there are no code compatibility issues.
What happened was some folks at LINE have a metrics library, and instead of depending on zipkin-reporter, they relied on brave's indirect dependency version. In a rush to fix this quick, brave-armeria was reverted to 5.x and a new module was made for brave6. Another was also made for brave5 despite almost certainly no more released on that line of code.
The problem is that a normal person would expect "armeria-brave" to have the more recent version of brave, and instead they will be pinned and not receive code updates. They will have no easy way to find this out because there is no code in "armeria-brave" to mark deprecated. This basically means people will unknowingly always stay at an old version.
Another choice would have been to still make the separate modules, but make the main "armeria-brave" tactically set the old dependency it had before. This is better because it allows a migration without serious impact. This is what I believe would have been the choice if it wasn't rushed to stop a fire at LINE.
I hope people can consider this and merge it with any modifications that keep LINE not having to manage versions, yet not pin the rest of the world to an old library.
If not, I hope people can be transparent with who is making the decision and literally what the problem is, as this doesn't seem difficult to fix from how it was described in #5438. Maybe others who know the library like @kojilin can explain if somehow managing a dependency like this uniquely cannot work at LINE. I often promote armeria as a project for everyone, not just LINE, and hope we can turn this around.
Modifications:
- Upgraded armeria-brave back to Brave 6, which doesn't care about the zipkin-reporter version
- Added the last zipkin-reporter-brave 3.x dependency to armeria-brave, but not armeria-brave5 or armeria-brave6.
- Added zipkin core jar dependency to armeria-brave, so that ZipkinSpanHandler can compile with no extra deps
Result:
- Someone using armeria-brave can, as recommended before, use the reporter-bom and override 2.x with 3.x.
- Someone who doesn't want a reporter dep can exclude it or use armeria-brave6
- See #5438
fyi I published this local and here are the differences, assuming you don't use the armeria-bom and just look at indirect deps from armeria-brave. I tried to match the pre-reporter 3.0 thing, even though brave5 uses reporter 3.
com.linecorp.armeria:armeria-brave:1.26.4
[INFO] +- io.zipkin.brave:brave:jar:5.16.0:compile
[INFO] | \- io.zipkin.reporter2:zipkin-reporter-brave:jar:2.16.3:compile
[INFO] | \- io.zipkin.reporter2:zipkin-reporter:jar:2.16.3:compile
[INFO] | \- io.zipkin.zipkin2:zipkin:jar:2.23.2:compile
[INFO] +- io.zipkin.brave:brave-instrumentation-http:jar:5.16.0:compile
com.linecorp.armeria:armeria-brave:<this PR>
[INFO] | +- io.zipkin.brave:brave:jar:6.0.0:compile
[INFO] | \- io.zipkin.brave:brave-instrumentation-http:jar:6.0.0:compile
[INFO] +- io.zipkin.reporter2:zipkin-reporter-brave:jar:2.17.2:compile
[INFO] | \- io.zipkin.reporter2:zipkin-reporter:jar:2.17.2:compile
[INFO] | \- io.zipkin.zipkin2:zipkin:jar:2.27.0:compile
Note: if 1.27.1 works at LINE than we actually don't need to pin to reporter 2.x. Basically if 1.27.1 works, it could be as simple as adding a zipkin core jar dep.
As you can see 1.27.1 armeria-brave uses a recent zipkin-reporter version (3.x not 2.x as armeria 1.26.4 had used):
[INFO] +- com.linecorp.armeria:armeria-brave5:jar:1.27.1:compile
[INFO] | +- io.zipkin.brave:brave:jar:5.18.0:compile
[INFO] | | +- io.zipkin.reporter2:zipkin-reporter-brave:jar:3.0.0:compile
[INFO] | | | \- io.zipkin.reporter2:zipkin-reporter:jar:3.0.0:compile
[INFO] | | \- io.zipkin.zipkin2:zipkin:jar:2.27.0:compile
[INFO] | \- io.zipkin.brave:brave-instrumentation-http:jar:5.18.0:compile
TL;DR; is if in fact 1.27.1 fixed it, maybe we can get by with most recent zipkin-reporter 3.x, but an extra dep on io.zipkin.zipkin2:zipkin and a note about why.
SUMMARY:
My suspicion is that this is more about a "convenience dep" on io.zipkin.zipkin2:zipkin rather than specifically about reporter 2.x vs 3.x. If it was about zipkin-reporter 2.x vs 3.x, the armeria 1.27.1 would have failed to work.
So, I think we should update this to the latest reporter and also add the "convenience dep" this makes version maintenance a lot easier as you can blindly update these versions and not have to worry about 2.x or 3.x
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
a117c25) 73.99% compared to head (55f0957) 74.00%. Report is 7 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #5453 +/- ##
==========================================
Coverage 73.99% 74.00%
- Complexity 20740 20775 +35
==========================================
Files 1800 1801 +1
Lines 76426 76530 +104
Branches 9728 9749 +21
==========================================
+ Hits 56552 56636 +84
- Misses 15266 15276 +10
- Partials 4608 4618 +10
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@jrhee17 is there any way to verify this at LINE with one of the apps that struggled or a similar reproducer? I want to make sure we get this right..
So, I think we should update this to the latest reporter and also add the "convenience dep" this makes version maintenance a lot easier as you can blindly update these versions and not have to worry about 2.x or 3.x
I didn't have time to actually analyze the dependency relationship between brave, zipkin, armeria so let me know if I'm misunderstanding anything 😄
The report we received was literally NoClassDefFoundError: zipkin2/reporter/brave/ZipkinSpanHandler.
Rather than pinning to a major version (2.x or 3.x), I think it makes sense to follow compatibility between zipkin and brave.
So if brave6 compatibility with zipkin3 is actively tested, I think it probably makes sense to update to the latest version
but an extra dep on io.zipkin.zipkin2:zipkin and a note about why.
If we directly use zipkin 3, then couldn't we drop zipkin 2?
is there any way to verify this at LINE with one of the apps that struggled or a similar reproducer
Once we merge this to main, I can request one of our users to test with a snapshot version
@jrhee17 so the thing is that it is possible to use zipkin2/reporter/brave/ZipkinSpanHandler without even the zipkin core library present. Because the input type is brave's MutableSpan.
So, in this case I think if you can test after merge to master, let's not include the zipkin core library at all, just add the dep on zipkin-reporter-brave. If things work, that's better as less to manage and less dependencies. For example, zipkin's core jar is a few hundred KB.
actually zipkin2/reporter/brave/ZipkinSpanHandler needs zipkin core jar, I can see it does. The async one does not. I'll keep the zipkin core dep.
🔍 Build Scan® (commit: 55f09576ee9f9e2f847a71274366406659396b43)
| Job name | Status | Build Scan® |
|---|---|---|
| build-windows-latest-jdk-19 | ✅ | https://ge.armeria.dev/s/lwkcxmcbrq5r6 |
| build-self-hosted-unsafe-jdk-8 | ✅ | https://ge.armeria.dev/s/huhva47fab5x4 |
| build-self-hosted-unsafe-jdk-19-snapshot-blockhound | ✅ | https://ge.armeria.dev/s/zi3plwb4ah6im |
| build-self-hosted-unsafe-jdk-17-min-java-17-coverage | ✅ | https://ge.armeria.dev/s/ydoc4i5scko22 |
| build-self-hosted-unsafe-jdk-17-min-java-11 | ✅ | https://ge.armeria.dev/s/63tqqoexokbvm |
| build-self-hosted-unsafe-jdk-17-leak | ✅ | https://ge.armeria.dev/s/dbrra7lt27dfo |
| build-self-hosted-unsafe-jdk-11 | ✅ | https://ge.armeria.dev/s/khkmmkvwz3cyo |
| build-macos-12-jdk-19 | ✅ | https://ge.armeria.dev/s/tfsrtgc2iog7k |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
ok I set current versions and also added a missing test about what this is all about, basically that there are no class problems in indirect deps
The changes look acceptable since there are no code breaking changes in Brave 6. @mauhiz Any thoughts on this?
I'm good with this. The PR submitter took extra care to maintain compatibility, and now that he is a code owner for the module he will be able to ensure that users won't be left wishing they didn't upgrade :)
On Tue, 13 Feb 2024, 16:35 Ikhun Um, @.***> wrote:
The changes look acceptable since there are no code breaking changes in Brave 6. @mauhiz https://github.com/mauhiz Any thoughts on this?
— Reply to this email directly, view it on GitHub https://github.com/line/armeria/pull/5453#issuecomment-1940596771, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN34SZFEPAQHGVWWS37MILYTMJVDAVCNFSM6AAAAABC7IFNZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBQGU4TMNZXGE . You are receiving this because you were mentioned.Message ID: @.***>
fyi new toys for those using the deps in question https://github.com/openzipkin/zipkin-reporter-java/releases/tag/3.3.0
so I assume we don't need this PR anymore as basically the change is removing armeria-brave and adding the package-info to deprecate brave5?
so I assume we don't need this PR anymore as basically the change is removing armeria-brave and adding the package-info to deprecate brave5?
Yep. Let me push the patch to this PR. 😉
