No more ThreadLocals, delegate everything to 'palantir/tracing-java' #799

qinfchen · 2018-09-06T00:42:13Z

Before this PR

People might start adopting our shiny new palantir/tracing-java libraries, while keeping remoting3 ones on the classpath. This would be very bad because there would be two ThreadLocals trying to keep track of the current trace:

com.palantir.remoting3.tracing.Tracer <-- one ThreadLocal in here
com.palantir.tracing.Tracer <-- another ThreadLocal in here!

Concretely, all the clients in Atlas would use the remoting3 tracers but newer user-code would likely use tracing-java. This would make tracing information far less useful for debugging.

After this PR

com.palantir.remoting3.tracing.Tracer no longer maintains its own ThreadLocal, instead it delegates everything to com.palantir.tracing.Tracer.

Caveats

🚨The return type of Tracer#subscribe(String, SpanObserver) behaves differently now (it used to preserve reference equality and it no longer does).
🚨The Tracer#getAndClearTrace returns an incomplete copy of the trace. I think this is OK because that method was never usable externally because the returned type was not public.
Should there be a way of detecting this delegating functionality is on the classpath?

When migrating tritium to the rebranded tracing, @j-baker and @schlosna pointed out that that tracers are threadlocal and could potentially break tracing if remoting-tracers and conjure tracers were not on the same thread.
One proposal suggested by @iamdanfox was to delegate remotings tracing calls to conjure tracers. This pr is to demonstrate how the implementation would look like.
See more details here: palantir/tritium#100.

iamdanfox · 2018-09-06T14:11:01Z

tracing/src/main/java/com/palantir/remoting3/tracing/AsyncSlf4jSpanObserver.java

    }
