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

brave: adds PROTO3 encoding #252

Merged
merged 5 commits into from
Feb 13, 2024
Merged

brave: adds PROTO3 encoding #252

merged 5 commits into from
Feb 13, 2024

Conversation

codefromthecrypt
Copy link
Member

@codefromthecrypt codefromthecrypt commented Feb 13, 2024

This adds MutableSpanEncoder.PROTO3 similar to what's available for zipkin2.Span. The code and tests are based on similar inside zipkin and brave. This is added to the reporter, not brave, to reduce version lock-up for a useful, but more rare encoding type.

Benchmark                                                                               Mode     Cnt     Score     Error   Units
MutableSpanBytesEncoderBenchmarks.encode_bigClientSpan_json:gc.alloc.rate.norm        sample      15  584.183 ± 0.045    B/op
MutableSpanBytesEncoderBenchmarks.encode_bigClientSpan_json:p0.99                     sample            1.166           us/op
MutableSpanBytesEncoderBenchmarks.encode_bigClientSpan_proto:gc.alloc.rate.norm       sample      15  472.141 ± 0.006    B/op
MutableSpanBytesEncoderBenchmarks.encode_bigClientSpan_proto:p0.99                    sample            1.042           us/op
MutableSpanBytesEncoderBenchmarks.encode_serverSpan_json:gc.alloc.rate.norm           sample      15  288.054 ± 0.008    B/op
MutableSpanBytesEncoderBenchmarks.encode_serverSpan_json:p0.99                        sample            0.417           us/op
MutableSpanBytesEncoderBenchmarks.encode_serverSpan_proto:gc.alloc.rate.norm          sample      15  200.052 ± 0.006    B/op
MutableSpanBytesEncoderBenchmarks.encode_serverSpan_proto:p0.99                       sample            0.417           us/op
MutableSpanBytesEncoderBenchmarks.sizeInBytes_bigClientSpan_json:gc.alloc.rate.norm   sample      15    0.043 ± 0.011    B/op
MutableSpanBytesEncoderBenchmarks.sizeInBytes_bigClientSpan_json:p0.99                sample            0.334           us/op
MutableSpanBytesEncoderBenchmarks.sizeInBytes_bigClientSpan_proto:gc.alloc.rate.norm  sample      15    0.028 ± 0.004    B/op
MutableSpanBytesEncoderBenchmarks.sizeInBytes_bigClientSpan_proto:p0.99               sample            0.250           us/op
MutableSpanBytesEncoderBenchmarks.sizeInBytes_serverSpan_json:gc.alloc.rate.norm      sample      15    0.013 ± 0.002    B/op
MutableSpanBytesEncoderBenchmarks.sizeInBytes_serverSpan_json:p0.99                   sample            0.125           us/op
MutableSpanBytesEncoderBenchmarks.sizeInBytes_serverSpan_proto:gc.alloc.rate.norm     sample      15    0.008 ± 0.001    B/op
MutableSpanBytesEncoderBenchmarks.sizeInBytes_serverSpan_proto:p0.99                  sample            0.125           us/op

Fixes #178

This adds `MutableSpanEncoder.PROTO3` similar to what's available for
`zipkin2.Span`. The code and tests are based on similar inside zipkin
and brave. This is added to the reporter, not brave, to reduce version
lock-up for a useful, but more rare encoding type.

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

This adds 24KB to the brave jar, which I think is not enough for people to fuss about.

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

Note: write speed is less than json, which might not seem intuitive. However, there is some work to do converting string IPs to their binary representation. Similarly, we need to decode the hex IDs. So, if you were expecting dramatically different encoding speed, bear this in mind. It is in the same ballpark as JSON even if on the wire will be smaller and of course other advantages of proto. I'll post benchmarks in a few minutes when they complete.

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Feb 13, 2024

actually I got found the biggest issue and now proto is not as slow encoding. Regardless, this isn't new just something I noticed looking closely. zipkin-core cheats by coercing IPs to bytes in the critical path.

Someone who wants to mess around and improve the ipv6 writer more, but anyway encoding is off-thread. The most important latency is sizeOfBytes which is a guard to make sure a span isn't too big to send. This is faster in proto a lot.

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

updating the ipv6 writing code to latest guava (as the previous snippet copied allocated a lot..) did the trick. proto is now as fast or faster than json as one might expect.

@anuraaga I'm done mucking around if you have time for a look. only other thing I'll do here is bump to brave 6.0.1 which is releasing now.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt merged commit be50820 into master Feb 13, 2024
3 checks passed
@codefromthecrypt
Copy link
Member Author

thanks for all the reviews, @anuraaga! I'll do a quick test with micrometer+spring boot prior to cutting the release.

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.

Support proto in AsyncZipkinSpanHandler
2 participants