Add DD tags to log instrumentations #1688
Conversation
lpriima
force-pushed
the
lpriima/mdc_log_injection_v2
branch
6 times, most recently
from
2a2d21a to
7ff119b
Compare
lpriima
force-pushed
the
lpriima/mdc_log_injection_v2
branch
from
7ff119b to
f99c506
Compare
| log.debug("file '{}' added to shutdown delete hook", file); | ||
| } else { | ||
| log.debug("file '{}' deleted", file); |
There was a problem hiding this comment.
This is just more debug logging not really related to this change
lpriima
force-pushed
the
lpriima/mdc_log_injection_v2
branch
from
f99c506 to
7aa0225
Compare
| def "helper class names are hardcoded in Log Instrumentations"() { | ||
| expect: | ||
| LogContextScopeListener.name == "datadog.trace.agent.tooling.log.LogContextScopeListener" | ||
| ThreadLocalWithDDTagsInitValue.name == "datadog.trace.agent.tooling.log.ThreadLocalWithDDTagsInitValue" | ||
| } |
There was a problem hiding this comment.
I don't understand the point of this test. Why would you want to assert the full name of these classes?
There was a problem hiding this comment.
I would assume the purpose is to guard against automated refactoring changing the classes but not the class names hard coded in various places. i.e this test would fail if either of these classes were renamed.
There was a problem hiding this comment.
I believe most refactoring tools would actually detect this usage and also update the string since it's a fully qualified class name.
There was a problem hiding this comment.
Yes, this is for possible class renaming and it's temporary solution.
Better solution should be running tests with actual classpath restricted by this method:
https://github.com/DataDog/dd-trace-java/pull/1688/files#diff-ddc855d905a56896d1c42dfd404e8541R50-R53
Log4jThreadContextWithEnableThreadLocalsTest that's bullet 2 of description |
|
It isn't clear to me how that validates the fix. Please clarify. Perhaps better describe the problem this is solving and how this fixes it? |
I've updates description here. |
Java tracer support configuration of UST for logs since DataDog/dd-trace-java#1688 (0.59)
Java tracer support configuration of UST for logs since DataDog/dd-trace-java#1688 (0.59)
The issue #1654 was introduced after instrumentation of log4j2. Original instrumentation assumed that interface ThreadContextMap always contains threadlocal field with instance of Map in generic parameter.
In fact, log4j2 has 2 other implementations ( CopyOnWriteSortedArrayThreadContextMap , GarbageFreeSortedArrayThreadContextMap ) which contains StringMap in generic parameter:
And this org.apache.logging.log4j.util.StringMap is not instance of java.util.Map. And It was causing ClassCastException in #1654 .
As a solution in this changeset class ThreadLocalWithDDTagsInitValue extends ThreadLocal has also generic parameter, which will contain "Map-like" type of instrumented implementation. "Map-like" property means method ThreadLocalWithDDTagsInitValue#putToMap looks for the way how to "put" datadog key/values in the instance of this type by following duck typing scenario:
put(String, ?)method then use it. If notputValue(String, ?)method then use it. If notRegression test Log4jThreadContextWithEnableThreadLocalsTest for this issue, modifies static final field to ensure log4j2 uses org.apache.logging.log4j.util.StringMap.