Support log trace injection for log4j 1.x and log4j 2.x when used without Slf4j#894
Conversation
…ut seems not to be relevant here
…UST satisfy for log injection
| } | ||
|
|
||
| def "mdc shows trace and span ids for active scope"() { | ||
| when: |
There was a problem hiding this comment.
Tests moved to the abstract class: https://github.com/DataDog/dd-trace-java/pull/894/files#diff-d48ec7e298e78bab82f00e009f3ed477
labbati
changed the title
labbati
changed the title
| .or(nameStartsWith("com.jinspired.")) | ||
| .or(nameStartsWith("org.jinspired.")) | ||
| .or(nameStartsWith("org.apache.log4j.")) | ||
| .and(not(named("org.apache.log4j.MDC"))) |
There was a problem hiding this comment.
This seems wrong - and should be inside or, shouldn't it?
There was a problem hiding this comment.
Good catch, at the end of the day it worked but it was very wrong 😄
|
You may want to look into why tests failing there - looks like jacoco needs some tuning |
|
|
||
| @Override | ||
| public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() { | ||
| return singletonMap(isConstructor(), MDCContextAdvice.class.getName()); |
There was a problem hiding this comment.
org.apache.log4j.MDC seems to be class with only static methods. It seems slightly awkward to instrument it's constructor. Are you sure it is being called at all?
There was a problem hiding this comment.
Ah, nm, I was looking at only one implementation out of a few... using constructor as advice hook still seems odd.
There was a problem hiding this comment.
It took a while to find out the source code corresponding to the version being downloaded by gradle. Here is the link for future reference: http://svn.apache.org/viewvc/logging/log4j/trunk/src/main/java/org/apache/log4j/MDC.java?view=markup
There was a problem hiding this comment.
You do not have to change this now, or at all - but I think isTypeInitializer might have been better matcher here.
| compile project(':dd-java-agent:agent-tooling') | ||
|
|
||
| testCompile group: 'org.apache.logging.log4j', name: 'log4j-core', version: log4jVersion | ||
| testCompile group: 'org.apache.logging.log4j', name: 'log4j-api', version: log4jVersion |
There was a problem hiding this comment.
nitpic: you may want to group testCompile clauses together and put them after non-test dependencies.
|
|
||
| @AutoService(Instrumenter.class) | ||
| public class ThreadContextInstrumentation extends Instrumenter.Default { | ||
| public static final String MDC_INSTRUMENTATION_NAME = "log4j-thread-context"; |
There was a problem hiding this comment.
Should naming simply be log4j1 and log4j2 so make things less confusing?
There was a problem hiding this comment.
That is a good idea. Alternatively we could name it mdc in the same way it happens for the Slf4j instrumentation. WDYT?
There was a problem hiding this comment.
I do not have a strong preference
mar-kolya
left a comment
mar-kolya
left a comment
There was a problem hiding this comment.
LGTM overall.
But need to make sure that build is passing and couple of minor comments.
|
@mar-kolya might I have your final approval to merge? |
2 participants
Previously we only supported log correlation when the java application was using Slf4j.
This PR add support for trace <-> log correlation outside Slf4j if the app is using log4j 1.x or logj 2.x.
In addition to that we only defined a mandatory test suite that a logging library integration must satisfy in order to properly support trace correlation.