Skip to content
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

Remove armeria-brave module #5453

Merged
merged 4 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion brave/brave/build.gradle
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
dependencies {
api project(':brave5')
api project(':brave6')
// Brave 5.x depended on zipkin-reporter-brave, but Brave 6.x has no
// dependencies. We add back dependencies that allow ZipkinSpanHandler to
// compile. This allows people to migrate without managing these
// dependencies on their own. Those who want a dependency-free integration
// can use armeria-brave6.
api libs.zipkin.reporter.brave // For AsyncZipkinSpanHandler (native Brave)
api libs.zipkin // For ZipkinSpanHandler (uses Zipkin core types)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add this to brave6 module instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@codefromthecrypt codefromthecrypt Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. for convenience, pin reporter (as done now in armeria-brave) and know this needs to be revisited in the event of a reporter 4?
  2. if you do that, also add the pretty large io.zipkin2:zipkin dep

over to you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package com.linecorp.armeria.brave;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.when;

import org.junit.jupiter.api.Test;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoSettings;
import org.mockito.quality.Strictness;

import brave.handler.MutableSpan;
import brave.handler.SpanHandler;
import brave.handler.SpanHandler.Cause;
import brave.propagation.TraceContext;
import zipkin2.reporter.BytesMessageSender;
import zipkin2.reporter.Encoding;
import zipkin2.reporter.Reporter;
import zipkin2.reporter.brave.AsyncZipkinSpanHandler;
import zipkin2.reporter.brave.ZipkinSpanHandler;

/**
* This tests that zipkin-reporter-brave types work without adding any explicit
* dependencies.
*/
@MockitoSettings(strictness = Strictness.LENIENT)
class CompilationTest {
codefromthecrypt marked this conversation as resolved.
Show resolved Hide resolved

TraceContext context = TraceContext.newBuilder().traceId(1).spanId(1).build();
MutableSpan span = new MutableSpan();
@Mock
BytesMessageSender sender;

@Test
void zipkinSpanHandler() {
final SpanHandler handler = ZipkinSpanHandler.create(Reporter.NOOP);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this test should give 100pct confidence that a dep change won't break without knowing

assertThat(handler.begin(context, span, null)).isTrue();
assertThat(handler.end(context, span, Cause.FINISHED)).isTrue();
}

@Test
void asyncZipkinSpanHandler() {
when(sender.encoding()).thenReturn(Encoding.JSON);

try (AsyncZipkinSpanHandler handler = AsyncZipkinSpanHandler.newBuilder(sender).build()) {
assertThat(handler.begin(context, span, null)).isTrue();
assertThat(handler.end(context, span, Cause.FINISHED)).isTrue();
}
}
}
12 changes: 11 additions & 1 deletion dependencies.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ assertj = "3.25.2"
awaitility = "4.2.0"
blockhound = "1.0.8.RELEASE"
bouncycastle = "1.70"
brave5 = "5.18.0"
brave5 = "5.18.1"
brave6 = "6.0.0"
brotli4j = "1.15.0"
bucket4j = "7.6.0"
Expand Down Expand Up @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since bumping, might as well!

Suggested change
zipkin-reporter = "3.2.1"
zipkin-reporter = "3.3.0"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At let me remove these dependencies since they are not used. 😉

# Ensure that we use the same ZooKeeper version as what Curator depends on.
# See: https://github.com/apache/curator/blob/master/pom.xml
# (Switch to the right tag to find out the right version.)
Expand Down Expand Up @@ -1364,6 +1366,14 @@ version.ref = "java-websocket"
module = "xml-apis:xml-apis"
version.ref = "xml-apis"

[libraries.zipkin]
module = "io.zipkin.zipkin2:zipkin"
version.ref = "zipkin"

[libraries.zipkin-reporter-brave]
module = "io.zipkin.reporter2:zipkin-reporter-brave"
version.ref = "zipkin-reporter"

[libraries.zookeeper]
module = "org.apache.zookeeper:zookeeper"
version.ref = "zookeeper"
Expand Down
Loading