Inject span context into log4j2 2.13.2+ context.

[anuraaga]

This uses a very new API ContextDataProvider - no need for complex scope decorators, just reads the data from our context directly. While it means we miss out on a lot of log4j2 users, I think it's nice to have this for the latest version and we'd probably still want to provide a version possibly using ContextDataInjector

https://logging.apache.org/log4j/2.x/log4j-core/apidocs/org/apache/logging/log4j/core/ContextDataInjector.html

or using decorators.

I realized this doesn't follow the pattern for instrumentation. It's an instrumentation that uses a library's SPI to be automatically injected. So I think this means we want the agent to

• Inject shaded version into agent classloader
• Inject shaded or unshaded version into user classloader if it is not already present. We can't inject arbitrarily since otherwise it's a lot of overhead for no benefit if we return duplicate contextData from two providers, though it does still work.

Do we support this mode? Let me know if you have any suggestions.

[anuraaga]

Blargh, wish trace ID was a string! /cc @jkwatson

[jkwatson]

yes yes yes. me, too

[anuraaga]

Doh what am I doing, it already existed here 😭

https://github.com/open-telemetry/opentelemetry-java/blob/master/extensions/logging/log4j2_extensions/src/main/java/io/opentelemetry/contrib/logging/log4j2/TraceContextDataProvider.java

But this seems like instrumentation, not an SDK extension so should we move it over?

[anuraaga]

I added traceFlags too for parity with the SDK extension and co-authored-by @zenmoto to connect back to it. I think we can either

• Merge this and delete from SDK
• Close this without merging

Let me know if you have any thoughts, including @zenmoto :)

[iNikem]

Yeah, blurry border between otel-java extentions and this repo instrumentations...

[trask]

@open-telemetry/java-maintainers any thoughts about where this belongs? see above: #735 (comment)

[anuraaga]

@trask Any thoughts on how to use this sort of instrumentation from the agent?

[zenmoto]

My thinking on this was just that logging libraries are frequently used and may often be used outside of or separately from an agent. It probably fits better in the instrumentation project as long as it can be used independently. I apologize if I got that wrong.

[trask]

Hey @zenmoto, we recently expanded the scope of this group from auto-instrumentation to all kinds of instrumentation! Our bad for not noticing your PR to the opentelemetry-java repo.

That said, it's still not super clear whether modifying MDC properties is "instrumentation" or not, since it doesn't emit any telemetry.

We've also added a spring auto-configuration module to this repo recently (e.g. #729) that doesn't meet the bar of emitting telemetry though.

I'll take the action item to discuss with the opentelemetry-java team and come up with a proposal for what belongs where and come back to everyone on this issue.

@trask Any thoughts on how to use this sort of instrumentation from the agent?

My first thought is to find where log4j is doing the SPI lookup, and bytecode instrument that to inject our service.

My second thought is that if this is a common pattern of adding things to SPI lookup, we could conceivably instrument ServiceLoader.

[anuraaga]

@trask Thanks! Will think about the ServiceLoader issue.

My feedback for the discussion on the instrumentation vs SDK is any interaction with another library should probably go in this repo, mainly since we have good tooling related to it like latestDepTest. SDK should probably be generally primitives, so a generic scope decorator would go there. I suspect our typed tracers will end up there at some point too though not sure about that one.

[zenmoto]

Thanks, @trask. The goal here is also to extend this module to allow emitting of logs through the SDK and to the collector, but that full chain isn't in place yet. The collector has the service for accepting the logs, and I recently added a PR (open-telemetry/opentelemetry-java#1421) to start plumbing that through. It looks like that PR was premature, though, so I'm currently working on a PR to describe the API which I'll put up after @tigrannajaryan's log record spec PR merges (open-telemetry/opentelemetry-specification#694). So that's the wider context of where I'm trying to go here.

[trask]

Thanks for the context @zenmoto, I'm really looking forward to replacing my current approach of capturing logs as spans 😂.

I opened #751 to try to get agreement from all parties on what belongs in which repo.

In the meantime I don't want to leave this PR hanging, so will merge and we'll sort it all out after #751.

Thanks @anuraaga and @zenmoto!