Inject dd.{service|version|env} in logs #1579
Conversation
| throws InvocationTargetException, IllegalAccessException { | ||
| putMethod.invoke(null, Tags.DD_SERVICE, Config.get().getServiceName()); | ||
| { | ||
| final Map<String, String> mergedSpanTags = Config.get().getMergedSpanTags(); |
There was a problem hiding this comment.
It's better not to separate Map.containsKey from Map.get because it involves two lookups. Instead it's better to get the value and check if it's not null. If this happens quite a lot the difference will add up - how often will this get called? If once per span, please make the change, otherwise don't worry about it.
There was a problem hiding this comment.
It happens 1 time per instrumentation. 3 times total during agent startup. But I've changed it
There was a problem hiding this comment.
Sure, sorry, didn't get the full context from the PR alone.
|
@tylerbenson As far as I understand, the |
|
Ok, assuming that's how it works then fine. It would be nice to see if we could have tests validate that somehow? Eg, if you spawn a new thread and do a println without any span in scope, does it show these properties correctly? |
|
It doesn't use inheritable thread local anymore: I'm still working on this change. |
lpriima
force-pushed
the
lpriima/dd.{service|version|env}_to_logs
branch
from
b8bee9e to
14b30bf
Compare
| final Field mdcAdapterField = mdcClass.getDeclaredField("mdcAdapter"); | ||
| mdcAdapterField.setAccessible(true); | ||
| final MDCAdapter mdcAdapterInstance = (MDCAdapter) mdcAdapterField.get(null); | ||
| final Field copyOnThreadLocalField = | ||
| mdcAdapterInstance.getClass().getDeclaredField("copyOnThreadLocal"); | ||
| copyOnThreadLocalField.setAccessible(true); | ||
| copyOnThreadLocalField.set(mdcAdapterInstance, new ThreadLocalWithDDTagsInitValue()); |
There was a problem hiding this comment.
It would be nice if we could avoid all this reflection, but I don't think we can because of our shadow rename for SLF4j.
There was a problem hiding this comment.
I was trying several ways, how to do it more beautiful, but found nothing better than this
If they rename private field, test should fail.
| @@ -42,18 +44,30 @@ protected boolean defaultEnabled() { | |||
|
|
|||
| @Override | |||
| public String[] helperClassNames() { | |||
| return new String[] {LogContextScopeListener.class.getName()}; | |||
| return new String[] { | |||
| LogContextScopeListener.class.getName(), ThreadLocalWithDDTagsInitValue.class.getName(), | |||
There was a problem hiding this comment.
This creates extra class loads that we don't need.
While annoying it is better to do "foo.bar.QuuxClass" instead of QuuxClass.class.getName()
| public class ThreadLocalWithDDTagsInitValue extends ThreadLocal<Map<String, String>> { | ||
| @Override | ||
| protected Map<String, String> initialValue() { | ||
| return LogContextScopeListener.LOG_CONTEXT_DD_TAGS; |
There was a problem hiding this comment.
I noticed this Map is immutable. Do we end up injecting this Map directly into the MDC?
If so, how does this work if the MDC is modified?
There was a problem hiding this comment.
It results in throwing an exception like this:
at java.util.Collections$UnmodifiableMap.put(Collections.java:1457)
at ch.qos.logback.classic.util.LogbackMDCAdapter.put(LogbackMDCAdapter.java:110)
at org.slf4j.MDC.put(MDC.java:147)
at org.apache.log4j.MDC.put(MDC.java:25)
at com.xxx.application.code(...)```
Injecting 3 new tags :
in MDC of every thread for 3 supported logger interfaces .
Logger instrumentation is disabled by default. It can be enabled by
-Ddd.logs.injection=true.