Revert log injection changes

randomanderson · randomanderson · commit 8e388fc9665e · 2020-07-09T14:30:40.000-04:00
diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java
@@ -203,10 +203,6 @@ public boolean matches(final T target) {
       if (name.equals("ch.qos.logback.core.AsyncAppenderBase$Worker")) {
         return false;
       }
-      // for inserting service, env, version in MDC of every thread
-      if (name.equals("ch.qos.logback.classic.util.LogbackMDCAdapter")) {
-        return false;
-      }
 
       return true;
     }
diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/log/LogContextScopeListener.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/log/LogContextScopeListener.java
@@ -2,15 +2,9 @@
 
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan;
 
-import datadog.trace.api.Config;
 import datadog.trace.api.CorrelationIdentifier;
-import datadog.trace.bootstrap.instrumentation.api.Tags;
 import datadog.trace.context.ScopeListener;
-import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
 import lombok.extern.slf4j.Slf4j;
 
 /**
@@ -56,34 +50,4 @@ public void afterScopeClosed() {
       log.debug("Exception removing log context context", e);
     }
   }
-
-  static final Map<String, String> LOG_CONTEXT_DD_TAGS;
-
-  static {
-    Map<String, String> logContextDDTags = new HashMap<>();
-    logContextDDTags.put(Tags.DD_SERVICE, Config.get().getServiceName());
-    final Map<String, String> mergedSpanTags = Config.get().getMergedSpanTags();
-    String version = "";
-    String env = "";
-    if (mergedSpanTags != null) {
-      version = mergedSpanTags.get("version");
-      if (version == null) {
-        version = "";
-      }
-      env = mergedSpanTags.get("env");
-      if (env == null) {
-        env = "";
-      }
-    }
-    logContextDDTags.put(Tags.DD_VERSION, version);
-    logContextDDTags.put(Tags.DD_ENV, env);
-    LOG_CONTEXT_DD_TAGS = Collections.unmodifiableMap(logContextDDTags);
-  }
-
-  public static void addDDTagsToMDC(final Method putMethod)
-      throws InvocationTargetException, IllegalAccessException {
-    for (final Map.Entry<String, String> e : LOG_CONTEXT_DD_TAGS.entrySet()) {
-      putMethod.invoke(null, e.getKey(), e.getValue());
-    }
-  }
 }
diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/log/ThreadLocalWithDDTagsInitValue.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/log/ThreadLocalWithDDTagsInitValue.java
diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/ClassLoaderMatcherTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/ClassLoaderMatcherTest.groovy
@@ -1,8 +1,6 @@
 package datadog.trace.agent.test
 
 import datadog.trace.agent.tooling.ClassLoaderMatcher
-import datadog.trace.agent.tooling.log.LogContextScopeListener
-import datadog.trace.agent.tooling.log.ThreadLocalWithDDTagsInitValue
 import datadog.trace.bootstrap.DatadogClassLoader
 import datadog.trace.util.test.DDSpecification
 
@@ -40,12 +38,6 @@ class ClassLoaderMatcherTest extends DDSpecification {
     DatadogClassLoader.name == "datadog.trace.bootstrap.DatadogClassLoader"
   }
 
-  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"
-  }
-
   /*
    * A URLClassloader which only delegates java.* classes
    */
diff --git a/dd-java-agent/instrumentation/log4j1/src/main/java/datadog/trace/instrumentation/log4j1/Log4j1MDCInstrumentation.java b/dd-java-agent/instrumentation/log4j1/src/main/java/datadog/trace/instrumentation/log4j1/Log4j1MDCInstrumentation.java
@@ -1,15 +1,14 @@
 package datadog.trace.instrumentation.log4j1;
 
 import static java.util.Collections.singletonMap;
-import static net.bytebuddy.matcher.ElementMatchers.isTypeInitializer;
+import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 
 import com.google.auto.service.AutoService;
 import datadog.trace.agent.tooling.Instrumenter;
 import datadog.trace.agent.tooling.log.LogContextScopeListener;
 import datadog.trace.api.Config;
 import datadog.trace.api.GlobalTracer;
