Merge branch 'master' into mhlidd/config_inversion_plugins

Author: mhlidd

communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java

@@ -271,13 +271,11 @@ private boolean processInfoResponse(State newState, String response) {
         newState.debuggerLogEndpoint = DEBUGGER_ENDPOINT_V1;
       }
       // both debugger v2 and diagnostics endpoints are forwarding events to the DEBUGGER intake
-      // because older agents support diagnostics, we fall back to it before falling back to v1
+      // because older agents support diagnostics from DD agent 7.49
       if (containsEndpoint(endpoints, DEBUGGER_ENDPOINT_V2)) {
         newState.debuggerSnapshotEndpoint = DEBUGGER_ENDPOINT_V2;
       } else if (containsEndpoint(endpoints, DEBUGGER_DIAGNOSTICS_ENDPOINT)) {
         newState.debuggerSnapshotEndpoint = DEBUGGER_DIAGNOSTICS_ENDPOINT;
-      } else if (containsEndpoint(endpoints, DEBUGGER_ENDPOINT_V1)) {
-        newState.debuggerSnapshotEndpoint = DEBUGGER_ENDPOINT_V1;
       }
       if (containsEndpoint(endpoints, DEBUGGER_DIAGNOSTICS_ENDPOINT)) {
         newState.debuggerDiagnosticsEndpoint = DEBUGGER_DIAGNOSTICS_ENDPOINT;

communication/src/test/groovy/datadog/communication/ddagent/DDAgentFeaturesDiscoveryTest.groovy

@@ -440,8 +440,8 @@ class DDAgentFeaturesDiscoveryTest extends DDSpecification {
     1 * client.newCall(_) >> { Request request -> infoResponse(request, INFO_WITH_TELEMETRY_PROXY_RESPONSE) }
     features.supportsTelemetryProxy()
     features.supportsDebugger()
-    features.getDebuggerSnapshotEndpoint() == "debugger/v1/input"
-    !features.supportsDebuggerDiagnostics()
+    features.getDebuggerSnapshotEndpoint() == "debugger/v1/diagnostics"
+    features.supportsDebuggerDiagnostics()
     0 * _
   }
 

communication/src/test/resources/agent-features/agent-info-with-telemetry-proxy.json

@@ -15,6 +15,7 @@
     "/evp_proxy/v1/",
     "/evp_proxy/v2/",
     "/debugger/v1/input",
+    "/debugger/v1/diagnostics",
     "/v0.7/config"
   ],
   "feature_flags": [

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/benchmark/StaticEventLogger.java

@@ -97,7 +97,7 @@ private static String getAgentVersion() {
       for (int c = reader.read(); c != -1; c = reader.read()) {
         sb.append((char) c);
       }
-    } catch (IOException e) {
+    } catch (Throwable ignored) {
       // swallow exception
       return null;
     }

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java

@@ -131,7 +131,7 @@ private void applyNewConfiguration(Configuration newConfiguration) {
       }
       currentConfiguration = newConfiguration;
       if (changes.hasProbeRelatedChanges()) {
-        LOGGER.info("Applying new probe configuration, changes: {}", changes);
+        LOGGER.debug("Applying new probe configuration, changes: {}", changes);
         handleProbesChanges(changes, newConfiguration);
       }
     } finally {
@@ -208,7 +208,7 @@ private void recordInstrumentationProgress(
   private void retransformClasses(List<Class<?>> classesToBeTransformed) {
     for (Class<?> clazz : classesToBeTransformed) {
       try {
-        LOGGER.info("Re-transforming class: {}", clazz.getTypeName());
+        LOGGER.debug("Re-transforming class: {}", clazz.getTypeName());
         instrumentation.retransformClasses(clazz);
       } catch (Exception ex) {
         ExceptionHelper.logException(LOGGER, ex, "Re-transform error:");

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerAgent.java

@@ -341,9 +341,11 @@ private static String getLogEndpoint(
   private static String getSnapshotEndpoint(
       Config config, DDAgentFeaturesDiscovery ddAgentFeaturesDiscovery) {
     if (ddAgentFeaturesDiscovery.supportsDebugger()) {
-      return ddAgentFeaturesDiscovery
-          .buildUrl(ddAgentFeaturesDiscovery.getDebuggerSnapshotEndpoint())
-          .toString();
+      String debuggerSnapshotEndpoint = ddAgentFeaturesDiscovery.getDebuggerSnapshotEndpoint();
+      if (debuggerSnapshotEndpoint == null) {
+        throw new IllegalArgumentException("Cannot find snapshot endpoint on datadog agent");
+      }
+      return ddAgentFeaturesDiscovery.buildUrl(debuggerSnapshotEndpoint).toString();
     }
     return config.getFinalDebuggerSnapshotUrl();
   }

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java

@@ -59,8 +59,11 @@
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.pool.TypePool;
 import org.objectweb.asm.ClassReader;
+import org.objectweb.asm.ClassVisitor;
 import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.commons.JSRInlinerAdapter;
 import org.objectweb.asm.tree.ClassNode;
 import org.objectweb.asm.tree.MethodNode;
 import org.objectweb.asm.tree.analysis.Analyzer;
@@ -76,7 +79,7 @@
  * debugger configuration
  */
 public class DebuggerTransformer implements ClassFileTransformer {
-  private static final Logger log = LoggerFactory.getLogger(DebuggerTransformer.class);
+  private static final Logger LOGGER = LoggerFactory.getLogger(DebuggerTransformer.class);
   private static final String CANNOT_FIND_METHOD = "Cannot find method %s::%s%s";
   private static final String INSTRUMENTATION_FAILS = "Instrumentation fails for %s";
   private static final String CANNOT_FIND_LINE = "No executable code was found at %s:L%s";
@@ -148,7 +151,7 @@ public DebuggerTransformer(
       } else if (itwType.equals("line")) {
         probeCreator = this::createLineProbes;
       } else {
-        log.warn(
+        LOGGER.warn(
             "Invalid value for 'dd.debugger.instrument-the-world' property: {}. "
                 + "Valid values are 'method' or 'line'.",
             itwType);
@@ -202,7 +205,7 @@ private void processITWFiles(
     for (String fileName : fileNames) {
       Path excludePath = Paths.get(fileName);
       if (!Files.exists(excludePath)) {
-        log.warn("Cannot find exclude file: {}", excludePath);
+        LOGGER.warn("Cannot find exclude file: {}", excludePath);
         continue;
       }
       try {
@@ -223,7 +226,7 @@ private void processITWFiles(
                   classes.add(line);
                 });
       } catch (IOException ex) {
-        log.warn("Error reading exclude file '{}' for Instrument-The-World: ", fileName, ex);
+        LOGGER.warn("Error reading exclude file '{}' for Instrument-The-World: ", fileName, ex);
       }
     }
   }
@@ -250,7 +253,7 @@ public byte[] transform(
       if (definitions.isEmpty()) {
         return null;
       }
-      log.debug("Matching definitions for class[{}]: {}", fullyQualifiedClassName, definitions);
+      LOGGER.debug("Matching definitions for class[{}]: {}", fullyQualifiedClassName, definitions);
       if (!instrumentationIsAllowed(fullyQualifiedClassName, definitions)) {
         return null;
       }
@@ -260,22 +263,22 @@ public byte[] transform(
       if (transformed) {
         return writeClassFile(definitions, loader, classFilePath, classNode);
       }
-      // This is an info log because in case of SourceFile definition and multiple top-level
+      // We are logging this because in case of SourceFile definition and multiple top-level
       // classes, type may match, but there is one classfile per top-level class so source file
       // will match, but not the classfile.
       // e.g. Main.java contains Main & TopLevel class, line numbers are in TopLevel class
-      log.info(
+      LOGGER.debug(
           "type {} matched but no transformation for definitions: {}", classFilePath, definitions);
     } catch (Throwable ex) {
-      log.warn("Cannot transform: ", ex);
+      LOGGER.warn("Cannot transform: ", ex);
       reportInstrumentationFails(definitions, fullyQualifiedClassName);
     }
     return null;
   }
 
   private boolean skipInstrumentation(ClassLoader loader, String classFilePath) {
     if (definitionMatcher.isEmpty()) {
-      log.debug("No debugger definitions present.");
+      LOGGER.debug("No debugger definitions present.");
       return true;
     }
     if (classFilePath == null) {
@@ -311,7 +314,7 @@ private byte[] transformTheWorld(
           location = codeSource.getLocation();
         }
       }
-      log.debug(
+      LOGGER.debug(
           "Parsing class '{}' {}B loaded from loader='{}' location={}",
           classFilePath,
           classfileBuffer.length,
@@ -320,7 +323,7 @@ private byte[] transformTheWorld(
       ClassNode classNode = parseClassFile(classFilePath, classfileBuffer);
       if (isClassLoaderRelated(classNode)) {
         // Skip ClassLoader classes
-        log.debug("Skipping ClassLoader class: {}", classFilePath);
+        LOGGER.debug("Skipping ClassLoader class: {}", classFilePath);
         excludeClasses.add(classFilePath);
         return null;
       }
@@ -337,10 +340,10 @@ private byte[] transformTheWorld(
       if (transformed) {
         return writeClassFile(probes, loader, classFilePath, classNode);
       } else {
-        log.debug("Class not transformed: {}", classFilePath);
+        LOGGER.debug("Class not transformed: {}", classFilePath);
       }
     } catch (Throwable ex) {
-      log.warn("Cannot transform: ", ex);
+      LOGGER.warn("Cannot transform: ", ex);
       writeToInstrumentationLog(classFilePath);
     }
     return null;
@@ -354,7 +357,7 @@ private boolean isMethodIncludedForTransformation(
     }
     String fqnMethod = classNode.name + "::" + methodNode.name;
     if (excludeMethods.contains(fqnMethod)) {
-      log.debug("Skipping method: {}", fqnMethod);
+      LOGGER.debug("Skipping method: {}", fqnMethod);
       return false;
     }
     return methodNames.add(methodNode.name);
@@ -404,7 +407,7 @@ private synchronized void writeToInstrumentationLog(String classFilePath) {
       writer.write(classFilePath);
       writer.write("\n");
     } catch (Exception ex) {
-      log.warn("Cannot write to instrumentation.log", ex);
+      LOGGER.warn("Cannot write to instrumentation.log", ex);
     }
   }
 
@@ -443,7 +446,7 @@ private boolean isIncludedForTransformation(String classFilePath) {
   private boolean instrumentationIsAllowed(
       String fullyQualifiedClassName, List<ProbeDefinition> definitions) {
     if (denyListHelper.isDenied(fullyQualifiedClassName)) {
-      log.info("Instrumentation denied for {}", fullyQualifiedClassName);
+      LOGGER.debug("Instrumentation denied for {}", fullyQualifiedClassName);
       InstrumentationResult result =
           InstrumentationResult.Factory.blocked(
               fullyQualifiedClassName,
@@ -455,7 +458,7 @@ private boolean instrumentationIsAllowed(
       return false;
     }
     if (!allowListHelper.isAllowAll() && !allowListHelper.isAllowed(fullyQualifiedClassName)) {
-      log.info("Instrumentation not allowed for {}", fullyQualifiedClassName);
+      LOGGER.debug("Instrumentation not allowed for {}", fullyQualifiedClassName);
       InstrumentationResult result =
           InstrumentationResult.Factory.blocked(
               fullyQualifiedClassName,
@@ -487,11 +490,12 @@ private byte[] writeClassFile(
       classNode.version = Opcodes.V1_8;
     }
     ClassWriter writer = new SafeClassWriter(loader);
-    log.debug("Generating bytecode for class: {}", Strings.getClassName(classFilePath));
+    ClassVisitor visitor = new JsrInliningClassVisitor(writer);
+    LOGGER.debug("Generating bytecode for class: {}", Strings.getClassName(classFilePath));
     try {
-      classNode.accept(writer);
+      classNode.accept(visitor);
     } catch (Throwable t) {
-      log.error("Cannot write classfile for class: {} Exception: ", classFilePath, t);
+      LOGGER.error("Cannot write classfile for class: {} Exception: ", classFilePath, t);
       reportInstrumentationFails(definitions, Strings.getClassName(classFilePath));
       return null;
     }
@@ -526,8 +530,8 @@ private void verifyByteCode(String classFilePath, byte[] classFile) {
     printWriter.flush();
     String result = stringWriter.toString();
     if (!result.isEmpty()) {
-      log.warn("Verification of instrumented class {} failed", classFilePath);
-      log.debug("Verify result: {}", stringWriter);
+      LOGGER.warn("Verification of instrumented class {} failed", classFilePath);
+      LOGGER.debug("Verify result: {}", stringWriter);
       throw new RuntimeException("Generated bytecode is invalid for " + classFilePath);
     }
   }
@@ -560,9 +564,9 @@ private boolean performInstrumentation(
       if (matchingDefs.isEmpty()) {
         continue;
       }
-      if (log.isDebugEnabled()) {
+      if (LOGGER.isDebugEnabled()) {
         List<String> probeIds = matchingDefs.stream().map(ProbeDefinition::getId).collect(toList());
-        log.debug(
+        LOGGER.debug(
             "Instrumenting method: {}.{}{} for probe ids: {}",
             fullyQualifiedClassName,
             methodNode.name,
@@ -627,7 +631,7 @@ private void reportErrorForAllProbes(List<ProbeDefinition> definitions, String m
   private void addDiagnostics(
       ProbeDefinition definition, List<DiagnosticMessage> diagnosticMessages) {
     debuggerSink.addDiagnostics(definition.getProbeId(), diagnosticMessages);
-    log.debug("Diagnostic messages for definition[{}]: {}", definition, diagnosticMessages);
+    LOGGER.debug("Diagnostic messages for definition[{}]: {}", definition, diagnosticMessages);
   }
 
   private void notifyBlockedDefinitions(
@@ -658,7 +662,7 @@ private InstrumentationResult applyInstrumentation(
           status = definition.instrument(methodInfo, probeDiagnostics, toInstrumentInfo.probeIds);
         }
       } catch (Throwable t) {
-        log.warn("Exception during instrumentation: ", t);
+        LOGGER.warn("Exception during instrumentation: ", t);
         status = InstrumentationResult.Status.ERROR;
         addDiagnosticForAllProbes(
             new DiagnosticMessage(DiagnosticMessage.Kind.ERROR, t), diagnostics);
@@ -694,7 +698,7 @@ private List<ToInstrumentInfo> filterAndSortDefinitions(
       // and therefore need to be instrumented once
       // note: exception probes are log probes and are handled the same way
       if (!Config.get().isDistributedDebuggerEnabled() && definition instanceof TriggerProbe) {
-        log.debug(
+        LOGGER.debug(
             "The distributed debugger feature is disabled. Trigger probes will not be installed.");
       } else if (isCapturedContextProbe(definition)) {
         if (definition.isLineProbe()) {
@@ -871,7 +875,7 @@ private List<MethodNode> matchMethodDescription(
         }
       }
     } catch (Exception ex) {
-      log.warn("Cannot match method: {}", ex.toString());
+      LOGGER.warn("Cannot match method: {}", ex.toString());
     }
     return result;
   }
@@ -888,22 +892,22 @@ private MethodNode matchSourceFile(
     if (matchingMethods != null) {
       matchingMethods.forEach(
           methodNode -> {
-            log.debug("Found lineNode {} method: {}", matchingLine, methodNode.name);
+            LOGGER.debug("Found lineNode {} method: {}", matchingLine, methodNode.name);
           });
       // pick the first matching method.
       // TODO need a way to disambiguate if multiple methods match the same line
       return matchingMethods.isEmpty() ? null : matchingMethods.get(0);
     }
-    log.debug("Cannot find line: {} in class {}", matchingLine, classNode.name);
+    LOGGER.debug("Cannot find line: {} in class {}", matchingLine, classNode.name);
     return null;
   }
 
   private void dumpInstrumentedClassFile(String className, byte[] data) {
     if (config.isDynamicInstrumentationClassFileDumpEnabled()) {
-      log.debug("Generated bytecode len: {}", data.length);
+      LOGGER.debug("Generated bytecode len: {}", data.length);
       Path classFilePath = dumpClassFile(className, data);
       if (classFilePath != null) {
-        log.debug("Instrumented class saved as: {}", classFilePath.toString());
+        LOGGER.debug("Instrumented class saved as: {}", classFilePath.toString());
       }
     }
   }
@@ -912,7 +916,7 @@ private void dumpOriginalClassFile(String className, byte[] classfileBuffer) {
     if (config.isDynamicInstrumentationClassFileDumpEnabled()) {
       Path classFilePath = dumpClassFile(className + "_orig", classfileBuffer);
       if (classFilePath != null) {
-        log.debug("Original class saved as: {}", classFilePath.toString());
+        LOGGER.debug("Original class saved as: {}", classFilePath.toString());
       }
     }
   }
@@ -924,11 +928,31 @@ private static Path dumpClassFile(String className, byte[] classfileBuffer) {
       Files.write(classFilePath, classfileBuffer, StandardOpenOption.CREATE);
       return classFilePath;
     } catch (IOException e) {
-      log.error("", e);
+      LOGGER.error("", e);
       return null;
     }
   }
 
+  /**
+   * A {@link org.objectweb.asm.ClassVisitor} that uses {@link
+   * org.objectweb.asm.commons.JSRInlinerAdapter} to remove JSR instructions and inlines the
+   * referenced subroutines. This allows pre-Java 6 classes with finally blocks to be successfully
+   * transformed. Without this an IllegalArgumentException for "JSR/RET are not supported with
+   * computeFrames option" would be thrown when writing the transformed class.
+   */
+  static class JsrInliningClassVisitor extends ClassVisitor {
+    protected JsrInliningClassVisitor(ClassVisitor parent) {
+      super(Opcodes.ASM9, parent);
+    }
+
+    @Override
+    public MethodVisitor visitMethod(
+        int access, String name, String descriptor, String signature, String[] exceptions) {
+      MethodVisitor mv = super.visitMethod(access, name, descriptor, signature, exceptions);
+      return new JSRInlinerAdapter(mv, access, name, descriptor, signature, exceptions);
+    }
+  }
+
   static class SafeClassWriter extends ClassWriter {
     private final ClassLoader classLoader;
 
@@ -981,7 +1005,7 @@ protected String getCommonSuperClass(String type1, String type2) {
         }
         return common.getInternalName();
       } catch (Exception ex) {
-        ExceptionHelper.logException(log, ex, "getCommonSuperClass failed: ");
+        ExceptionHelper.logException(LOGGER, ex, "getCommonSuperClass failed: ");
         return tpDatadogClassLoader.describe("java.lang.Object").resolve().getInternalName();
       }
     }

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/codeorigin/DefaultCodeOriginRecorder.java

@@ -60,6 +60,10 @@ public DefaultCodeOriginRecorder(
 
   @Override
   public String captureCodeOrigin(boolean entry) {
+    if (!entry) {
+      LOG.debug("Not capturing code origin for exit");
+      return null;
+    }
     StackTraceElement element = findPlaceInStack();
     String fingerprint = Fingerprinter.fingerprint(element);
     CodeOriginProbe probe = probesByFingerprint.get(fingerprint);