openzipkin / zipkin-reporter-java

Shared library for reporting zipkin spans on transports such as http or kafka
Apache License 2.0
126 stars 70 forks source link

Decreases switch statements and deprecates BytesMessageEncoder #250

Closed codefromthecrypt closed 9 months ago

codefromthecrypt commented 9 months ago

I noticed in our senders and reporters there were multiple internal switch statements to get functions for a specific encoding. This is worse externally for zipkin-reporter-brave, as call sites need to duplicate switch statements, and also update them in order to allow another encoding. This is the case with PROTO3, for example, which is not yet supported for brave spans.

Here are the main changes:

With these changes in place, external senders such as those defined in spring-boot, can inherit sensible defaults without maintenance.

This acknowledges some specialized senders (such as okhttp) won't use Encoding.encode(List<byte[]>). However, this change makes commodity senders a lot easier to write and configure, as they can now be driven by just Encoding and an endpoint.

codefromthecrypt commented 9 months ago

seems like spring boot need to release soon, so I will check on this now. hopefully, looks ok to the team, but if you have a chance to take a look, please do!

codefromthecrypt commented 9 months ago

Here are some notable diffs.

-       HttpSender() {
-               super(Encoding.JSON);
+       HttpSender(Encoding encoding) {
+               super(encoding);
        }

        @Override
@@ -74,7 +74,7 @@ abstract class HttpSender extends BytesMessageSender.Base {
                if (this.closed) {
                        throw new ClosedSenderException();
                }
-               byte[] body = BytesMessageEncoder.JSON.encode(encodedSpans);
+               byte[] body = this.encoding.encode(encodedSpans);
                HttpHeaders headers = getDefaultHeaders();
                if (needsCompression(body)) {
                        body = compress(body);
@@ -86,7 +86,7 @@ abstract class HttpSender extends BytesMessageSender.Base {
        HttpHeaders getDefaultHeaders() {
                HttpHeaders headers = new HttpHeaders();
                headers.set("b3", "0");
-               headers.set("Content-Type", "application/json");
+               headers.set("Content-Type", this.encoding.mediaType());
                return headers;
        }
                @Bean
                @ConditionalOnMissingBean(value = MutableSpan.class, parameterizedContainer = BytesEncoder.class)
                @ConditionalOnEnabledTracing
-               BytesEncoder<MutableSpan> braveSpanEncoder() {
-                       return new JsonV2Encoder();
+               BytesEncoder<MutableSpan> braveSpanEncoder(Encoding encoding) {
+                       return MutableSpanBytesEncoder.forEncoding(encoding);
                }

                @Bean
@@ -157,8 +158,8 @@ class ZipkinConfigurations {
                @Bean
                @ConditionalOnMissingBean(value = Span.class, parameterizedContainer = BytesEncoder.class)
                @ConditionalOnEnabledTracing
-               BytesEncoder<Span> zipkinSpanEncoder() {
-                       return SpanBytesEncoder.JSON_V2;
+               BytesEncoder<Span> zipkinSpanEncoder(Encoding encoding) {
+                       return SpanBytesEncoder.forEncoding(encoding);
                }
codefromthecrypt commented 9 months ago

thanks for the look @anuraaga @reta! I need to add the long overdue brave proto encoder next to avoid folks running into a wall. It is a somewhat mechanical port of what's in zipkin and brave already..