-
Notifications
You must be signed in to change notification settings - Fork 867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add log4j2 appender javaagent instrumentation #4944
Add log4j2 appender javaagent instrumentation #4944
Conversation
} | ||
|
||
@Override | ||
public void transform(TypeTransformer transformer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had forgotten about the log4j appender we have. Is the reason we can't use logic from the library instrumentation to support old versions of log4j? Log transformation is much more complicated than ID injection so we shouldn't duplicate instrumentation I think if at all possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, there are some things we can share and some we can't due to 2.0 vs 2.16. I'll send a follow-up that extracts the part(s) that are shareable so we can see if it's worth an extra module for those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way - do we need to support <2.16? :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me think longer on this, I'm actually not sure...
I will update this PR to target 2.16 and share mapping code with the library instrumentation. I can always propose a separate 2.0 javaagent version later (that in addition to capturing logs, logs a warning about using a bad log4j version).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
|
||
testImplementation("org.awaitility:awaitility") | ||
|
||
testImplementation("com.lmax:disruptor:3.4.2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment to explain, it's needed for the async logger test
...emetry/javaagent/instrumentation/log4j/appender/v2_0/Log4jAppenderInstrumentationModule.java
Outdated
Show resolved
Hide resolved
...emetry/javaagent/instrumentation/log4j/appender/v2_0/Log4jAppenderInstrumentationModule.java
Outdated
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/log4j/appender/v2_0/Log4jAppenderInstrumentation.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/javaagent/instrumentation/log4j/appender/v2_0/Log4jHelper.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/javaagent/instrumentation/log4j/appender/v2_0/Log4jHelper.java
Outdated
Show resolved
Hide resolved
* log4j2 * Spotless * Target 2.16 for javaagent instrumentation and share * review feedback * Add comment * Remove unnecessary configuration * Fix comment
There's no autoconfiguration of log exporter (yet), so this instrumentation doesn't benefit end users unless they explicitly configure a logging exporter via javaagent extension (this is how the tests work).