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

Decreases switch statements and deprecates BytesMessageEncoder #250

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

codefromthecrypt
Copy link
Member

@codefromthecrypt codefromthecrypt commented Feb 12, 2024

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:

  • Add Encoding.encode(List<byte[]>) from the now deprecated BytesMessageEncoder.
  • Add Encoding.mediaType(), useful in internal and external HTTP senders.
  • Re-use the existing zipkin SpanBytesEncoder.forEncoding(Encoding) to reduce redundant switch logic.
  • Add MutableSpanBytesEncoder.forEncoding(Encoding) for Brave
  • Add MutableSpanBytesEncoder.create(Encoding, Tag<Throwable>) for Brave users who want an alt error tag.

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 codefromthecrypt changed the title deprecates BytesMessageEncoder for Encoding.encode and adds Encoding.… Adds Encoding.encode and mediaType; deprecates BytesMessageEncoder Feb 12, 2024
@codefromthecrypt codefromthecrypt changed the title Adds Encoding.encode and mediaType; deprecates BytesMessageEncoder Decreases switch statements and deprecates BytesMessageEncoder Feb 12, 2024
@codefromthecrypt codefromthecrypt marked this pull request as ready for review February 12, 2024 15:31
case JSON:
return JSON_V2;
case PROTO3:
throw new UnsupportedOperationException("PROTO3 is not yet a built-in encoder");
Copy link
Member Author

Choose a reason for hiding this comment

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

cool thing is that I have a pending change to support this. However, anyone using forEncoding could update into it without prior knowledge.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Member Author

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
Copy link
Member Author

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 codefromthecrypt merged commit bb3f5b7 into master Feb 13, 2024
3 checks passed
@codefromthecrypt codefromthecrypt deleted the less-switch branch February 13, 2024 01:40
@codefromthecrypt
Copy link
Member Author

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

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

Successfully merging this pull request may close these issues.

3 participants