pick up rebranded remoting change

[qinfchen]

remoting-tracing has been moved to https://github.com/palantir/tracing-java

[schlosna]

@qinfchen Do we have a gradle substitute rule for this for downstream consumers?

[schlosna]

actually, nevermind as we're changing packages as well so consumers can have both remoting3 & new tracing coexist peacefully, though we'll want all tracing using a single Tracer

[iamdanfox]

having both co-existing is actually quite concerning though, because if com.palantir.remoting3.Tracer#getTraceId() and com.palantir.tracing.Tracer#getTraceId() disagree then you'd be in a pretty weird state. Also the logging subscribers will probably only be attached to one of them!

[iamdanfox]

Can we take a few mins to plan how we do this seamlessly?

[j-baker]

Why would we rebrand tracing? Doesn't it just break all of our tracing?

[j-baker]

It's all based around thread locals, you need everyone to be using the same thread locals :(

[iamdanfox]

does this even need to be api?

[schlosna]

moving tracing to implementation

[schlosna]

Putting on hold per @iamdanfox & @j-baker discussion around coalescing from remoting3 to single Tracer

[iamdanfox]

OK here's a proposal:

1. let's release a version of com.palantir.remoting3.Tracer which delegates all methods to the shiny new com.palantir.tracing.Tracer.
2. add a static block to some class in tracing-java that throws if the remoting3 Tracers class is on the classpath (it would check implementationVersion and allow the ones that delegate)

[schlosna]

@iamdanfox 's proposal sounds reasonable to me. Should we have similar plumbing & state checks for remoting2 & remoting1 Tracers? We'll also want to manually migrate folks like AtlasDB CloseableTrace over as well.

https://github.com/palantir/http-remoting/blob/2.6.3/tracing/src/main/java/com/palantir/remoting2/tracing/Tracer.java
https://github.com/palantir/http-remoting/blob/1.1.2/tracing/src/main/java/com/palantir/remoting1/tracing/Tracer.java

[iamdanfox]

@schlosna from 3.43.0 onwards, remoting3's tracing delegates to the shiny new tracing jars so I think this is now ready to consider merging again!

[schlosna]

@iamdanfox we probably need to force folks to upgrade to remoting3 3.43.0+ before I can merge this.

[schlosna]

@iamdanfox I think I'm happy with this now. I added some defensive code here to try and prevent double tracing where folks still have an older http-remoting3 tracer.

[iamdanfox]

Ideally, I'd prefer not to require this class on the classpath. What about using reflection to just test for the existence of this class, then use .getPackage().getImplementationVersion()?

[schlosna]

Yeah, I can adjust this to test using reflection and fallback to reflection for slow path when old remoting3 is detected. I won’t keep that in tritium long term, but know there are some places that won’t get both excavated together, so want some transition time.

[schlosna]

Do you have a time frame you need this change & release?

[schlosna]

@iamdanfox Updated to use reflection check & fallback to reflection. Let me know if this looks good to you and I'll merge & tag a build

[schlosna]

There are significant performance slow downs using the new com.palantir.tracing.Tracer implementation as evidenced by jmh, see
#115 (comment):

Before, using com.palantir.remoting3:tracing on develop:

Benchmark                                          Mode  Cnt    Score    Error  Units
ProxyBenchmark.instrumentedWithEverything          avgt    5  882.844 ± 52.446  ns/op
ProxyBenchmark.instrumentedWithTracing             avgt    5  450.734 ± 74.964  ns/op
ProxyBenchmark.instrumentedWithMetrics             avgt    5  352.870 ±  5.791  ns/op
ProxyBenchmark.instrumentedWithPerformanceLogging  avgt    5   17.203 ±  0.932  ns/op
ProxyBenchmark.raw                                 avgt    5    3.001 ±  0.232  ns/op

After, using com.palantir.tracing.Tracer on ds/java-tracing:

Benchmark                                          Mode  Cnt     Score    Error  Units
ProxyBenchmark.instrumentedWithEverything          avgt    5  1447.724 ± 26.477  ns/op
ProxyBenchmark.instrumentedWithTracing             avgt    5   630.983 ± 18.414  ns/op
ProxyBenchmark.instrumentedWithMetrics             avgt    5   355.538 ±  9.785  ns/op
ProxyBenchmark.instrumentedWithPerformanceLogging  avgt    5    17.266 ±  0.511  ns/op
ProxyBenchmark.raw                                 avgt    5     2.912 ±  0.061  ns/op