Remove armeria-brave module #5453
Conversation
codefromthecrypt
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
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 com.linecorp.armeria:armeria-brave: |
|
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): 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 |
|
SUMMARY: My suspicion is that this is more about a "convenience dep" on 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 ReportAll modified and coverable lines are covered by tests ✅
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 ☔ View full report in Codecov by Sentry. |
|
@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.. |
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 So if brave6 compatibility with zipkin3 is actively tested, I think it probably makes sense to update to the latest version
If we directly use zipkin 3, then couldn't we drop zipkin 2?
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. |
|
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 |
|
|
||
| @Test | ||
| void zipkinSpanHandler() { | ||
| final SpanHandler handler = ZipkinSpanHandler.create(Reporter.NOOP); |
There was a problem hiding this comment.
so this test should give 100pct confidence that a dep change won't break without knowing
codefromthecrypt
changed the title
|
updated desc |
Signed-off-by: Adrian Cole <[email protected]>
codefromthecrypt
force-pushed
the
brave6
branch
from
f158706
to
92d9816
Compare
|
squashed |
|
The changes look acceptable since there are no code breaking changes in Brave 6. |
|
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
<#5453 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN34SZFEPAQHGVWWS37MILYTMJVDAVCNFSM6AAAAABC7IFNZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBQGU4TMNZXGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
brave/brave/src/test/java/com/linecorp/armeria/brave/CompilationTest.java
Outdated
Show resolved
Hide resolved
brave/brave/build.gradle
Outdated
| api libs.zipkin.reporter.brave // For AsyncZipkinSpanHandler (native Brave) | ||
| api libs.zipkin // For ZipkinSpanHandler (uses Zipkin core types) |
There was a problem hiding this comment.
Shouldn't we add this to brave6 module instead?
There was a problem hiding this comment.
no because brave 6 intentionally doesn't pin zipkin dependencies (as it doesn't need them at all, actually this is the main point of brave 6 ;))
There was a problem hiding this comment.
AsyncZipkinSpanHandler is the only good way to report spans, ZipkinSpanHandler was a legacy bridge, and pins zipkin classes (rather than brave's native MutablSpan which has its own codecs)
There was a problem hiding this comment.
starting in brave 5, it didn't need a dependency on the library zipkin server needed. brave 6 removed the deprecated functions that still accessed those types. brave 6 can work with zipkin-reporter 2 (which uses zipkin core types) or zipkin-reporter 3 (which doesn't). If we pin brave 6 here to zipkin core types it adds a dependency that it several hundred K and won't be used. I highly suggest the LINE internal code stop using zipkin core types and use MutableSpan instead, or declare their own dependency on core types.
There was a problem hiding this comment.
I was told this was about metrics, so it is far more efficient for the internal code to skip entirely switching to zipkin core types. here's an example of how to do something like that https://github.com/openzipkin/brave/blob/master/brave/src/test/java/brave/features/handler/SpanMetricsCustomizer.java
There was a problem hiding this comment.
so after shower, here's an idea.
we add an armeria sender somewhere, and in that case someone can depend on that and get the correct recent reporter deps. If we add it to armeria-brave, basically that's fine and at least isn't random to add reporter (v3) anymore (because sender and its tests will need zipkin-reporter 3 types).
What we are left with is that large zipkin dep io.zipkin.zipkin2:zipkin which this was all about. zipkin-reporter-brave and all the senders exclude it because it is huge and unused even when reporting to zipkin, if you use the most current way AsyncZipkinSpanHandler as our armeria-example does.
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>zipkin-reporter</artifactId>
<version>${project.version}</version>
<!-- Senders don't use zipkin types. Excluding allows brave users to
avoid them by default. -->
<exclusions>
<exclusion>
<groupId>io.zipkin.zipkin2</groupId>
<artifactId>zipkin</artifactId>
</exclusion>
</exclusions>
</dependency>So, basically it boils things down to one thing.. do we need to keep adding a dep on this? If so, we need to manage zipkin's version, and keep bumping like dependabot even though it isn't used, and also any app that uses brave will be a few hundred K larger than it needs to be.
That's all the info and pros and cons, so I'll leave you to decison making. Thanks for listening @minwoox!
There was a problem hiding this comment.
Let me answer your question.
In general I'm very curious about what armeria's expectations are about indirect dependencies.. that a project should forever declare dependencies on anything they depended on at some point, even after switching major version numbers?
It depends. 😉 If the indirect dependencies are tightly coupled in terms of using our API, we want to retain them even after a major version update so that: 1) users can seamlessly upgrade the Armeria version without modifying their dependency configuration, and 2) we can manage the versions of indirect dependencies. If they're not coupled, we simply let users choose whatever they prefer.
do we need to keep adding a dep on this? If so, we need to manage zipkin's version, and keep bumping like dependabot even though it isn't used,
So it's the same context here. If most of armeria-brave users use AsyncZipkinSpanHandler, we definitely need to add zipkin-reporter. Let me investigate how armeria-brave users use the API so that we can make a decision. 😉
There was a problem hiding this comment.
What's probably confusing is that in Brave 4 these were coupled, Brave 5 decoupled them but kept back compat, and Brave 6 removed coupling.
Adding zipkin-reporter-brave is fine, as it is small. The side effect of adding this is that we have to pin a specific version of zipkin-reporter (3.x now), and will have a problem again on 4.x. zipkin-reporter is a different library than brave, even if it has a bridge. For example, it is also used in otel.
regardless, it won't bring in the polarizing and unneeded dep io.zipkin.zipkin2 dep.
So there are two decisions
- for convenience, pin reporter (as done now in armeria-brave) and know this needs to be revisited in the event of a reporter 4?
- if you do that, also add the pretty large io.zipkin2:zipkin dep
over to you!
There was a problem hiding this comment.
After discussing with the team, we've decided that removing the armeria-brave module is the best approach to address the indirect dependency problem and eliminate any ambiguity by maintaining three different modules.
Previously, we retained the armeria-brave module to avoid causing breaking changes for users. However, we've now agreed that removing the armeria-brave module and providing armeria-brave5 is acceptable, as users will not need to modify their code. They only need to change the module name in the build file.
By removing armeria-brave, which has dependencies on zipkin, we can resolve the indirect dependency issue with armeria-brave6, which only includes the brave dependency.
Furthermore, we plan to deprecate the armeria-brave5 module by adding @Deprecated to the package-info.java file within the module. This will facilitate users in migrating to armeria-brave6.
Please, let me know if we have any missing points and what you think!
Thanks, Adrian!
There was a problem hiding this comment.
sounds like a good plan, I didn't think about package-info.java for deprecation.. that's a neat trick I'll add to my bag 👍
…onTest.java Co-authored-by: minux <[email protected]>
|
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? |
Yep. Let me push the patch to this PR. 😉 |
dependencies.toml
Outdated
| @@ -159,6 +159,8 @@ tomcat8 = "8.5.98" | |||
| tomcat9 = "9.0.85" | |||
| tomcat10 = "10.1.18" | |||
| xml-apis = "1.4.01" | |||
| zipkin = "3.0.6" | |||
| zipkin-reporter = "3.2.1" | |||
There was a problem hiding this comment.
since bumping, might as well!
| zipkin-reporter = "3.2.1" | |
| zipkin-reporter = "3.3.0" |
There was a problem hiding this comment.
At let me remove these dependencies since they are not used. 😉
minwoox
changed the title
| @@ -0,0 +1,29 @@ | |||
| /* | |||
| * Copyright 2019 LINE Corporation | |||
There was a problem hiding this comment.
I believe this wasn't a new file because we used to have it but we moved it to another module and back here again. 😆 So let me just keep the year.
| tasks.sourcesJar.from "${brave6ProjectDir}/src/main/resources" | ||
| tasks.javadoc.source "${brave6ProjectDir}/src/main/java" | ||
|
|
||
| tasks.register('generateSources', Copy.class) { |
There was a problem hiding this comment.
curious do we prefer to use the convention like
def generateSources = tasks.register('generateSources', Copy.class) {
...
}
tasks.named("compileJava").dependsOn(generateSources)
There was a problem hiding this comment.
Could you let me know if you are suggesting this convention? Or are you just asking? 😆
If you are suggesting it, could you share the reason, please? 😉 cc @jrhee17
There was a problem hiding this comment.
Maybe both, it's good to avoid some configuration time. And saw our native-image-config using similar way.
Notice this recently https://docs.gradle.org/current/userguide/task_configuration_avoidance.html
There was a problem hiding this comment.
Thank you for sharing the link. I wasn't aware that we were using an anti-pattern. 😓 Since we've employed this pattern across all our build files, let me create another PR to address this issue.
Avoid referencing a task by name. In the majority of cases, referencing a task by name is a fragile pattern and should be avoided. Although the task name is available on the TaskProvider, effort should be made to use references from a strongly typed model instead.
|
Thanks, @codefromthecrypt! |
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:
armeria-bravemoduleResult:
armeria-bravemodule has been fully removed. Please switch to usingarmeria-brave6along with additional dependencies:zipkin-reporterif you are utilizingAsyncZipkinSpanHandler.zipkinif you are utilizingZipkinSpanHandler.armeria-brave5if you wish to retain the previous module functionality.