Skip to content

Commit b710ce8

Browse files
jpbempeldevflow.devflow-routing-intake
jpbempel
and
devflow.devflow-routing-intake
authored
Debugger fix coverage (#11807)
fix coverage fix coverage for - ExceptionProbeManager.ThrowableState - SpringHelper.ParserSpringVersion - SourceMapper - ConfigurationUpdater add check coverage for debugger tests exclude from coverage JDK version or Spring version dependent move detect method parameters to helper fix covergage for semeru8 disable coverage for all ibm jvms enable Kotlin for all JDKs Co-authored-by: devflow.devflow-routing-intake <[email protected]>
1 parent f62f905 commit b710ce8

10 files changed

Lines changed: 168 additions & 127 deletions

File tree

‎.gitlab-ci.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1046,11 +1046,17 @@ test_debugger:
10461046
DEFAULT_TEST_JVMS: /^(8|11|17|21|25|semeru8)$/ # the latest "tip" version is LTS v25
10471047
parallel:
10481048
matrix: *test_matrix
1049+
# avoid running coverage for semeru8, semeru11, semeru17 and ibm8 as some tests are disabled and therefore cannot reach the
1050+
# exepected coverage
1051+
script:
1052+
- if [[ "$testJvm" != "semeru8" && "$testJvm" != "semeru11" && "$testJvm" != "semeru17" && "$testJvm" != "ibm8" ]]; then export GRADLE_PARAMS="$GRADLE_PARAMS -PcheckCoverage"; fi
1053+
- !reference [.test_job, script]
10491054

10501055
test_debugger_arm64:
10511056
extends: .test_job_arm64
10521057
variables:
10531058
GRADLE_TARGET: ":debuggerTest"
1059+
GRADLE_PARAMS: "-PcheckCoverage"
10541060
CACHE_TYPE: "base"
10551061
parallel:
10561062
matrix: *test_matrix

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,11 @@ excludedClassesCoverage += [
2727
// tested through smoke tests
2828
'com.datadog.debugger.exception.FailedTestReplayExceptionDebugger',
2929
// dynamically compiled test classes - exclude to prevent Jacoco instrumentation interference
30-
'com.datadog.debugger.symboltest.SymbolExtraction*'
30+
'com.datadog.debugger.symboltest.SymbolExtraction*',
31+
// Depends on multiple versions of Spring to be tested while we have dep lockfiles
32+
'com.datadog.debugger.util.SpringHelper*',
33+
// Used for helper methods that are JDK version specific
34+
'com.datadog.debugger.agent.ConfigurationUpdater.JDKVersionSpecificHelper'
3135
]
3236

3337
dependencies {

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

Lines changed: 126 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import java.util.concurrent.TimeUnit;
4242
import java.util.concurrent.locks.Lock;
4343
import java.util.concurrent.locks.ReentrantLock;
44+
import java.util.function.Consumer;
4445
import java.util.function.Function;
4546
import java.util.function.Supplier;
4647
import java.util.stream.Collectors;
@@ -210,131 +211,19 @@ private void handleProbesChanges(ConfigurationComparer changes, Configuration ne
210211
}
211212
List<Class<?>> changedClasses =
212213
finder.getAllLoadedChangedClasses(instrumentation.getAllLoadedClasses(), changes);
213-
changedClasses = detectMethodParameters(changes, changedClasses);
214-
changedClasses = detectRecordWithTypeAnnotation(changes, changedClasses);
214+
changedClasses =
215+
JDKVersionSpecificHelper.detectMethodParameters(
216+
errorMsg -> reportError(changes, errorMsg), instrumentation, changedClasses);
217+
changedClasses =
218+
JDKVersionSpecificHelper.detectRecordWithTypeAnnotation(
219+
errorMsg -> reportError(changes, errorMsg), changedClasses);
215220
retransformClasses(changedClasses);
216221
// ensures that we have at least re-transformed 1 class
217222
if (changedClasses.size() > 0) {
218223
LOGGER.debug("Re-transformation done");
219224
}
220225
}
221226

222-
/*
223-
* Because of this bug (https://bugs.openjdk.org/browse/JDK-8240908), classes compiled with
224-
* method parameters (javac -parameters) strip this attribute once retransformed
225-
* Spring 6/Spring boot 3 rely exclusively on this attribute and may throw an exception
226-
* if no attribute found.
227-
*/
228-
private List<Class<?>> detectMethodParameters(
229-
ConfigurationComparer changes, List<Class<?>> changedClasses) {
230-
if (JAVA_AT_LEAST_19) {
231-
// bug is fixed since JDK19, no need to perform detection
232-
return changedClasses;
233-
}
234-
List<Class<?>> result = new ArrayList<>();
235-
for (Class<?> changedClass : changedClasses) {
236-
boolean addClass = true;
237-
try {
238-
Method[] declaredMethods = changedClass.getDeclaredMethods();
239-
// capping scanning of methods to 100 to avoid generated class with thousand of methods
240-
// assuming that in those first 100 methods there is at least one with at least one
241-
// parameter
242-
for (int methodIdx = 0;
243-
methodIdx < declaredMethods.length && methodIdx < 100;
244-
methodIdx++) {
245-
Method method = declaredMethods[methodIdx];
246-
Parameter[] parameters = method.getParameters();
247-
if (parameters.length == 0) {
248-
continue;
249-
}
250-
if (parameters[0].isNamePresent()) {
251-
if (!SpringHelper.isSpringUsingOnlyMethodParameters(instrumentation)) {
252-
return changedClasses;
253-
}
254-
LOGGER.debug(
255-
"Detecting method parameter: method={} param={}, Skipping retransforming this class",
256-
method.getName(),
257-
parameters[0].getName());
258-
// skip the class: compiled with -parameters
259-
reportError(
260-
changes,
261-
"Method Parameters detected, instrumentation not supported for "
262-
+ changedClass.getTypeName());
263-
addClass = false;
264-
}
265-
// we found at leat a method with one parameter if name is not present we can stop there
266-
break;
267-
}
268-
} catch (Exception e) {
269-
LOGGER.debug("Exception scanning method parameters", e);
270-
}
271-
if (addClass) {
272-
result.add(changedClass);
273-
}
274-
}
275-
return result;
276-
}
277-
278-
private List<Class<?>> detectRecordWithTypeAnnotation(
279-
ConfigurationComparer changes, List<Class<?>> changedClasses) {
280-
if (!JAVA_AT_LEAST_16) {
281-
// records introduced in JDK 16 (final version)
282-
return changedClasses;
283-
}
284-
List<Class<?>> result = new ArrayList<>();
285-
for (Class<?> changedClass : changedClasses) {
286-
boolean addClass = true;
287-
try {
288-
if (changedClass.getSuperclass() != null
289-
&& changedClass.getSuperclass().getTypeName().equals("java.lang.Record")
290-
&& Modifier.isFinal(changedClass.getModifiers())) {
291-
if (hasTypeAnnotationOnRecordComponent(changedClass)) {
292-
LOGGER.debug(
293-
"Record with type annotation detected, instrumentation not supported for {}",
294-
changedClass.getTypeName());
295-
reportError(
296-
changes,
297-
"Record with type annotation detected, instrumentation not supported for "
298-
+ changedClass.getTypeName());
299-
addClass = false;
300-
}
301-
}
302-
} catch (Exception e) {
303-
LOGGER.debug("Exception detecting record with type annotation", e);
304-
}
305-
if (addClass) {
306-
result.add(changedClass);
307-
}
308-
}
309-
return result;
310-
}
311-
312-
private boolean hasTypeAnnotationOnRecordComponent(Class<?> recordClass) {
313-
if (GET_RECORD_COMPONENTS_METHOD == null || GET_ANNOTATED_TYPES_METHOD == null) {
314-
return false;
315-
}
316-
try {
317-
Object recordComponentsArray = GET_RECORD_COMPONENTS_METHOD.invoke(recordClass);
318-
int len = Array.getLength(recordComponentsArray);
319-
for (int i = 0; i < len; i++) {
320-
Object recordComponent = Array.get(recordComponentsArray, i);
321-
AnnotatedType annotatedType =
322-
(AnnotatedType) GET_ANNOTATED_TYPES_METHOD.invoke(recordComponent);
323-
for (Annotation annotation : annotatedType.getAnnotations()) {
324-
Target annotationTarget = annotation.annotationType().getAnnotation(Target.class);
325-
if (annotationTarget != null
326-
&& Arrays.stream(annotationTarget.value())
327-
.anyMatch(it -> it == ElementType.TYPE_USE)) {
328-
return true;
329-
}
330-
}
331-
}
332-
return false;
333-
} catch (Exception ex) {
334-
return false;
335-
}
336-
}
337-
338227
private void reportReceived(ConfigurationComparer changes) {
339228
for (ProbeDefinition def : changes.getAddedDefinitions()) {
340229
if (def instanceof ExceptionProbe) {
@@ -461,4 +350,123 @@ Map<String, ProbeDefinition> getAppliedDefinitions() {
461350
Map<String, InstrumentationResult> getInstrumentationResults() {
462351
return instrumentationResults;
463352
}
353+
354+
private static class JDKVersionSpecificHelper {
355+
356+
public static List<Class<?>> detectRecordWithTypeAnnotation(
357+
Consumer<String> reportError, List<Class<?>> changedClasses) {
358+
if (!JAVA_AT_LEAST_16) {
359+
// records introduced in JDK 16 (final version)
360+
return changedClasses;
361+
}
362+
List<Class<?>> result = new ArrayList<>();
363+
for (Class<?> changedClass : changedClasses) {
364+
boolean addClass = true;
365+
try {
366+
if (changedClass.getSuperclass() != null
367+
&& changedClass.getSuperclass().getTypeName().equals("java.lang.Record")
368+
&& Modifier.isFinal(changedClass.getModifiers())) {
369+
if (hasTypeAnnotationOnRecordComponent(changedClass)) {
370+
LOGGER.debug(
371+
"Record with type annotation detected, instrumentation not supported for {}",
372+
changedClass.getTypeName());
373+
reportError.accept(
374+
"Record with type annotation detected, instrumentation not supported for "
375+
+ changedClass.getTypeName());
376+
addClass = false;
377+
}
378+
}
379+
} catch (Exception e) {
380+
LOGGER.debug("Exception detecting record with type annotation", e);
381+
}
382+
if (addClass) {
383+
result.add(changedClass);
384+
}
385+
}
386+
return result;
387+
}
388+
389+
private static boolean hasTypeAnnotationOnRecordComponent(Class<?> recordClass) {
390+
if (GET_RECORD_COMPONENTS_METHOD == null || GET_ANNOTATED_TYPES_METHOD == null) {
391+
return false;
392+
}
393+
try {
394+
Object recordComponentsArray = GET_RECORD_COMPONENTS_METHOD.invoke(recordClass);
395+
int len = Array.getLength(recordComponentsArray);
396+
for (int i = 0; i < len; i++) {
397+
Object recordComponent = Array.get(recordComponentsArray, i);
398+
AnnotatedType annotatedType =
399+
(AnnotatedType) GET_ANNOTATED_TYPES_METHOD.invoke(recordComponent);
400+
for (Annotation annotation : annotatedType.getAnnotations()) {
401+
Target annotationTarget = annotation.annotationType().getAnnotation(Target.class);
402+
if (annotationTarget != null
403+
&& Arrays.stream(annotationTarget.value())
404+
.anyMatch(it -> it == ElementType.TYPE_USE)) {
405+
return true;
406+
}
407+
}
408+
}
409+
return false;
410+
} catch (Exception ex) {
411+
return false;
412+
}
413+
}
414+
415+
/*
416+
* Because of this bug (https://bugs.openjdk.org/browse/JDK-8240908), classes compiled with
417+
* method parameters (javac -parameters) strip this attribute once retransformed
418+
* Spring 6/Spring boot 3 rely exclusively on this attribute and may throw an exception
419+
* if no attribute found.
420+
*/
421+
public static List<Class<?>> detectMethodParameters(
422+
Consumer<String> reportError,
423+
Instrumentation instrumentation,
424+
List<Class<?>> changedClasses) {
425+
if (JAVA_AT_LEAST_19) {
426+
// bug is fixed since JDK19, no need to perform detection
427+
return changedClasses;
428+
}
429+
List<Class<?>> result = new ArrayList<>();
430+
for (Class<?> changedClass : changedClasses) {
431+
boolean addClass = true;
432+
try {
433+
Method[] declaredMethods = changedClass.getDeclaredMethods();
434+
// capping scanning of methods to 100 to avoid generated class with thousand of methods
435+
// assuming that in those first 100 methods there is at least one with at least one
436+
// parameter
437+
for (int methodIdx = 0;
438+
methodIdx < declaredMethods.length && methodIdx < 100;
439+
methodIdx++) {
440+
Method method = declaredMethods[methodIdx];
441+
Parameter[] parameters = method.getParameters();
442+
if (parameters.length == 0) {
443+
continue;
444+
}
445+
if (parameters[0].isNamePresent()) {
446+
if (!SpringHelper.isSpringUsingOnlyMethodParameters(instrumentation)) {
447+
return changedClasses;
448+
}
449+
LOGGER.debug(
450+
"Detecting method parameter: method={} param={}, Skipping retransforming this class",
451+
method.getName(),
452+
parameters[0].getName());
453+
// skip the class: compiled with -parameters
454+
reportError.accept(
455+
"Method Parameters detected, instrumentation not supported for "
456+
+ changedClass.getTypeName());
457+
addClass = false;
458+
}
459+
// we found at leat a method with one parameter if name is not present we can stop there
460+
break;
461+
}
462+
} catch (Exception e) {
463+
LOGGER.debug("Exception scanning method parameters", e);
464+
}
465+
if (addClass) {
466+
result.add(changedClass);
467+
}
468+
}
469+
return result;
470+
}
471+
}
464472
}

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -197,10 +197,6 @@ public List<Snapshot> getSnapshots() {
197197
return snapshots;
198198
}
199199

200-
public boolean isSampling() {
201-
return !snapshots.isEmpty();
202-
}
203-
204200
public void addSnapshot(Snapshot snapshot) {
205201
snapshots.add(snapshot);
206202
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ public void evaluate(
108108
exceptionProbeManager.getStateByThrowable(innerMostThrowable);
109109
if (state != null) {
110110
// Already unwinding the exception
111-
if (!state.isSampling()) {
111+
if (state.getSnapshots().isEmpty()) {
112112
// skip snapshot because no snapshot from previous stack level
113113
return;
114114
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ private static boolean isSpringUsingOnlyMethodParametersSpecificClass(Instrument
7676
return false;
7777
}
7878

79-
private static class ParsedSpringVersion {
79+
static class ParsedSpringVersion {
8080
private static final Pattern VERSION_PATTERN = Pattern.compile("(\\d+)\\.(\\d+)\\.(\\d+)");
8181

8282
final int major;

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22

33
import static com.datadog.debugger.agent.ConfigurationAcceptor.Source.REMOTE_CONFIG;
44
import static com.datadog.debugger.agent.DebuggerProductChangesListener.LOG_PROBE_PREFIX;
5+
import static com.datadog.debugger.agent.DebuggerProductChangesListener.METRIC_PROBE_PREFIX;
6+
import static com.datadog.debugger.agent.DebuggerProductChangesListener.SPAN_DECORATION_PROBE_PREFIX;
7+
import static com.datadog.debugger.agent.DebuggerProductChangesListener.SPAN_PROBE_PREFIX;
58
import static com.datadog.debugger.probe.ProbeDefinitionDeserializer.deserializeLogProbe;
69
import static com.datadog.debugger.probe.ProbeDefinitionDeserializer.deserializeSpanDecorationProbe;
710
import static com.datadog.debugger.probe.ProbeDefinitionDeserializer.deserializeTriggerProbe;
@@ -643,7 +646,10 @@ public void handleException() {
643646
ConfigurationUpdater configurationUpdater = createConfigUpdater(debuggerSinkWithMockStatusSink);
644647
Exception ex = new Exception("oops");
645648
configurationUpdater.handleException(LOG_PROBE_PREFIX + PROBE_ID.getId(), ex);
646-
verify(probeStatusSink).addError(eq(ProbeId.from(PROBE_ID.getId() + ":0")), eq(ex));
649+
configurationUpdater.handleException(METRIC_PROBE_PREFIX + PROBE_ID.getId(), ex);
650+
configurationUpdater.handleException(SPAN_PROBE_PREFIX + PROBE_ID.getId(), ex);
651+
configurationUpdater.handleException(SPAN_DECORATION_PROBE_PREFIX + PROBE_ID.getId(), ex);
652+
verify(probeStatusSink, times(4)).addError(eq(ProbeId.from(PROBE_ID.getId() + ":0")), eq(ex));
647653
}
648654

649655
@Test

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,18 @@ public void kotlinSourceRemapper() {
3030
assertTrue(sourceRemapper instanceof SourceRemapper.KotlinSourceRemapper);
3131
assertEquals(24, sourceRemapper.remapSourceLine(42));
3232
}
33+
34+
@Test
35+
public void noKotlinDebug() {
36+
SourceMap sourceMapMock = mock(SourceMap.class);
37+
when(sourceMapMock.getDefaultStratumName()).thenReturn("Main");
38+
StratumExt stratumMainMock = mock(StratumExt.class);
39+
when(sourceMapMock.getStratum(eq("Kotlin"))).thenReturn(stratumMainMock);
40+
when(sourceMapMock.getStratum(eq("KotlinDebug"))).thenReturn(null);
41+
IllegalArgumentException illegalArgumentException =
42+
assertThrows(
43+
IllegalArgumentException.class,
44+
() -> SourceRemapper.getSourceRemapper("foo.kt", sourceMapMock));
45+
assertEquals("No stratumDebug found for KotlinDebug", illegalArgumentException.getMessage());
46+
}
3347
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -968,7 +968,6 @@ public void symbolExtraction15() throws IOException, URISyntaxException {
968968
}
969969

970970
@Test
971-
@EnabledForJreRange(max = JRE.JAVA_25)
972971
@DisabledIf(
973972
value = "datadog.environment.JavaVirtualMachine#isJ9",
974973
disabledReason = "Flaky on J9 JVMs")

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,4 +35,12 @@ void isSpringUsingOnlyMethodParametersFalseFallback() throws Exception {
3535
when(inst.getAllLoadedClasses()).thenReturn(new Class[0]);
3636
assertFalse(SpringHelper.isSpringUsingOnlyMethodParameters(inst));
3737
}
38+
39+
@Test
40+
void invalidSpringVersion() {
41+
IllegalArgumentException illegalArgumentException =
42+
assertThrows(
43+
IllegalArgumentException.class, () -> new SpringHelper.ParsedSpringVersion("foo"));
44+
assertEquals("Cannot parse SpringVersion: foo", illegalArgumentException.getMessage());
45+
}
3846
}

0 commit comments

Comments
 (0)