diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a56fe676388..2e1b6d7641b 100644 --- a/.gitlab-ci.yml +++ b/.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 diff --git a/dd-java-agent/agent-debugger/build.gradle b/dd-java-agent/agent-debugger/build.gradle index b6836682a5d..47bbce94548 100644 --- a/dd-java-agent/agent-debugger/build.gradle +++ b/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 { diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java index 3caa2a149ab..5a68838f3b8 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java +++ b/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,8 +211,12 @@ private void handleProbesChanges(ConfigurationComparer changes, Configuration ne } List> 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) { @@ -219,122 +224,6 @@ private void handleProbesChanges(ConfigurationComparer changes, Configuration ne } } - /* - * 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> detectMethodParameters( - ConfigurationComparer changes, List> changedClasses) { - if (JAVA_AT_LEAST_19) { - // bug is fixed since JDK19, no need to perform detection - return changedClasses; - } - List> 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> detectRecordWithTypeAnnotation( - ConfigurationComparer changes, List> changedClasses) { - if (!JAVA_AT_LEAST_16) { - // records introduced in JDK 16 (final version) - return changedClasses; - } - List> 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 getAppliedDefinitions() { Map getInstrumentationResults() { return instrumentationResults; } + + private static class JDKVersionSpecificHelper { + + public static List> detectRecordWithTypeAnnotation( + Consumer reportError, List> changedClasses) { + if (!JAVA_AT_LEAST_16) { + // records introduced in JDK 16 (final version) + return changedClasses; + } + List> 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> detectMethodParameters( + Consumer reportError, + Instrumentation instrumentation, + List> changedClasses) { + if (JAVA_AT_LEAST_19) { + // bug is fixed since JDK19, no need to perform detection + return changedClasses; + } + List> 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; + } + } } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java index 3aeb7d05237..71b80c1d02f 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java @@ -197,10 +197,6 @@ public List getSnapshots() { return snapshots; } - public boolean isSampling() { - return !snapshots.isEmpty(); - } - public void addSnapshot(Snapshot snapshot) { snapshots.add(snapshot); } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/ExceptionProbe.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/ExceptionProbe.java index c1e2b67ca8a..d405203648f 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/ExceptionProbe.java +++ b/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; } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/util/SpringHelper.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/util/SpringHelper.java index d058caab85d..dbcb2f1a477 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/util/SpringHelper.java +++ b/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; diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java index 0ec8e1622e1..fe383c040b5 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java +++ b/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 diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SourceRemapperTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SourceRemapperTest.java index 891338d7674..c050915f5ae 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SourceRemapperTest.java +++ b/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()); + } } diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SymbolExtractionTransformerTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SymbolExtractionTransformerTest.java index c6e488752c4..594b76e67d0 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SymbolExtractionTransformerTest.java +++ b/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") diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/util/SpringHelperTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/util/SpringHelperTest.java index 526ecdd2adf..3381dfe71e6 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/util/SpringHelperTest.java +++ b/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()); + } }