Opentelemetry-java: Context change interceptor

[bogdandrutu]

This feature was requested initially in https://github.com/open-telemetry/opentelemetry-java/issues/295 and moved here because all other points are addressed.

One of the goal is to support log correlation, but at one point @kbrockhoff mentioned that what OpenCensus implemented https://github.com/census-instrumentation/opencensus-java/tree/master/contrib/log_correlation/log4j2 is not good, can you please clarify?

[bogdandrutu]

@tylerbenson you asked for a prototype on how to build a real interceptor for Context changes, see https://github.com/open-telemetry/opentelemetry-java/pull/923

[bogdandrutu]

Ping @kbrockhoff

[carlosalberto]

Closing due to lack of activity. @kbrockhoff Please re-open if/as needed ;)

[trask]

@carlosalberto I think this is important, can we re-open?

The only way to create an OpenTelemetry scope listener currently is by creating a class with a very special name io.grpc.override.ContextStorageOverride. This seems like a very odd public API surface that we are exposing, and what happens if there is already one of these classes on the classpath?

Scope listeners are important for logging MDC integrations (to populate spanId and traceId into logging MDC).

[carlosalberto]

Marking this initially as after-ga. @trask let me know if you think we ought to have this for GA instead.

[trask]

The main use case we need this for is logging MDC. Since logging is out of scope for GA, I think this is ok, at least from instrumentation repo perspective.

[HaloFour]

Please reconsider addressing this pre-GA. I understand that logging isn't included in GA, but I think it's critically important for adopters of OTel to be able to correlate logs with tracing.

I am currently also looking at the ContextStorageProvider SPI route to see if I can implement this but it seems like a brittle solution.

[jkwatson]

@HaloFour "after ga" in this case mostly means that the current maintainers don't think we can get to it before GA, and that it's not a strict requirement for GA. If you're interested in contributing an implementation, we certainly can review and it and work on getting it merged before GA.

[HaloFour]

@jkwatson

Understood. That backlog never seems to get shorter, does it? 😄

I'm still toying around but I was able to move my MDC-managing logic to a ContextStorage implementation provided by SPI and my unit tests all pass, so that's a good sign. I'll have to put more thought into how something like this might be generalized into an API that doesn't involve SPI.

As an aside, I noticed that the default io.opentelemetry.context.ThreadLocalContextStorage sets the THREAD_LOCAL_STORAGE to Context.root() in a static constructor, but that's only going to set it for whatever thread happens to be executing that constructor. If you want the default value to always be Context.root() if the thread local hasn't been set you'll want the following:

private static final ThreadLocal<Context> THREAD_LOCAL_STORAGE = ThreadLocal.withInitialValue(() -> Context.root());

Although if you're going to gracefully handle the thread local (and ContextStorage#current) returning null it's probably worth not bothering with an initial value.

[anuraaga]

@HaloFour We agree that logging is really important :) So we already created instrumentation for logback and log4j - does it satisfy your use case?

https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/master/instrumentation/logback/logback-1.0.0

https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/master/instrumentation/log4j

[HaloFour]

@anuraaga

Maybe, I've not yet figured out how I can use the instrumentation or the Java agent since I'm not using the OpenTelemetry SDK tracer.

I think using the ContextStorageProvider SPI interface resolves my use case for now in that it gives me a way to hook into when a Context is attached or detached. Since it requires rewriting ThreadLocalContextStorage it feels a little involved to be considered part of the API but it is relatively straightforward and it seems to do the trick.

[anuraaga]

@HaloFour Instrumentation does not depend on the SDK, it only uses the OTel API. I believe you should be able to use those ok - it would be very helpful if you could check out the readme for the logging library you use (they don't require agent :) and see how it goes for you!

[HaloFour]

@jkwatson

I put up a WIP PR with a prototype API to allow registering a callback when the context changes without having to use SPI to replace the underlying ContextStorage implementation. It does put an onus on implementations of ContextStorage to handle registration and invocation of that callback but I think a reference implementation of that callback.

https://github.com/open-telemetry/opentelemetry-java/pull/1961