openzipkin / brave

Java distributed tracing implementation compatible with Zipkin backend services.
Apache License 2.0
2.35k stars 714 forks source link

removes IpLiteral dependency from ZipkinV2JsonWriter #1414

Closed codefromthecrypt closed 5 months ago

codefromthecrypt commented 5 months ago

This removes a dependency on IpLiteral.detectFamily from json writing, and adds a benchmark to make sure it doesn't hurt performance. While a nit, this clarifies that it has already happened, so that a proto encoder port will not think it needs to re-detect also.

Before

Benchmark                                                              Mode     Cnt    Score   Error   Units
ZipkinV2JsonWriterBenchmarks.write_bigClientSpan:gc.alloc.rate.norm  sample      15   24.104 ± 0.011    B/op
ZipkinV2JsonWriterBenchmarks.write_bigClientSpan:p0.99               sample            0.791           us/op
ZipkinV2JsonWriterBenchmarks.write_serverSpan:gc.alloc.rate.norm     sample      15   24.040 ± 0.006    B/op
ZipkinV2JsonWriterBenchmarks.write_serverSpan:p0.99                  sample            0.333           us/op

After

Benchmark                                                              Mode     Cnt    Score   Error   Units
ZipkinV2JsonWriterBenchmarks.write_bigClientSpan:gc.alloc.rate.norm  sample      15   24.101 ± 0.011    B/op
ZipkinV2JsonWriterBenchmarks.write_bigClientSpan:p0.99               sample            0.750           us/op
ZipkinV2JsonWriterBenchmarks.write_serverSpan:gc.alloc.rate.norm     sample      15   24.039 ± 0.007    B/op
ZipkinV2JsonWriterBenchmarks.write_serverSpan:p0.99                  sample            0.333           us/op
codefromthecrypt commented 5 months ago

hmm this seems unrelated

 Error:  Failures: 
Error:    ThreadLocalSpanClassLoaderTest.currentTracer_basicUsage_unloadable:35 class brave.propagation.ThreadLocalSpanClassLoaderTest$ExplicitTracerBasicUsage includes state that couldn't be garbage collected
codefromthecrypt commented 5 months ago

Thanks for the look, folks!