Getting rid of ThreadLocals

[robertbraeutigam]

At this point the SDK is riddled with usages of ThreadLocals. Sometimes this is easier to spot, when using current() or similar methods. Sometimes it is implicit and very hard to spot.

Using ThreadLocals makes it very difficult and error-prone to use this SDK in reactive designs, when using CompletableFutures, or when using it in Scala with "cats-effect" or similar effect library.

I propose that that core library should not use ThreadLocal at all. At most it should be a (clearly marked) optional layer on top.

[jkwatson]

I think "riddled with" is overstating it quite a bit [I see precisely 9 instantiations of ThreadLocals in the entire codebase (outside of tests)]. Almost all of the ThreadLocal usage are local caches, and hence not critical to functionality, just performance.

You are free to use a different ContextStorage implementation that doesn't use ThreadLocal if you want to. That's really the only functionally-relevant ThreadLocal that I know of.

[jkwatson]

In addition, I have never had any problems using our API and SDK with CompletableFuture. You just need to make sure that the ExecutorService that is used is one that is wrapped with the Context wrappers, and everything works fine.

[robertbraeutigam]

Thanks for answering. Yes, "riddled" might be a bad way to put it. I wanted to say any invocation, even harmless sounding ones may include threadlocals under the hood. Which is a problem, if you want to avoid them. Just now I'm working on a problem where a threadlocal was used in startSpan, because the parent was not set. Now spans appear under traces that have nothing to do with it.

Let me rephrase the problem. Seems to me (correct me if I'm wrong), that the SDK assumes, that a span will run on a single thread. This is probably the case for conventional Java applications, but reactive apps, CompletableFutures, cats-effect, etc. do not generally give you this guarantee.

In other words if you want to have a span for foo().thenCompose(bar()), how do you do it? You have no idea where foo is running, you have no idea where bar is running. They may be running on the same thread maybe not. Maybe running on the same thread pool, maybe not. What if bar wants to add some attributes, or start a child-span? Seems to me the only reasonable way to do this is to pass some object along. This should be the easy default. But it seems it's not. How would you do it?

[jkwatson]

We absolutely, in no way, assume that java applications will run on a single thread. This is why our Context implementation has many methods to assist with context propagation into other threads.

If you are going to write manual instrumentation, you need to deeply understand how your application framework uses threads, and utilize the utilities we provide on the Context interface to make sure that the context is properly propagated. Our APIs will not do this for you, but they will give you the tools you need, if you spend the time to understand them.

Context propagation is very tricky, and there is no "one size fits all" solution, which is why we provide you the tools, but do not try to solve every possible use-case.

If you want auto-instrumentation that will manage internal context propagation for you, please use the opentelemetry java-agent, which manages context propagation for most common frameworks. If you want to do manual instrumentation, please study the Context interface and learn the utilities it provides, before assuming that we don't know what we're doing.

[jkwatson]

If your goal is completely avoid the use of ThreadLocals for context storage, you have two options:

• Write a different ContextStorage implementation that fulfills your needs.
• Manually propagate your Context instances where you want them via method parameters, or some such. This is much like the way that all of golang works.

Both of these options will involve a lot of work and will be very error prone. I recommend understanding how your framework spawns threads and use our Context utilities to make sure the spawning of them properly propagates the Context into the spawned threads.

[trask]

you're not the primary use-case we have designed for

I don't necessarily think this is true. We did discuss reactive programming at length when designing the OpenTelemetry context.

@robertbraeutigam you'll see this reflected in all of the APIs accepting an explicit Context, and you can confirm this by checking that the OpenTelemetry SDK never calls Context.makeCurrent(), making thread locals entirely opt-in by users.

@robertbraeutigam it would help if you could post an example of where ThreadLocals are leaking in to your application, maybe they are coming from OpenTelemetry instrumentation as opposed to the OpenTelemetry SDK?

[jkwatson]

@trask if you don't explicitly override the parent, the SDK will ask for Span.current() as the parent when the span starts. So, this one is not necessarily obvious. And, I think my statement stands, that our easiest API usage does assume that you're being using the ThreadLocal implementation of ContextStorage, and if you aren't careful, it'll be used to parent you spans implicitly.

We definitely make it possible to not use the ThreadLocals, but it's not the easiest, most obvious path through the API.

[trask]

@trask if you don't explicitly override the parent, the SDK will ask for Span.current() as the parent when the span starts.

Sorry, part of my point was that if you never call makeCurrent, then this usage has no effect since it's always the root (empty) context.

[johnpoth]

@robertbraeutigam thanks for this thread ! I am having exactly the same problem. Can't really add anything more than what you've said.

@jkwatson @trask could you show how to use the library without the ThreadLocals ?

Thanks !

[trask]

hi @johnpoth! all of the OpenTelemetry Java APIs allow you to pass in Context explicitly (so that you don't need to use ThreadLocals), e.g.

opentelemetry-java/api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java

Lines 99 to 110 in ee7fd27

	* void doWork(Span parent) {
	* Span childSpan = tracer.spanBuilder("MyChildSpan")
	* .setParent(Context.current().with(parent))
	* .startSpan();
	* childSpan.addEvent("my event");
	* try {
	* doSomeWork(childSpan); // Manually propagate the new span down the stack.
	* } finally {
	* // To make sure we end the span even in case of an exception.
	* childSpan.end(); // Manually end the span.
	* }
	* }

[johnpoth]

@trask Thank you so much for your response!

I see that your snippet uses Context.current() which does call some ThreadLocalStorage. Will that pose some problems? In my case, some Spans will start and end in different threads.

[trask]

ah, sorry, you don't need to call Context.current(), just pass in the Context instance that you are passing around explicitly

[johnpoth]

@trask Right hence my confusion. The methods in Context to create a new Context Object seem to all refer to the ThreadLocal stuff. How to handle the Context lifecycle without the ThreadLocals ? Thank you so much !

[jkwatson]

The only way I know of might be to use the log4j MDC, but that also uses a ThreadLocal as the storage.

[johnpoth]

@jkwatson thank you so much for your response! I'll check that out. In the meantime I've let myself use makeCurrent() for that purpose when I start and end spans:

`
try (Scope ignored = context.makeCurrent()) {

Span span = builder.setParent(context).startSpan();
return context.with(span);

}
`

I've noticed however that the logEvent is triggered when startSpan() is called and so I get 0000 for traceId and SpanId in the logs on the first run. I figure startSpan() should be called after something like context.with(span).makeCurrent() but it's sort of a chicken and the egg problem as the Span is only available once startSpan() is called..... Thanks!

[johnpoth]

@jkwatson @trask so I've finally gotten rid of all the ThreadLocal stuff in the code base! However I've noticed [1] that the many libraries and frameworks supported in the OpenTelemetry ecosystem do rely on the OpenTelemetry Context being set in the ThreadLocal storage.

Is there a workaround for that ? I feel like the answer is no but you never know :)

Thanks !

[1] https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/bootstrap/Java8BytecodeBridge.java#L22

[jkwatson]

This would be a question for the instrumentation side of things. My guess is that it would take quite a bit of re-work of the instrumentation, but it's not my area of expertise, so I recommend asking over in the instrumentation repository.

[trask]

Is there a workaround for that?

it depends, can you be more specific about the issue you are facing?