-import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.util.Map;
 import net.bytebuddy.asm.Advice;
@@ -38,7 +37,7 @@ public ElementMatcher<? super TypeDescription> typeMatcher() {
   @Override
   public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
     return singletonMap(
-        isTypeInitializer(), Log4j1MDCInstrumentation.class.getName() + "$MDCContextAdvice");
+        isConstructor(), Log4j1MDCInstrumentation.class.getName() + "$MDCContextAdvice");
   }
 
   @Override
@@ -48,16 +47,18 @@ public String[] helperClassNames() {
 
   public static class MDCContextAdvice {
     @Advice.OnMethodExit(suppress = Throwable.class)
-    public static void mdcClassInitialized(@Advice.Origin final Class<?> mdcClass) {
+    public static void mdcClassInitialized(@Advice.This Object instance) {
+      if (instance == null) {
+        return;
+      }
+
       try {
+        Class<?> mdcClass = instance.getClass();
         final Method putMethod = mdcClass.getMethod("put", String.class, Object.class);
         final Method removeMethod = mdcClass.getMethod("remove", String.class);
         GlobalTracer.get().addScopeListener(new LogContextScopeListener(putMethod, removeMethod));
-        // log4j1 uses subclass of InheritableThreadLocal and we don't need to modify private thread
-        // local field:
-        LogContextScopeListener.addDDTagsToMDC(putMethod);
-      } catch (final NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
-        org.slf4j.LoggerFactory.getLogger(mdcClass)
+      } catch (final NoSuchMethodException e) {
+        org.slf4j.LoggerFactory.getLogger(instance.getClass())
             .debug("Failed to add log4j ThreadContext span listener", e);
       }
     }
diff --git a/dd-java-agent/instrumentation/log4j1/src/test/groovy/Log4j1MDCTest.groovy b/dd-java-agent/instrumentation/log4j1/src/test/groovy/Log4j1MDCTest.groovy
@@ -19,15 +19,4 @@ class Log4j1MDCTest extends LogContextInjectionTestBase {
   def get(String key) {
     return MDC.get(key)
   }
-
-  @Override
-  def remove(String key) {
-    MDC.context
-    return MDC.remove(key)
-  }
-
-  @Override
-  def clear() {
-    return MDC.clear()
-  }
 }
diff --git a/dd-java-agent/instrumentation/log4j2/src/main/java/datadog/trace/instrumentation/log4j2/ThreadContextInstrumentation.java b/dd-java-agent/instrumentation/log4j2/src/main/java/datadog/trace/instrumentation/log4j2/ThreadContextInstrumentation.java
@@ -7,10 +7,8 @@
 import com.google.auto.service.AutoService;
 import datadog.trace.agent.tooling.Instrumenter;
 import datadog.trace.agent.tooling.log.LogContextScopeListener;
-import datadog.trace.agent.tooling.log.ThreadLocalWithDDTagsInitValue;
 import datadog.trace.api.Config;
 import datadog.trace.api.GlobalTracer;
-import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.util.Map;
 import net.bytebuddy.asm.Advice;
@@ -20,7 +18,6 @@
 
 @AutoService(Instrumenter.class)
 public class ThreadContextInstrumentation extends Instrumenter.Default {
-  private static final String TYPE_NAME = "org.apache.logging.log4j.ThreadContext";
   public static final String MDC_INSTRUMENTATION_NAME = "log4j";
 
   public ThreadContextInstrumentation() {
@@ -34,7 +31,7 @@ protected boolean defaultEnabled() {
 
   @Override
   public ElementMatcher<? super TypeDescription> typeMatcher() {
-    return named(TYPE_NAME);
+    return named("org.apache.logging.log4j.ThreadContext");
   }
 
   @Override
@@ -45,40 +42,18 @@ public Map<? extends ElementMatcher<? super MethodDescription>, String> transfor
 
   @Override
   public String[] helperClassNames() {
-    return new String[] {
-      "datadog.trace.agent.tooling.log.LogContextScopeListener",
-      "datadog.trace.agent.tooling.log.ThreadLocalWithDDTagsInitValue",
-    };
+    return new String[] {"datadog.trace.agent.tooling.log.LogContextScopeListener"};
   }
 
   public static class ThreadContextAdvice {
     @Advice.OnMethodExit(suppress = Throwable.class)
-    public static void mdcClassInitialized(@Advice.Origin final Class<?> threadContextClass) {
+    public static void mdcClassInitialized(@Advice.Origin final Class threadClass) {
       try {
-        final Method putMethod = threadContextClass.getMethod("put", String.class, String.class);
-        final Method removeMethod = threadContextClass.getMethod("remove", String.class);
+        final Method putMethod = threadClass.getMethod("put", String.class, String.class);
+        final Method removeMethod = threadClass.getMethod("remove", String.class);
         GlobalTracer.get().addScopeListener(new LogContextScopeListener(putMethod, removeMethod));
-
-        final Field contextMapField = threadContextClass.getDeclaredField("contextMap");
-        contextMapField.setAccessible(true);
-        Object contextMap = contextMapField.get(null);
-        if (contextMap
-            .getClass()
-            .getCanonicalName()
-            .equals("org.apache.logging.slf4j.MDCContextMap")) {
-          org.slf4j.LoggerFactory.getLogger(threadContextClass)
-              .debug(
-                  "Log4j to SLF4J Adapter detected. "
-                      + TYPE_NAME
-                      + "'s ThreadLocal"
-                      + " field will not be instrumented because it delegates to slf4-MDC");
-          return;
-        }
-        final Field localMapField = contextMap.getClass().getDeclaredField("localMap");
-        localMapField.setAccessible(true);
-        localMapField.set(contextMap, new ThreadLocalWithDDTagsInitValue());
-      } catch (final NoSuchMethodException | NoSuchFieldException | IllegalAccessException e) {
-        org.slf4j.LoggerFactory.getLogger(threadContextClass)
+      } catch (final NoSuchMethodException e) {
+        org.slf4j.LoggerFactory.getLogger(threadClass)
             .debug("Failed to add log4j ThreadContext span listener", e);
       }
     }
diff --git a/dd-java-agent/instrumentation/log4j2/src/test/groovy/Log4jThreadContextTest.groovy b/dd-java-agent/instrumentation/log4j2/src/test/groovy/Log4jThreadContextTest.groovy
@@ -12,14 +12,4 @@ class Log4jThreadContextTest extends LogContextInjectionTestBase {
   def get(String key) {
     return ThreadContext.get(key)
   }
-
-  @Override
-  def remove(String key) {
-    return ThreadContext.remove(key)
-  }
-
-  @Override
-  def clear() {
-    return ThreadContext.clearAll()
-  }
 }
diff --git a/dd-java-agent/instrumentation/slf4j-mdc/src/main/java/datadog/trace/instrumentation/slf4j/mdc/MDCInjectionInstrumentation.java b/dd-java-agent/instrumentation/slf4j-mdc/src/main/java/datadog/trace/instrumentation/slf4j/mdc/MDCInjectionInstrumentation.java
@@ -7,10 +7,8 @@
 import com.google.auto.service.AutoService;
 import datadog.trace.agent.tooling.Instrumenter;
 import datadog.trace.agent.tooling.log.LogContextScopeListener;
-import datadog.trace.agent.tooling.log.ThreadLocalWithDDTagsInitValue;
 import datadog.trace.api.Config;
 import datadog.trace.api.GlobalTracer;
-import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.security.ProtectionDomain;
 import java.util.Map;
@@ -72,29 +70,17 @@ public boolean matches(final MethodDescription target) {
 
   @Override
   public String[] helperClassNames() {
-    return new String[] {
-      "datadog.trace.agent.tooling.log.LogContextScopeListener",
-      "datadog.trace.agent.tooling.log.ThreadLocalWithDDTagsInitValue",
-    };
+    return new String[] {"datadog.trace.agent.tooling.log.LogContextScopeListener"};
   }
 
   public static class MDCAdvice {
     @Advice.OnMethodExit(suppress = Throwable.class)
-    public static void mdcClassInitialized(@Advice.Origin final Class<?> mdcClass) {
+    public static void mdcClassInitialized(@Advice.Origin final Class mdcClass) {
       try {
         final Method putMethod = mdcClass.getMethod("put", String.class, String.class);
         final Method removeMethod = mdcClass.getMethod("remove", String.class);
         GlobalTracer.get().addScopeListener(new LogContextScopeListener(putMethod, removeMethod));
-
-        final Field mdcAdapterField = mdcClass.getDeclaredField("mdcAdapter");
-        mdcAdapterField.setAccessible(true);
-        final Object mdcAdapterInstance = mdcAdapterField.get(null);
-        final Field copyOnThreadLocalField =
-            mdcAdapterInstance.getClass().getDeclaredField("copyOnThreadLocal");
-        copyOnThreadLocalField.setAccessible(true);
-        copyOnThreadLocalField.set(mdcAdapterInstance, new ThreadLocalWithDDTagsInitValue());
-
-      } catch (final NoSuchMethodException | IllegalAccessException | NoSuchFieldException e) {
+      } catch (final NoSuchMethodException e) {
         org.slf4j.LoggerFactory.getLogger(mdcClass).debug("Failed to add MDC span listener", e);
       }
     }
diff --git a/dd-java-agent/instrumentation/slf4j-mdc/src/test/groovy/Slf4jMDCTest.groovy b/dd-java-agent/instrumentation/slf4j-mdc/src/test/groovy/Slf4jMDCTest.groovy
@@ -12,14 +12,4 @@ class Slf4jMDCTest extends LogContextInjectionTestBase {
   def get(String key) {
     return MDC.get(key)
   }
-
-  @Override
-  def remove(String key) {
-    return MDC.remove(key)
-  }
-
-  @Override
-  def clear() {
-    return MDC.clear()
-  }
 }
diff --git a/dd-java-agent/src/main/java/datadog/trace/bootstrap/AgentBootstrap.java b/dd-java-agent/src/main/java/datadog/trace/bootstrap/AgentBootstrap.java
@@ -5,6 +5,7 @@
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.lang.instrument.Instrumentation;
+import java.lang.invoke.MethodHandles;
 import java.lang.management.ManagementFactory;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
@@ -39,8 +40,8 @@
  *   <li>Do dot touch any logging facilities here so we can configure them later
  * </ul>
  */
-public final class AgentBootstrap {
-  private static final Class<?> thisClass = AgentBootstrap.class;
+public class AgentBootstrap {
+  private static final Class<?> thisClass = MethodHandles.lookup().lookupClass();
 
   public static void premain(final String agentArgs, final Instrumentation inst) {
     agentmain(agentArgs, inst);
diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/log/injection/LogContextInjectionTestBase.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/log/injection/LogContextInjectionTestBase.groovy
diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/Tags.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/Tags.java

Original file line number	Diff line number	Diff line change
`@@ -203,10 +203,6 @@ public boolean matches(final T target) {`
`203`	`203`	`if (name.equals("ch.qos.logback.core.AsyncAppenderBase$Worker")) {`
`204`	`204`	`return false;`
`205`	`205`	`}`
`206`		`- // for inserting service, env, version in MDC of every thread`
`207`		`- if (name.equals("ch.qos.logback.classic.util.LogbackMDCAdapter")) {`
`208`		`- return false;`
`209`		`- }`
`210`	`206`
`211`	`207`	`return true;`
`212`	`208`	`}`