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

Provide a bridge for Zipkin2 Sender interfaces to be used in AsyncReporter in Zipkin 3 #269

Open
iparadiso opened this issue May 29, 2024 · 2 comments

Comments

@iparadiso
Copy link

iparadiso commented May 29, 2024

Zipkin 3 comes with new interfaces such as ByteMessageSender rather than Sender that apps will need to upgrade to when upgrading from Zipkin 2 to Zipkin 3. In Zipkin 3.0, the AsyncReporter no longer supports a builder API for Sender, which prevents transitive libraries that integrate with Zipkin 2 types from being used in an app that migrates to Zipkin 3.

No deprecation of this builder API was provided in advance for users to transition away from this breaking change in 3.0.

Feature

Support a temporary/deprecated API in AsyncReporter to accept a Sender to provide a smoother migration for apps with transitive dependencies on lower versions of Zipkin that app owners cannot upgrade.

Rationale

It will help Java apps migrate from Zipkin 2 to 3. One common case is Spring Boot 3.2 to Spring Boot 3.3.

Example Scenario

Consider the case where an app has upgraded to Zipkin3, but this app may also be loading transitive libraries that are using Zipkin 2 types and creating their own Sender and AsyncReporter. When these libraries are loaded in an application that has Zipkin3 on the classpath, The Sender is accepted in the AsyncReporter builder so get an AbstractMethodError error.

Example code to illustrate this problem. A library has the following dependencies

api 'io.zipkin.reporter2:zipkin-reporter:2.10.2'
api 'io.zipkin.reporter2:zipkin-sender-okhttp3:2.10.2'

And creates their own Reporter implementation using the okttp sender.

public Reporter reporter(ZipkinTracingConfig config, ReporterMetrics zipkinTracerReporterMetrics) {
            Sender sender = OkHttpSender.newBuilder()
                    .endpoint(config.httpReporterUrl())
                    .connectTimeout(config.httpReporterConnectTimeout())
                    .readTimeout(config.httpReporterReadTimeout())
                    .build();
            return AsyncReporter.builder(sender)
                    .metrics(zipkinTracerReporterMetrics)
                    .queuedMaxSpans(config.httpReporterMaxQueuedSpans())
                    .messageTimeout(config.httpReporterMessageTimeout(), TimeUnit.MILLISECONDS)
                    .build();
    }

When Zipkin 3 is on the classpath, this code throws the following error

Caused by: AbstractMethodError: Receiver class OkHttpSender does not define or inherit an implementation of the resolved method 'abstract Encoding encoding()' of interface BytesMessageSender.


    at AsyncReporter$Builder.<init>(AsyncReporter.java:95)


    at AsyncReporter.builder(AsyncReporter.java:55)


    at AsyncReporter.builder(AsyncReporter.java:50)


    at ZipkinTracingModule.reporter(ZipkinTracingModule.java:134)

@codefromthecrypt codefromthecrypt transferred this issue from openzipkin/zipkin Aug 3, 2024
@codefromthecrypt
Copy link
Member

Basically, the best way is to upgrade to brave 6 and eliminate the reporter dep.

Keep in mind that Brave 6 has no dependency on zipkin-reporter. So, the actual upgrade path is to move to brave 6 first (which decouples you from zipkin-reporter versions). I think this has been mentioned on various issues around this topic.

In the event more is needed here, it isn't likely the code will go backwards. Reporter 3 will stay as it is, it is allowed to break compat as it is a major version bump.

In the case people have the labor and karma to do a one-off 2.x release to add the methods in 3, they could. However, this is a tall ask because we don't maintain release branches. So this would need to create one, then make a release from that branch which would currently be manual.

@codefromthecrypt
Copy link
Member

here's the advice also on spring-projects/spring-boot#40914 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants