Debugger fix coverage

.gitlab-ci.yml

@@ -1046,11 +1046,17 @@ test_debugger:
     DEFAULT_TEST_JVMS: /^(8|11|17|21|25|semeru8)$/ # the latest "tip" version is LTS v25
   parallel:
     matrix: *test_matrix
+  # avoid running coverage for semeru8, semeru11, semeru17 and ibm8 as some tests are disabled and therefore cannot reach the
+  # exepected coverage
+  script:
+    - if [[ "$testJvm" != "semeru8" && "$testJvm" != "semeru11" && "$testJvm" != "semeru17" && "$testJvm" != "ibm8" ]]; then export GRADLE_PARAMS="$GRADLE_PARAMS -PcheckCoverage"; fi
+    - !reference [.test_job, script]
 
 test_debugger_arm64:
   extends: .test_job_arm64
   variables:
     GRADLE_TARGET: ":debuggerTest"
+    GRADLE_PARAMS: "-PcheckCoverage"
     CACHE_TYPE: "base"
   parallel:
     matrix: *test_matrix

dd-java-agent/agent-debugger/build.gradle

@@ -27,7 +27,11 @@ excludedClassesCoverage += [
   // tested through smoke tests
   'com.datadog.debugger.exception.FailedTestReplayExceptionDebugger',
   // dynamically compiled test classes - exclude to prevent Jacoco instrumentation interference
-  'com.datadog.debugger.symboltest.SymbolExtraction*'
+  'com.datadog.debugger.symboltest.SymbolExtraction*',
+  // Depends on multiple versions of Spring to be tested while we have dep lockfiles
+  'com.datadog.debugger.util.SpringHelper*',
+  // Used for helper methods that are JDK version specific
+  'com.datadog.debugger.agent.ConfigurationUpdater.JDKVersionSpecificHelper'
 ]
 
 dependencies {

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

@@ -41,6 +41,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -210,131 +211,19 @@ private void handleProbesChanges(ConfigurationComparer changes, Configuration ne
     }
     List<Class<?>> changedClasses =
         finder.getAllLoadedChangedClasses(instrumentation.getAllLoadedClasses(), changes);
-    changedClasses = detectMethodParameters(changes, changedClasses);
-    changedClasses = detectRecordWithTypeAnnotation(changes, changedClasses);
+    changedClasses =
+        JDKVersionSpecificHelper.detectMethodParameters(
+            errorMsg -> reportError(changes, errorMsg), instrumentation, changedClasses);
+    changedClasses =
+        JDKVersionSpecificHelper.detectRecordWithTypeAnnotation(
+            errorMsg -> reportError(changes, errorMsg), changedClasses);
     retransformClasses(changedClasses);
     // ensures that we have at least re-transformed 1 class
     if (changedClasses.size() > 0) {
       LOGGER.debug("Re-transformation done");
     }
   }
 
-  /*
-   * Because of this bug (https://bugs.openjdk.org/browse/JDK-8240908), classes compiled with
-   * method parameters (javac -parameters) strip this attribute once retransformed
-   * Spring 6/Spring boot 3 rely exclusively on this attribute and may throw an exception
-   * if no attribute found.
-   */
-  private List<Class<?>> detectMethodParameters(
-      ConfigurationComparer changes, List<Class<?>> changedClasses) {
-    if (JAVA_AT_LEAST_19) {
-      // bug is fixed since JDK19, no need to perform detection
-      return changedClasses;
-    }
-    List<Class<?>> result = new ArrayList<>();
-    for (Class<?> changedClass : changedClasses) {
-      boolean addClass = true;
-      try {
-        Method[] declaredMethods = changedClass.getDeclaredMethods();
-        // capping scanning of methods to 100 to avoid generated class with thousand of methods
-        // assuming that in those first 100 methods there is at least one with at least one
-        // parameter
-        for (int methodIdx = 0;
-            methodIdx < declaredMethods.length && methodIdx < 100;
-            methodIdx++) {
-          Method method = declaredMethods[methodIdx];
-          Parameter[] parameters = method.getParameters();
-          if (parameters.length == 0) {
-            continue;
-          }
-          if (parameters[0].isNamePresent()) {
-            if (!SpringHelper.isSpringUsingOnlyMethodParameters(instrumentation)) {
-              return changedClasses;
-            }
-            LOGGER.debug(
-                "Detecting method parameter: method={} param={}, Skipping retransforming this class",
-                method.getName(),
-                parameters[0].getName());
-            // skip the class: compiled with -parameters
-            reportError(
-                changes,
-                "Method Parameters detected, instrumentation not supported for "
-                    + changedClass.getTypeName());
-            addClass = false;
-          }
-          // we found at leat a method with one parameter if name is not present we can stop there
-          break;
-        }
-      } catch (Exception e) {
-        LOGGER.debug("Exception scanning method parameters", e);
-      }
-      if (addClass) {
-        result.add(changedClass);
-      }
-    }
-    return result;
-  }
-
-  private List<Class<?>> detectRecordWithTypeAnnotation(
-      ConfigurationComparer changes, List<Class<?>> changedClasses) {
-    if (!JAVA_AT_LEAST_16) {
-      // records introduced in JDK 16 (final version)
-      return changedClasses;
-    }
-    List<Class<?>> result = new ArrayList<>();
-    for (Class<?> changedClass : changedClasses) {
-      boolean addClass = true;
-      try {
-        if (changedClass.getSuperclass() != null
-            && changedClass.getSuperclass().getTypeName().equals("java.lang.Record")
-            && Modifier.isFinal(changedClass.getModifiers())) {
-          if (hasTypeAnnotationOnRecordComponent(changedClass)) {
-            LOGGER.debug(
-                "Record with type annotation detected, instrumentation not supported for {}",
-                changedClass.getTypeName());
-            reportError(
-                changes,
-                "Record with type annotation detected, instrumentation not supported for "
-                    + changedClass.getTypeName());
-            addClass = false;
-          }
-        }
-      } catch (Exception e) {
-        LOGGER.debug("Exception detecting record with type annotation", e);
-      }
-      if (addClass) {
-        result.add(changedClass);
-      }
-    }
-    return result;
-  }
-
-  private boolean hasTypeAnnotationOnRecordComponent(Class<?> recordClass) {
-    if (GET_RECORD_COMPONENTS_METHOD == null || GET_ANNOTATED_TYPES_METHOD == null) {
-      return false;
-    }
-    try {
-      Object recordComponentsArray = GET_RECORD_COMPONENTS_METHOD.invoke(recordClass);
-      int len = Array.getLength(recordComponentsArray);
-      for (int i = 0; i < len; i++) {
-        Object recordComponent = Array.get(recordComponentsArray, i);
-        AnnotatedType annotatedType =
-            (AnnotatedType) GET_ANNOTATED_TYPES_METHOD.invoke(recordComponent);
-        for (Annotation annotation : annotatedType.getAnnotations()) {
-          Target annotationTarget = annotation.annotationType().getAnnotation(Target.class);
-          if (annotationTarget != null
-              && Arrays.stream(annotationTarget.value())
-                  .anyMatch(it -> it == ElementType.TYPE_USE)) {
-            return true;
-          }
-        }
-      }
-      return false;
-    } catch (Exception ex) {
-      return false;
-    }
-  }
-
   private void reportReceived(ConfigurationComparer changes) {
     for (ProbeDefinition def : changes.getAddedDefinitions()) {
       if (def instanceof ExceptionProbe) {
@@ -461,4 +350,123 @@ Map<String, ProbeDefinition> getAppliedDefinitions() {
   Map<String, InstrumentationResult> getInstrumentationResults() {
     return instrumentationResults;
   }
+
+  private static class JDKVersionSpecificHelper {
+
+    public static List<Class<?>> detectRecordWithTypeAnnotation(
+        Consumer<String> reportError, List<Class<?>> changedClasses) {
+      if (!JAVA_AT_LEAST_16) {
+        // records introduced in JDK 16 (final version)
+        return changedClasses;
+      }
+      List<Class<?>> result = new ArrayList<>();
+      for (Class<?> changedClass : changedClasses) {
+        boolean addClass = true;
+        try {
+          if (changedClass.getSuperclass() != null
+              && changedClass.getSuperclass().getTypeName().equals("java.lang.Record")
+              && Modifier.isFinal(changedClass.getModifiers())) {
+            if (hasTypeAnnotationOnRecordComponent(changedClass)) {
+              LOGGER.debug(
+                  "Record with type annotation detected, instrumentation not supported for {}",
+                  changedClass.getTypeName());
+              reportError.accept(
+                  "Record with type annotation detected, instrumentation not supported for "
+                      + changedClass.getTypeName());
+              addClass = false;
+            }
+          }
+        } catch (Exception e) {
+          LOGGER.debug("Exception detecting record with type annotation", e);
+        }
+        if (addClass) {
+          result.add(changedClass);
+        }
+      }
+      return result;
+    }
+
+    private static boolean hasTypeAnnotationOnRecordComponent(Class<?> recordClass) {
+      if (GET_RECORD_COMPONENTS_METHOD == null || GET_ANNOTATED_TYPES_METHOD == null) {
+        return false;
+      }
+      try {
+        Object recordComponentsArray = GET_RECORD_COMPONENTS_METHOD.invoke(recordClass);
+        int len = Array.getLength(recordComponentsArray);
+        for (int i = 0; i < len; i++) {
+          Object recordComponent = Array.get(recordComponentsArray, i);
+          AnnotatedType annotatedType =
+              (AnnotatedType) GET_ANNOTATED_TYPES_METHOD.invoke(recordComponent);
+          for (Annotation annotation : annotatedType.getAnnotations()) {
+            Target annotationTarget = annotation.annotationType().getAnnotation(Target.class);
+            if (annotationTarget != null
+                && Arrays.stream(annotationTarget.value())
+                    .anyMatch(it -> it == ElementType.TYPE_USE)) {
+              return true;
+            }
+          }
+        }
+        return false;
+      } catch (Exception ex) {
+        return false;
+      }
+    }
+
+    /*
+     * Because of this bug (https://bugs.openjdk.org/browse/JDK-8240908), classes compiled with
+     * method parameters (javac -parameters) strip this attribute once retransformed
+     * Spring 6/Spring boot 3 rely exclusively on this attribute and may throw an exception
+     * if no attribute found.
+     */
+    public static List<Class<?>> detectMethodParameters(
+        Consumer<String> reportError,
+        Instrumentation instrumentation,
+        List<Class<?>> changedClasses) {
+      if (JAVA_AT_LEAST_19) {
+        // bug is fixed since JDK19, no need to perform detection
+        return changedClasses;
+      }
+      List<Class<?>> result = new ArrayList<>();
+      for (Class<?> changedClass : changedClasses) {
+        boolean addClass = true;
+        try {
+          Method[] declaredMethods = changedClass.getDeclaredMethods();
+          // capping scanning of methods to 100 to avoid generated class with thousand of methods
+          // assuming that in those first 100 methods there is at least one with at least one
+          // parameter
+          for (int methodIdx = 0;
+              methodIdx < declaredMethods.length && methodIdx < 100;
+              methodIdx++) {
+            Method method = declaredMethods[methodIdx];
+            Parameter[] parameters = method.getParameters();
+            if (parameters.length == 0) {
+              continue;
+            }
+            if (parameters[0].isNamePresent()) {
+              if (!SpringHelper.isSpringUsingOnlyMethodParameters(instrumentation)) {
+                return changedClasses;
+              }
+              LOGGER.debug(
+                  "Detecting method parameter: method={} param={}, Skipping retransforming this class",
+                  method.getName(),
+                  parameters[0].getName());
+              // skip the class: compiled with -parameters
+              reportError.accept(
+                  "Method Parameters detected, instrumentation not supported for "
+                      + changedClass.getTypeName());
+              addClass = false;
+            }
+            // we found at leat a method with one parameter if name is not present we can stop there
+            break;
+          }
+        } catch (Exception e) {
+          LOGGER.debug("Exception scanning method parameters", e);
+        }
+        if (addClass) {
+          result.add(changedClass);
+        }
+      }
+      return result;
+    }
+  }
 }

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java

@@ -197,10 +197,6 @@ public List<Snapshot> getSnapshots() {
       return snapshots;
     }
 
-    public boolean isSampling() {
-      return !snapshots.isEmpty();
-    }
-
     public void addSnapshot(Snapshot snapshot) {
       snapshots.add(snapshot);
     }

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/ExceptionProbe.java

@@ -108,7 +108,7 @@ public void evaluate(
           exceptionProbeManager.getStateByThrowable(innerMostThrowable);
       if (state != null) {
         // Already unwinding the exception
-        if (!state.isSampling()) {
+        if (state.getSnapshots().isEmpty()) {
           // skip snapshot because no snapshot from previous stack level
           return;
         }

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/util/SpringHelper.java

@@ -76,7 +76,7 @@ private static boolean isSpringUsingOnlyMethodParametersSpecificClass(Instrument
     return false;
   }
 
-  private static class ParsedSpringVersion {
+  static class ParsedSpringVersion {
     private static final Pattern VERSION_PATTERN = Pattern.compile("(\\d+)\\.(\\d+)\\.(\\d+)");
 
     final int major;

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java

@@ -2,6 +2,9 @@
 
 import static com.datadog.debugger.agent.ConfigurationAcceptor.Source.REMOTE_CONFIG;
 import static com.datadog.debugger.agent.DebuggerProductChangesListener.LOG_PROBE_PREFIX;
+import static com.datadog.debugger.agent.DebuggerProductChangesListener.METRIC_PROBE_PREFIX;
+import static com.datadog.debugger.agent.DebuggerProductChangesListener.SPAN_DECORATION_PROBE_PREFIX;
+import static com.datadog.debugger.agent.DebuggerProductChangesListener.SPAN_PROBE_PREFIX;
 import static com.datadog.debugger.probe.ProbeDefinitionDeserializer.deserializeLogProbe;
 import static com.datadog.debugger.probe.ProbeDefinitionDeserializer.deserializeSpanDecorationProbe;
 import static com.datadog.debugger.probe.ProbeDefinitionDeserializer.deserializeTriggerProbe;
@@ -643,7 +646,10 @@ public void handleException() {
     ConfigurationUpdater configurationUpdater = createConfigUpdater(debuggerSinkWithMockStatusSink);
     Exception ex = new Exception("oops");
     configurationUpdater.handleException(LOG_PROBE_PREFIX + PROBE_ID.getId(), ex);
-    verify(probeStatusSink).addError(eq(ProbeId.from(PROBE_ID.getId() + ":0")), eq(ex));
+    configurationUpdater.handleException(METRIC_PROBE_PREFIX + PROBE_ID.getId(), ex);
+    configurationUpdater.handleException(SPAN_PROBE_PREFIX + PROBE_ID.getId(), ex);
+    configurationUpdater.handleException(SPAN_DECORATION_PROBE_PREFIX + PROBE_ID.getId(), ex);
+    verify(probeStatusSink, times(4)).addError(eq(ProbeId.from(PROBE_ID.getId() + ":0")), eq(ex));
   }
 
   @Test

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SourceRemapperTest.java

@@ -30,4 +30,18 @@ public void kotlinSourceRemapper() {
     assertTrue(sourceRemapper instanceof SourceRemapper.KotlinSourceRemapper);
     assertEquals(24, sourceRemapper.remapSourceLine(42));
   }
+
+  @Test
+  public void noKotlinDebug() {
+    SourceMap sourceMapMock = mock(SourceMap.class);
+    when(sourceMapMock.getDefaultStratumName()).thenReturn("Main");
+    StratumExt stratumMainMock = mock(StratumExt.class);
+    when(sourceMapMock.getStratum(eq("Kotlin"))).thenReturn(stratumMainMock);
+    when(sourceMapMock.getStratum(eq("KotlinDebug"))).thenReturn(null);
+    IllegalArgumentException illegalArgumentException =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> SourceRemapper.getSourceRemapper("foo.kt", sourceMapMock));
+    assertEquals("No stratumDebug found for KotlinDebug", illegalArgumentException.getMessage());
+  }
 }

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SymbolExtractionTransformerTest.java

@@ -968,7 +968,6 @@ public void symbolExtraction15() throws IOException, URISyntaxException {
   }
 
   @Test
-  @EnabledForJreRange(max = JRE.JAVA_25)
   @DisabledIf(
       value = "datadog.environment.JavaVirtualMachine#isJ9",
       disabledReason = "Flaky on J9 JVMs")

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/util/SpringHelperTest.java

@@ -35,4 +35,12 @@ void isSpringUsingOnlyMethodParametersFalseFallback() throws Exception {
     when(inst.getAllLoadedClasses()).thenReturn(new Class[0]);
     assertFalse(SpringHelper.isSpringUsingOnlyMethodParameters(inst));
   }
+
+  @Test
+  void invalidSpringVersion() {
+    IllegalArgumentException illegalArgumentException =
+        assertThrows(
+            IllegalArgumentException.class, () -> new SpringHelper.ParsedSpringVersion("foo"));
+    assertEquals("Cannot parse SpringVersion: foo", illegalArgumentException.getMessage());
+  }
 }