-
-    @Override
-    public void doConsume(Span span) {


this was an implementation detail of the abstract AsyncSpanObserver class (which we no longer extend), so I think it's ok to delete.

iamdanfox · 2018-09-06T14:29:25Z

tracing/src/main/java/com/palantir/remoting3/tracing/AsyncSlf4jSpanObserver.java

-    @JsonSerialize(as = ImmutableZipkinCompatSpan.class)
-    @Value.Immutable
-    @Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE)
-    abstract static class ZipkinCompatSpan {


These classes have been completely deleted because they're an implementation detail that now exists in the new tracing-java class: com.palantir.tracing.AsyncSlf4jSpanObserver

iamdanfox · 2018-09-06T14:31:01Z

tracing/src/main/java/com/palantir/remoting3/tracing/Tracer.java

    private Tracer() {}

-    // Thread-safe since thread-local
-    private static final ThreadLocal<Trace> currentTrace = ThreadLocal.withInitial(() -> {


This is the critical line! remoting3 no longer maintains any ThreadLocal state, instead it's all delegated to the shiny new tracing-java.

qinfchen · 2018-09-06T14:45:22Z

tracing/src/main/java/com/palantir/remoting3/tracing/Tracer.java

-        currentTrace.remove();
-        MDC.remove(Tracers.TRACE_ID_KEY);
-        return trace;
+        com.palantir.tracing.Trace trace = com.palantir.tracing.Tracer.getAndClearTrace();


@iamdanfox, is this possible that we do something like this instead of having the ExposedTrace that lives inside the tracing-java package?

- com.palantir.tracing.Trace trace = com.palantir.tracing.Tracer.getAndClearTrace(); - return new Trace(ExposedTrace.isObservable(trace), ExposedTrace.getTraceId(trace)); + boolean isObservable = com.palantir.tracing.Tracer.isTraceObservable(); + String traceId = com.palantir.tracing.Tracer.getTraceId(); + com.palantir.tracing.Tracer.getAndClearTrace(); + return new Trace(isObservable, traceId);

I think there's a bit of a race condition, where we might get the isTraceObservable info, then someone changes stuff beneath our feet, then we get the traceId, then finally we clear it...

yeah, that is not gonna work.

In every case in this impl we should be delegating all state :)

j-baker · 2018-09-06T15:31:02Z

tracing/src/main/java/com/palantir/remoting3/tracing/CloseableTracer.java

     */
    public static CloseableTracer startSpan(String operation) {
-        Tracer.startSpan(operation);
+        com.palantir.tracing.Tracer.startSpan(operation);


can we just be like com.palantir.tracing.CloseableTracer.startSpan?

i kind of prefer using com.palantir.tracing.Tracer.startSpan(operation); to match up against com.palantir.tracing.Tracer.fastCompleteSpan(); few lines below since we can't get the instance of the com.palantir.tracing.CloseableTracer.

iamdanfox · 2018-09-06T15:52:21Z

tracing/src/main/java/com/palantir/remoting3/tracing/Tracers.java

 /** Utility methods for making {@link ExecutorService} and {@link Runnable} instances tracing-aware. */
 public final class Tracers {
-    /** The key under which trace ids are inserted into SLF4J {@link org.slf4j.MDC MDCs}. */
-    public static final String TRACE_ID_KEY = "traceId";


I don't think we should delete public static final fields like this, because if some consumer is depending on com.palantir.remoting3.tracingTracers.TRACE_ID_KEY then they'd suddenly break?

it doesn't seem like anyone is using it, https://github.com/search?p=2&q=Tracers.TRACE_ID_KEY&type=Code
i also did a quick search on the key internally and no internal consumer is using it.

I still don't think we should remove things like this when there isn't a clear motivation - what if there was a user that didn't appear in your search? Did you check OSS consumers too (e.g. atlasdb?)

j-baker · 2018-09-06T16:06:44Z

tracing/src/main/java/com/palantir/remoting3/tracing/Trace.java

 * not thread-safe and is intended to be used in a thread-local context.
 */
-final class Trace {
+public final class Trace {


does this need to become public?

Without this being public, one of the test utils fails to compile.

I find an alternative that involved some objects and casting too!

it is also kind of odd that we have a public static method that returns Trace and Trace is private.
https://github.com/palantir/http-remoting/pull/799/files#diff-7ee517b85058e51de91b9bf446ff7585L246

@qinfchen I don't have the original context, but one possible justification could be that this Trace class is useful for testing but shouldn't be used by consumers??

This reverts commit 1bd0d56.

* Expose HostMetricsRegistry record methods (#780) This is so that we can separately implement a HostMetricsSink for it (see #779) such that we can share host metrics when constructing both conjure and remoting3 clients * publish BOM for jaxrs-client (#789) * Excavator: Render CircleCI file using template specified in .circleci/template.sh (#791) * Upgrade OkHttp to 3.11.0. (#794) * AssertionErrors are converted into service exceptions with type internal (#727) * No more ThreadLocals, delegate everything to 'palantir/tracing-java' (#799) * Use BINTRAY_*_REMOTING envs (#802) The project's default bintray creds are currently set up to publish to `conjure-java-runtime`. Use these custom env vars to maintain ability to publish http-remoting. * Better behaviour in the presence of 429s (#786)

qinfchen added 2 commits September 5, 2018 20:21

[WIP] delegate all tracing calls to tracing-java

36e5dac

cleanup

f321e2c

qinfchen requested a review from a team as a code owner September 6, 2018 00:42

iamdanfox added 14 commits September 6, 2018 13:07

Use published versions of libraries

c78a24b

Everything compiles

edfa0d6

Convert#span actually works now

6a8471a

Implement Convert#toRemotingTrace

68d893a

All converters propagate nulls

420c0e8

Tracer#subscribe returns a remoting span observer

6ec78ae

Delete redundant fully-qualified names in TracersTest

d4dc7d6

Move longToPaddedHex function out of Tracers

332bbe1

Delete an asConjure method

9e6effd

@ignore one TracerTest

1beda24

Thanks checkstyle

6a02d6f

Reduce visibility of Convert class

f3724a3

Fix one Convert method

9c45298

Remove mavenLocal()

13fb17c

iamdanfox reviewed Sep 6, 2018

View reviewed changes

iamdanfox added 2 commits September 6, 2018 15:28

Move Utilities to test source set

0a5b297

Better javadoc and warnings

8a06a46

iamdanfox force-pushed the qchen/delegateTracing branch from 9cad433 to b009613 Compare September 6, 2018 14:28

iamdanfox reviewed Sep 6, 2018

View reviewed changes

iamdanfox changed the title ~~[WIP]delegate remoting tracing calls to java tracing~~ No more ThreadLocals, delegate everything to 'palantir/tracing-java' Sep 6, 2018

Restore CloseableTracerTest

bb252e2

iamdanfox force-pushed the qchen/delegateTracing branch from b009613 to bb252e2 Compare September 6, 2018 14:36

qinfchen commented Sep 6, 2018

View reviewed changes

Delete some unused fields and point to their new locations

badb51c

iamdanfox force-pushed the qchen/delegateTracing branch from f403597 to badb51c Compare September 6, 2018 14:46

j-baker reviewed Sep 6, 2018

View reviewed changes

qinfchen added 3 commits September 6, 2018 11:31

rearrange method orders and remove broken javadoc links

b20a71e

remove Tracers.TRACE_ID_KEY and use java-tracing TRACE_ID_KEY

1bd0d56

thanks checkstyle

0ac9f54

iamdanfox reviewed Sep 6, 2018

View reviewed changes

j-baker reviewed Sep 6, 2018

View reviewed changes

qinfchen requested a review from schlosna September 6, 2018 16:38

Revert "remove Tracers.TRACE_ID_KEY and use java-tracing TRACE_ID_KEY"

e9c0d1e

This reverts commit 1bd0d56.

ferozco approved these changes Sep 7, 2018

View reviewed changes

Don't make Trace public

adf0f74

iamdanfox force-pushed the qchen/delegateTracing branch from 1968b2f to adf0f74 Compare September 7, 2018 12:15

iamdanfox merged commit 5755d6f into develop Sep 7, 2018

iamdanfox deleted the qchen/delegateTracing branch September 7, 2018 12:20

iamdanfox mentioned this pull request Sep 7, 2018

Upgrade http-remoting 3.41.1 -> 3.43.0 to make tracing delegate nicely palantir/atlasdb#3494

Merged

qinfchen mentioned this pull request Sep 11, 2018

compatibility check against tracer remoting palantir/tracing-java#9

Closed

iamdanfox mentioned this pull request Oct 4, 2018

Use com.palantir.tracing.Tracer palantir/tritium#115

Closed

iamdanfox mentioned this pull request Oct 22, 2018

Tracing package should not change on major version bumps #580

Closed

svc-excavator-bot mentioned this pull request Sep 11, 2019

Excavator: Upgrades Baseline to the latest version #1230

Merged

No more ThreadLocals, delegate everything to 'palantir/tracing-java' #799

No more ThreadLocals, delegate everything to 'palantir/tracing-java' #799

Uh oh!

Conversation

qinfchen commented Sep 6, 2018 • edited by iamdanfox Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before this PR

After this PR

Caveats

Uh oh!

iamdanfox Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamdanfox Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinfchen Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamdanfox Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qinfchen commented Sep 6, 2018 •

edited by iamdanfox

Loading

iamdanfox Sep 6, 2018 •

edited

Loading

iamdanfox Sep 6, 2018 •

edited

Loading

qinfchen Sep 6, 2018 •

edited

Loading

iamdanfox Sep 6, 2018 •

edited

Loading