Implement MDC auto-instrumentation for log4j2

[mateuszrzeszutek]

Resolves #1196

[trask]

Hey @mateuszrzeszutek, have you seen the log4j2 instrumentation we used to have (#803)?

It was capturing the logging data instead of injecting MDC values, but we will be bringing it back once we have OpenTelemetry logging API.

The reason I mention, is we could potentially use the same instrumentation points to inject the MDC values.

This is what @anuraaga did in #1180 for logback.

A couple advantages to this approach would be to support older log4j2 versions, and potentially share instrumentation points for both MDC and logging API capture in the future.

[anuraaga]

With logback, the need to modify the XML to reuse library instrumentation made it impractical to reuse in the auto. I'm hoping for log4j though we actually can just use the library as is.

I was thinking if doing it using the technique in #1172 (actually log4j is the reason I created that PR but am using it first in AWS sdk since closer to home :) ).

To support older versions, I would add library instrumentation that uses the old context SPI of log4j and use that in auto for the older versions.

Does this make sense?

[iNikem]

Is there an older SPI for old Log4J versions? Including Log4J 1.x?

[anuraaga]

Yeah there's this old one

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

It's still only 2.7+ so indeed it still doesn't provide full coverage so we may not like this option anyways.

I don't think any approach can apply to both 2.x and 1.x directly, they're very different APIs FWIU.

[iNikem]

I did not mean common SPI for 2.x and 1.x :) I was wondering if there is any SPI for 1.x.

For 2.x I am comfortable atm to only support 2.7+

[anuraaga]

Ah got it - yeah not aware of any SPI for 1.x unfortunately

[anuraaga]

Merged #1172 so if we want to use the SPI directly in auto instrumentation, we can :)

[mateuszrzeszutek]

Merged #1172 so if we want to use the SPI directly in auto instrumentation, we can :)

Thanks! Just injecting the service file looks much better.

Hey @mateuszrzeszutek, have you seen the log4j2 instrumentation we used to have (#803)?
It was capturing the logging data instead of injecting MDC values, but we will be bringing it back once we have OpenTelemetry logging API.
The reason I mention, is we could potentially use the same instrumentation points to inject the MDC values.

I thought about it but I prefer using log4j's built in SPI capabilities in this case - we're using log4j types that were introduced exactly for the purpose of context data injection, so the instrumentation code looks quite simple and clean (esp. in case of 2.13.2+). Compared to that, previous AbstractLogger-using one looks much more complicated and fickle - how many log and logMessage does a logger have and which one is the correct one?

I believe that it'd probably be cleaner to have a separate instrumentation for purposes of OTel logging API - and if not let's refactor it then. We don't really know the final shape of it yet anyway.

[anuraaga]

Looks swell, thanks! Just one small point about the test

[anuraaga]

Thanks!

[iNikem]

log4j/log4j-2.13.2/library/README.md needs update, et least traceFlags -> sampled

[iNikem]

Why do we need both instrumentation for 2.7 and for 2.13.2? Cannot we just use one for all version 2.7+?

[mateuszrzeszutek]

log4j/log4j-2.13.2/library/README.md needs update, et least traceFlags -> sampled

True, I missed that. Thanks!

Why do we need both instrumentation for 2.7 and for 2.13.2? Cannot we just use one for all version 2.7+?

We could use a single instrumentation for all 2.7+ versions - I just thought that it would be a bit more elegant to use built-in SPI if it's possible.

[anuraaga]

I don't know if there is a performance difference between the two, if not it could make sense for auto instrumentation to target 2.7.

But for library instrumentation, the 2.7 approach is even worse than gRPC SPI by requiring a system property. I have personally felt the annoyance before and appreciated they fixed the SPI - I think we do need the library instrumentation for 2.13 for those users.

[iNikem]

Agree that library instrumentation can stay as it is, for 2.13.2+ only. And now thinking about it... both ways for auto instrumentation are Ok: 1 or 2 instrumentations. @mateuszrzeszutek what do you prefer?

[mateuszrzeszutek]

Agree that library instrumentation can stay as it is, for 2.13.2+ only. And now thinking about it... both ways for auto instrumentation are Ok: 1 or 2 instrumentations. @mateuszrzeszutek what do you prefer?

I'm fine with both solutions actually. If anybody here has a strong preference towards one of them I'll change this PR accordingly.

[anuraaga]

I'm fine with either too :) I'll let @iNikem merge