diff --git a/dd-java-agent/agent-ci-visibility/civisibility-instrumentation-test-fixtures/src/main/groovy/datadog/trace/civisibility/CiVisibilityInstrumentationTest.groovy b/dd-java-agent/agent-ci-visibility/civisibility-instrumentation-test-fixtures/src/main/groovy/datadog/trace/civisibility/CiVisibilityInstrumentationTest.groovy index e71a8a5d8c9..2d2a8261d64 100644 --- a/dd-java-agent/agent-ci-visibility/civisibility-instrumentation-test-fixtures/src/main/groovy/datadog/trace/civisibility/CiVisibilityInstrumentationTest.groovy +++ b/dd-java-agent/agent-ci-visibility/civisibility-instrumentation-test-fixtures/src/main/groovy/datadog/trace/civisibility/CiVisibilityInstrumentationTest.groovy @@ -15,7 +15,6 @@ import datadog.trace.api.civisibility.config.LibraryCapability import datadog.trace.api.civisibility.config.TestFQN import datadog.trace.api.civisibility.config.TestIdentifier import datadog.trace.api.civisibility.config.TestMetadata -import datadog.trace.api.civisibility.coverage.CoveragePerTestBridge import datadog.trace.api.civisibility.events.TestEventsHandler import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector import datadog.trace.api.civisibility.telemetry.tag.Provider @@ -201,8 +200,6 @@ abstract class CiVisibilityInstrumentationTest extends InstrumentationSpecificat InstrumentationBridge.registerBuildEventsHandlerFactory { decorator -> new BuildEventsHandlerImpl<>(buildSystemSessionFactory, new JvmInfoFactoryImpl()) } - - CoveragePerTestBridge.registerCoverageStoreRegistry(coverageStoreFactory) } private static final class MockExecutionSettingsFactory implements ExecutionSettingsFactory { diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilitySystem.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilitySystem.java index 246ebe2b336..9372b203156 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilitySystem.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilitySystem.java @@ -8,7 +8,6 @@ import datadog.trace.api.civisibility.DDTestSuite; import datadog.trace.api.civisibility.InstrumentationBridge; import datadog.trace.api.civisibility.config.LibraryCapability; -import datadog.trace.api.civisibility.coverage.CoveragePerTestBridge; import datadog.trace.api.civisibility.events.BuildEventsHandler; import datadog.trace.api.civisibility.events.TestEventsHandler; import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector; @@ -118,7 +117,6 @@ public static void start(Instrumentation inst, SharedCommunicationObjects sco) { TestEventsHandlerFactory testEventsHandlerFactory = new TestEventsHandlerFactory(services, repoServices, coverageServices, executionSettings); InstrumentationBridge.registerTestEventsHandlerFactory(testEventsHandlerFactory); - CoveragePerTestBridge.registerCoverageStoreRegistry(coverageServices.coverageStoreFactory); AgentTracer.TracerAPI tracerAPI = AgentTracer.get(); tracerAPI.addShutdownListener(testEventsHandlerFactory::shutdown); diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/SkippableAwareCoverageStoreFactory.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/SkippableAwareCoverageStoreFactory.java index be50bf82d7c..90de26c1be8 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/SkippableAwareCoverageStoreFactory.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/SkippableAwareCoverageStoreFactory.java @@ -31,9 +31,4 @@ public CoverageStore create(@Nullable TestIdentifier testIdentifier) { return delegate.create(testIdentifier); } } - - @Override - public void setTotalProbeCount(String className, int totalProbeCount) { - delegate.setTotalProbeCount(className, totalProbeCount); - } } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileCoverageStore.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileCoverageStore.java index 6de1354e7c7..dfd4017a70a 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileCoverageStore.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileCoverageStore.java @@ -118,10 +118,5 @@ public CoverageStore create(@Nullable TestIdentifier testIdentifier) { private FileProbes createProbes(boolean isTestThread) { return new FileProbes(metrics, isTestThread); } - - @Override - public void setTotalProbeCount(String className, int totalProbeCount) { - // no op - } } } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileProbes.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileProbes.java index ac53ecda24a..47192376abd 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileProbes.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileProbes.java @@ -31,11 +31,6 @@ public class FileProbes implements CoverageProbes { nonCodeResources = isTestThread ? new HashMap<>() : new ConcurrentHashMap<>(); } - @Override - public void record(Class clazz, long classId, int probeId) { - record(clazz); - } - @Override public void record(Class clazz) { try { diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/ExecutionDataAdapter.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/ExecutionDataAdapter.java index 6441eb8ded3..f35d8cfbe25 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/ExecutionDataAdapter.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/ExecutionDataAdapter.java @@ -5,21 +5,28 @@ public class ExecutionDataAdapter { private final long classId; private final String className; - // Unbounded data structure that only exists within a single test span + // Jacoco's shared probe array for the class, used to back-fill aggregate coverage at report time + private final boolean[] jacocoProbes; + // Per-test probe array that Jacoco's instrumentation writes into while a test is running private final boolean[] probeActivations; - public ExecutionDataAdapter(long classId, String className, int totalProbeCount) { + public ExecutionDataAdapter(long classId, String className, boolean[] jacocoProbes) { this.classId = classId; this.className = className; - this.probeActivations = new boolean[totalProbeCount]; + this.jacocoProbes = jacocoProbes; + this.probeActivations = new boolean[jacocoProbes.length]; } public String getClassName() { return className; } - void record(int probeId) { - probeActivations[probeId] = true; + long getClassId() { + return classId; + } + + boolean[] getProbeActivations() { + return probeActivations; } ExecutionDataAdapter merge(ExecutionDataAdapter other) { @@ -29,6 +36,31 @@ ExecutionDataAdapter merge(ExecutionDataAdapter other) { return this; } + /** + * Folds the per-test coverage back into Jacoco's shared probe array. Jacoco's aggregate coverage + * (used for total module/session coverage percentage and report uploads) no longer sees probes + * recorded into the per-test array directly, so they are OR-ed back here at report time. The + * write is monotonic (bits are only ever set), so concurrent back-fills from multiple tests are + * safe. + * + *

The per-test array is allocated when a method of the class is entered (so the probe array + * can be swapped in), which can happen even if no probe ends up firing (e.g. the method throws + * before reaching its first probe). Returning whether any probe was actually covered lets the + * caller skip such classes and avoid emitting empty coverage entries. + * + * @return {@code true} if at least one probe was covered by the test + */ + boolean mergeIntoJacocoProbes() { + boolean covered = false; + for (int i = 0; i < probeActivations.length; i++) { + if (probeActivations[i]) { + jacocoProbes[i] = true; + covered = true; + } + } + return covered; + } + ExecutionData toExecutionData() { return new ExecutionData(classId, className, probeActivations); } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineCoverageStore.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineCoverageStore.java index 647f28ca181..6eeebd083c9 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineCoverageStore.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineCoverageStore.java @@ -14,6 +14,7 @@ import datadog.trace.civisibility.source.Utils; import java.io.InputStream; import java.util.ArrayList; +import java.util.Arrays; import java.util.BitSet; import java.util.Collection; import java.util.HashMap; @@ -37,16 +38,27 @@ public class LineCoverageStore extends ConcurrentCoverageStore { private static final Logger log = LoggerFactory.getLogger(LineCoverageStore.class); + /** + * Upper bound on the number of cached class analyses. Coverage stays correct beyond it (analysis + * just isn't cached), this only guards memory for pathologically large suites. + */ + private static final int MAX_ANALYSIS_CACHE_ENTRIES = 50_000; + private final CiVisibilityMetricCollector metrics; private final SourcePathResolver sourcePathResolver; + // Module-wide cache: (class id + probe set) -> covered lines, shared across tests so a class + // covered identically by many tests is parsed by Jacoco's Analyzer only once. + private final Map analysisCache; private LineCoverageStore( Function probesFactory, CiVisibilityMetricCollector metrics, - SourcePathResolver sourcePathResolver) { + SourcePathResolver sourcePathResolver, + Map analysisCache) { super(probesFactory); this.metrics = metrics; this.sourcePathResolver = sourcePathResolver; + this.analysisCache = analysisCache; } @Nullable @@ -70,6 +82,12 @@ protected TestReport report( Map coveredLinesBySourcePath = new HashMap<>(); for (Map.Entry, ExecutionDataAdapter> e : combinedExecutionData.entrySet()) { ExecutionDataAdapter executionDataAdapter = e.getValue(); + // Back-fill Jacoco's aggregate coverage (total coverage percentage and report uploads). Skip + // classes with no covered probes: the per-test array is allocated on method entry, so a + // method that throws before its first probe fires would otherwise yield an empty entry. + if (!executionDataAdapter.mergeIntoJacocoProbes()) { + continue; + } String className = executionDataAdapter.getClassName(); Class clazz = e.getKey(); @@ -83,24 +101,9 @@ protected TestReport report( } String sourcePath = sourcePaths.iterator().next(); - try (InputStream is = Utils.getClassStream(clazz)) { - BitSet coveredLines = - coveredLinesBySourcePath.computeIfAbsent(sourcePath, key -> new BitSet()); - ExecutionDataStore store = new ExecutionDataStore(); - store.put(executionDataAdapter.toExecutionData()); - - // TODO optimize this part to avoid parsing - // the same class multiple times for different test cases - Analyzer analyzer = new Analyzer(store, new SourceAnalyzer(coveredLines)); - analyzer.analyzeClass(is, null); - - } catch (Exception exception) { - log.debug( - "Skipping coverage reporting for {} ({}) because of error", - className, - sourcePath, - exception); - metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1); + BitSet coveredLines = analyzeClass(clazz, executionDataAdapter); + if (coveredLines != null) { + coveredLinesBySourcePath.computeIfAbsent(sourcePath, key -> new BitSet()).or(coveredLines); } } @@ -132,12 +135,81 @@ protected TestReport report( return report; } - public static final class Factory implements CoverageStore.Factory { + /** + * Resolves the covered lines for a class given a test's probe activations. Parsing the class with + * Jacoco's {@link Analyzer} is the dominant cost of reporting, and the result depends only on the + * class bytecode and the probe set, so it is memoized: the same class covered identically by + * different tests is parsed once. + * + * @return the covered lines, or {@code null} if the class could not be analyzed + */ + @Nullable + private BitSet analyzeClass(Class clazz, ExecutionDataAdapter executionDataAdapter) { + AnalysisCacheKey key = + new AnalysisCacheKey( + executionDataAdapter.getClassId(), executionDataAdapter.getProbeActivations()); + BitSet cached = analysisCache.get(key); + if (cached != null) { + return cached; + } + + try (InputStream is = Utils.getClassStream(clazz)) { + BitSet coveredLines = new BitSet(); + ExecutionDataStore store = new ExecutionDataStore(); + store.put(executionDataAdapter.toExecutionData()); + Analyzer analyzer = new Analyzer(store, new SourceAnalyzer(coveredLines)); + analyzer.analyzeClass(is, null); + + if (analysisCache.size() < MAX_ANALYSIS_CACHE_ENTRIES) { + analysisCache.putIfAbsent(key, coveredLines); + } + return coveredLines; + + } catch (Exception exception) { + log.debug( + "Skipping coverage reporting for {} because of error", + executionDataAdapter.getClassName(), + exception); + metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1); + return null; + } + } - private final Map probeCounts = new ConcurrentHashMap<>(); + /** Cache key identifying a class (by Jacoco class id) covered by a specific set of probes. */ + static final class AnalysisCacheKey { + private final long classId; + private final boolean[] probes; + private final int hash; + + AnalysisCacheKey(long classId, boolean[] probes) { + this.classId = classId; + this.probes = probes; + this.hash = 31 * Long.hashCode(classId) + Arrays.hashCode(probes); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof AnalysisCacheKey)) { + return false; + } + AnalysisCacheKey other = (AnalysisCacheKey) o; + return classId == other.classId && hash == other.hash && Arrays.equals(probes, other.probes); + } + + @Override + public int hashCode() { + return hash; + } + } + + public static final class Factory implements CoverageStore.Factory { private final CiVisibilityMetricCollector metrics; private final SourcePathResolver sourcePathResolver; + private final Map analysisCache = new ConcurrentHashMap<>(); public Factory(CiVisibilityMetricCollector metrics, SourcePathResolver sourcePathResolver) { this.metrics = metrics; @@ -146,16 +218,11 @@ public Factory(CiVisibilityMetricCollector metrics, SourcePathResolver sourcePat @Override public CoverageStore create(@Nullable TestIdentifier testIdentifier) { - return new LineCoverageStore(this::createProbes, metrics, sourcePathResolver); + return new LineCoverageStore(this::createProbes, metrics, sourcePathResolver, analysisCache); } private LineProbes createProbes(boolean isTestThread) { - return new LineProbes(metrics, probeCounts, isTestThread); - } - - @Override - public void setTotalProbeCount(String className, int totalProbeCount) { - probeCounts.put(className.replace('/', '.'), totalProbeCount); + return new LineProbes(metrics, isTestThread); } } } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineProbes.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineProbes.java index f99c78e2c29..f96c0b5f435 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineProbes.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineProbes.java @@ -19,7 +19,6 @@ public class LineProbes implements CoverageProbes { private final CiVisibilityMetricCollector metrics; - private final Map probeCounts; private final Map, ExecutionDataAdapter> executionData; private final Map nonCodeResources; @@ -27,10 +26,8 @@ public class LineProbes implements CoverageProbes { private Class lastCoveredClass; private ExecutionDataAdapter lastCoveredExecutionData; - LineProbes( - CiVisibilityMetricCollector metrics, Map probeCounts, boolean isTestThread) { + LineProbes(CiVisibilityMetricCollector metrics, boolean isTestThread) { this.metrics = metrics; - this.probeCounts = probeCounts; executionData = isTestThread ? new IdentityHashMap<>() : new ConcurrentHashMap<>(); nonCodeResources = isTestThread ? new HashMap<>() : new ConcurrentHashMap<>(); } @@ -41,20 +38,21 @@ public void record(Class clazz) { } @Override - public void record(Class clazz, long classId, int probeId) { + public boolean[] resolveProbeArray(Class clazz, long classId, boolean[] jacocoProbes) { try { if (lastCoveredClass != clazz) { - // optimization to avoid map lookup if activating several probes for same class in a row + // optimization to avoid map lookup if resolving the array for the same class in a row lastCoveredExecutionData = executionData.computeIfAbsent( lastCoveredClass = clazz, - k -> new ExecutionDataAdapter(classId, k.getName(), probeCounts.get(k.getName()))); + k -> new ExecutionDataAdapter(classId, k.getName(), jacocoProbes)); } - lastCoveredExecutionData.record(probeId); + return lastCoveredExecutionData.getProbeActivations(); } catch (Exception e) { metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.RECORD); - throw e; + // fall back to Jacoco's shared array so coverage is still recorded + return jacocoProbes; } } diff --git a/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/ExecutionDataAdapterTest.java b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/ExecutionDataAdapterTest.java new file mode 100644 index 00000000000..44d5708fcf2 --- /dev/null +++ b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/ExecutionDataAdapterTest.java @@ -0,0 +1,71 @@ +package datadog.trace.civisibility.coverage.line; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class ExecutionDataAdapterTest { + + @Test + void probeActivationsAreSizedFromJacocoArrayAndStartEmpty() { + boolean[] jacocoProbes = new boolean[4]; + ExecutionDataAdapter adapter = new ExecutionDataAdapter(1L, "com/example/Foo", jacocoProbes); + + boolean[] probeActivations = adapter.getProbeActivations(); + assertEquals(4, probeActivations.length); + assertArrayEquals(new boolean[4], probeActivations); + } + + @Test + void mergeIntoJacocoProbesOrsPerTestBitsBack() { + boolean[] jacocoProbes = new boolean[4]; + // a bit that was already set on the shared array (e.g. covered outside any test) + jacocoProbes[0] = true; + ExecutionDataAdapter adapter = new ExecutionDataAdapter(1L, "com/example/Foo", jacocoProbes); + + // simulate Jacoco's native probes writing into the per-test array + adapter.getProbeActivations()[2] = true; + + adapter.mergeIntoJacocoProbes(); + + // existing shared bit is preserved, per-test bit is folded back, untouched probes stay false + assertArrayEquals(new boolean[] {true, false, true, false}, jacocoProbes); + } + + @Test + void mergeIntoJacocoProbesNeverClearsBits() { + boolean[] jacocoProbes = new boolean[] {true, true, true, true}; + ExecutionDataAdapter adapter = new ExecutionDataAdapter(1L, "com/example/Foo", jacocoProbes); + // per-test array is all false; merging it back must not clear any shared bits + adapter.mergeIntoJacocoProbes(); + assertArrayEquals(new boolean[] {true, true, true, true}, jacocoProbes); + } + + @Test + void mergeCombinesProbeActivationsFromAnotherAdapter() { + boolean[] jacocoProbes = new boolean[4]; + ExecutionDataAdapter a = new ExecutionDataAdapter(1L, "com/example/Foo", jacocoProbes); + ExecutionDataAdapter b = new ExecutionDataAdapter(1L, "com/example/Foo", jacocoProbes); + a.getProbeActivations()[1] = true; + b.getProbeActivations()[3] = true; + + ExecutionDataAdapter merged = a.merge(b); + + assertTrue(merged.getProbeActivations()[1]); + assertTrue(merged.getProbeActivations()[3]); + assertFalse(merged.getProbeActivations()[0]); + } + + @Test + void toExecutionDataExposesPerTestProbes() { + boolean[] jacocoProbes = new boolean[3]; + ExecutionDataAdapter adapter = new ExecutionDataAdapter(7L, "com/example/Foo", jacocoProbes); + adapter.getProbeActivations()[1] = true; + + assertEquals(7L, adapter.toExecutionData().getId()); + assertArrayEquals(new boolean[] {false, true, false}, adapter.toExecutionData().getProbes()); + } +} diff --git a/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/LineCoverageStoreTest.java b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/LineCoverageStoreTest.java new file mode 100644 index 00000000000..8928e7587bb --- /dev/null +++ b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/LineCoverageStoreTest.java @@ -0,0 +1,86 @@ +package datadog.trace.civisibility.coverage.line; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import datadog.trace.api.DDTraceId; +import datadog.trace.api.civisibility.coverage.CoverageProbes; +import datadog.trace.api.civisibility.coverage.CoverageStore; +import datadog.trace.api.civisibility.coverage.TestReport; +import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector; +import datadog.trace.civisibility.source.SourcePathResolver; +import org.junit.jupiter.api.Test; + +class LineCoverageStoreTest { + + private static final class CoveredClass {} + + @Test + void reportFoldsPerTestCoverageBackIntoJacocoSharedArray() { + CiVisibilityMetricCollector metrics = mock(CiVisibilityMetricCollector.class); + SourcePathResolver sourcePathResolver = mock(SourcePathResolver.class); + // source path resolution is irrelevant here: the aggregate back-fill happens regardless + when(sourcePathResolver.getSourcePaths(any())).thenReturn(emptyList()); + + CoverageStore store = new LineCoverageStore.Factory(metrics, sourcePathResolver).create(null); + CoverageProbes probes = store.getProbes(); + + boolean[] jacocoProbes = new boolean[5]; + boolean[] perTest = probes.resolveProbeArray(CoveredClass.class, 42L, jacocoProbes); + // simulate Jacoco's native probes recording coverage into the per-test array + perTest[3] = true; + + // before report, Jacoco's shared (aggregate) array is untouched + assertFalse(jacocoProbes[3]); + + store.report(DDTraceId.ONE, 1L, 1L); + + // after report the per-test coverage is folded back so Jacoco's aggregate stays accurate + assertTrue(jacocoProbes[3]); + } + + @Test + void reportSkipsClassesWhoseProbeArrayWasResolvedButNeverWritten() { + CiVisibilityMetricCollector metrics = mock(CiVisibilityMetricCollector.class); + SourcePathResolver sourcePathResolver = mock(SourcePathResolver.class); + // resolve the source path so the class would be reported if it were not skipped + when(sourcePathResolver.getSourcePaths(any())) + .thenReturn(singletonList("src/test/java/datadog/smoke/CoveredClass.java")); + + CoverageStore store = new LineCoverageStore.Factory(metrics, sourcePathResolver).create(null); + CoverageProbes probes = store.getProbes(); + // a method was entered (probe array resolved) but it threw before any probe fired + probes.resolveProbeArray(CoveredClass.class, 42L, new boolean[5]); + + boolean coverageGathered = store.report(DDTraceId.ONE, 1L, 1L); + + assertFalse( + coverageGathered, "a test that covered no probes must not produce a coverage report"); + TestReport report = store.getReport(); + if (report != null) { + assertTrue(report.getTestReportFileEntries().isEmpty()); + } + } + + @Test + void analysisCacheKeyDistinguishesClassesAndProbeSets() { + boolean[] probes = {true, false, true}; + boolean[] sameProbes = {true, false, true}; + boolean[] otherProbes = {true, true, true}; + + LineCoverageStore.AnalysisCacheKey key = new LineCoverageStore.AnalysisCacheKey(1L, probes); + // identical class id + probe contents must collide so the analysis is reused + assertEquals(key, new LineCoverageStore.AnalysisCacheKey(1L, sameProbes)); + assertEquals(key.hashCode(), new LineCoverageStore.AnalysisCacheKey(1L, sameProbes).hashCode()); + // a different class or a different probe set must NOT hit the same cache entry + assertNotEquals(key, new LineCoverageStore.AnalysisCacheKey(2L, probes)); + assertNotEquals(key, new LineCoverageStore.AnalysisCacheKey(1L, otherProbes)); + } +} diff --git a/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/LineProbesTest.java b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/LineProbesTest.java new file mode 100644 index 00000000000..7ea3bb5a65e --- /dev/null +++ b/dd-java-agent/agent-ci-visibility/src/test/java/datadog/trace/civisibility/coverage/line/LineProbesTest.java @@ -0,0 +1,67 @@ +package datadog.trace.civisibility.coverage.line; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +import datadog.trace.api.civisibility.telemetry.CiVisibilityMetricCollector; +import org.junit.jupiter.api.Test; + +class LineProbesTest { + + private static final class ClassA {} + + private static final class ClassB {} + + private final CiVisibilityMetricCollector metrics = mock(CiVisibilityMetricCollector.class); + + @Test + void resolvesAPerTestArraySizedFromJacocoArray() { + LineProbes probes = new LineProbes(metrics, true); + boolean[] jacocoProbes = new boolean[6]; + + boolean[] perTest = probes.resolveProbeArray(ClassA.class, 1L, jacocoProbes); + + assertNotSame(jacocoProbes, perTest, "should not record into Jacoco's shared array"); + assertEquals(jacocoProbes.length, perTest.length); + } + + @Test + void returnsTheSameArrayForRepeatedResolutionsOfTheSameClass() { + LineProbes probes = new LineProbes(metrics, true); + boolean[] jacocoProbes = new boolean[3]; + + boolean[] first = probes.resolveProbeArray(ClassA.class, 1L, jacocoProbes); + boolean[] second = probes.resolveProbeArray(ClassA.class, 1L, jacocoProbes); + + assertSame(first, second); + } + + @Test + void keepsSeparateArraysPerClass() { + LineProbes probes = new LineProbes(metrics, true); + + boolean[] a = probes.resolveProbeArray(ClassA.class, 1L, new boolean[2]); + boolean[] b = probes.resolveProbeArray(ClassB.class, 2L, new boolean[2]); + + assertNotSame(a, b); + assertEquals(2, probes.getExecutionData().size()); + assertTrue(probes.getExecutionData().containsKey(ClassA.class)); + assertTrue(probes.getExecutionData().containsKey(ClassB.class)); + } + + @Test + void perTestWritesDoNotLeakIntoJacocoArrayBeforeReport() { + LineProbes probes = new LineProbes(metrics, true); + boolean[] jacocoProbes = new boolean[4]; + + boolean[] perTest = probes.resolveProbeArray(ClassA.class, 1L, jacocoProbes); + perTest[1] = true; + + // the shared array is only updated at report time via + // ExecutionDataAdapter#mergeIntoJacocoProbes + assertEquals(false, jacocoProbes[1]); + } +} diff --git a/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/ClassInstrumenterInstrumentation.java b/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/ClassInstrumenterInstrumentation.java deleted file mode 100644 index 7960a0ff40e..00000000000 --- a/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/ClassInstrumenterInstrumentation.java +++ /dev/null @@ -1,57 +0,0 @@ -package datadog.trace.instrumentation.jacoco; - -import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; -import static net.bytebuddy.matcher.ElementMatchers.isMethod; -import static net.bytebuddy.matcher.ElementMatchers.nameEndsWith; -import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; - -import com.google.auto.service.AutoService; -import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.agent.tooling.InstrumenterModule; -import datadog.trace.api.Config; -import datadog.trace.api.civisibility.coverage.CoveragePerTestBridge; -import net.bytebuddy.asm.Advice; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.matcher.ElementMatcher; - -@AutoService(InstrumenterModule.class) -public class ClassInstrumenterInstrumentation extends InstrumenterModule.CiVisibility - implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice { - public ClassInstrumenterInstrumentation() { - super("jacoco"); - } - - @Override - public boolean isEnabled() { - return super.isEnabled() && Config.get().isCiVisibilityCoverageLinesEnabled(); - } - - @Override - public String hierarchyMarkerType() { - return "org.jacoco.agent.rt.IAgent"; - } - - @Override - public ElementMatcher hierarchyMatcher() { - // The jacoco javaagent jar that is published relocates internal classes to an "obfuscated" - // package name ex. org.jacoco.agent.rt.internal_72ddf3b.core.internal.instr.ClassInstrumenter - return nameStartsWith("org.jacoco.agent.rt.internal") - .and(nameEndsWith(".core.internal.instr.ClassInstrumenter")); - } - - @Override - public void methodAdvice(MethodTransformer transformer) { - transformer.applyAdvice( - isMethod().and(named("visitTotalProbeCount")), - getClass().getName() + "$VisitTotalProbeCountAdvice"); - } - - public static class VisitTotalProbeCountAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - static void enter( - @Advice.FieldValue(value = "className") final String className, - @Advice.Argument(0) int count) { - CoveragePerTestBridge.setTotalProbeCount(className, count); - } - } -} diff --git a/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/MethodVisitorWrapper.java b/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/MethodVisitorWrapper.java index 8b80d00ce30..53b394ab997 100644 --- a/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/MethodVisitorWrapper.java +++ b/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/MethodVisitorWrapper.java @@ -17,6 +17,7 @@ public class MethodVisitorWrapper { private static final MethodHandle visitMethodInsnHandle; private static final MethodHandle visitInsnHandle; private static final MethodHandle visitIntInsnHandle; + private static final MethodHandle visitVarInsnHandle; private static final MethodHandle visitLdcInsnHandle; private static final MethodHandle getTypeHandle; @@ -43,6 +44,8 @@ public class MethodVisitorWrapper { visitInsnHandle = accessMethod(lookup, shadedMethodVisitorClass, "visitInsn", int.class); visitIntInsnHandle = accessMethod(lookup, shadedMethodVisitorClass, "visitIntInsn", int.class, int.class); + visitVarInsnHandle = + accessMethod(lookup, shadedMethodVisitorClass, "visitVarInsn", int.class, int.class); visitLdcInsnHandle = accessMethod(lookup, shadedMethodVisitorClass, "visitLdcInsn", Object.class); @@ -97,6 +100,10 @@ public void visitMethodInsn(int opcode, String owner, String name, String desc, visitMethodInsnHandle.invoke(mv, opcode, owner, name, desc, itf); } + public void visitVarInsn(int opcode, int var) throws Throwable { + visitVarInsnHandle.invoke(mv, opcode, var); + } + public void visitLdcInsn(Object cst) throws Throwable { visitLdcInsnHandle.invoke(mv, cst); } diff --git a/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/ProbeInserterInstrumentation.java b/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/ProbeInserterInstrumentation.java index 695b659d6e1..ad7929de302 100644 --- a/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/ProbeInserterInstrumentation.java +++ b/dd-java-agent/instrumentation/jacoco-0.8.9/src/main/java/datadog/trace/instrumentation/jacoco/ProbeInserterInstrumentation.java @@ -112,29 +112,17 @@ public ElementMatcher hierarchyMatcher() { @Override public void methodAdvice(MethodTransformer transformer) { transformer.applyAdvice( - isMethod().and(named("visitMaxs")).and(takesArguments(2)).and(takesArgument(0, int.class)), - getClass().getName() + "$VisitMaxsAdvice"); - transformer.applyAdvice( - isMethod() - .and(named("insertProbe")) - .and(takesArguments(1)) - .and(takesArgument(0, int.class)), - getClass().getName() + "$InsertProbeAdvice"); - } - - public static class VisitMaxsAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - static void enter(@Advice.Argument(value = 0, readOnly = false) int maxStack) { - maxStack = maxStack + 2; - } + isMethod().and(named("visitCode")).and(takesArguments(0)), + getClass().getName() + "$VisitCodeAdvice"); } - public static class InsertProbeAdvice { - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static class VisitCodeAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) static void exit( @Advice.FieldValue(value = "mv") final Object mv, @Advice.FieldValue(value = "arrayStrategy") final Object arrayStrategy, - @Advice.Argument(0) final int id) + @Advice.FieldValue(value = "variable") final int variable, + @Advice.FieldValue(value = "accessorStackSize", readOnly = false) int accessorStackSize) throws Throwable { Field classNameField = arrayStrategy.getClass().getDeclaredField("className"); classNameField.setAccessible(true); @@ -167,16 +155,24 @@ static void exit( MethodVisitorWrapper methodVisitor = MethodVisitorWrapper.wrap(mv); + // Jacoco's storeInstance() has just stored the class' shared probe array into local variable + // `variable`. Swap it for the per-test array so Jacoco's own probe writes + // (probes[id] = true) record per-test coverage with no per-probe overhead. When no test is + // active the bridge returns the shared array unchanged, preserving Jacoco's aggregate. + methodVisitor.visitVarInsn(Opcodes.ALOAD, variable); methodVisitor.pushClass(className); methodVisitor.visitLdcInsn(classId); - methodVisitor.push(id); - methodVisitor.visitMethodInsn( Opcodes.INVOKESTATIC, "datadog/trace/api/civisibility/coverage/CoveragePerTestBridge", - "recordCoverage", - "(Ljava/lang/Class;JI)V", + "resolveProbeArray", + "([ZLjava/lang/Class;J)[Z", false); + methodVisitor.visitVarInsn(Opcodes.ASTORE, variable); + + // the swap leaves 4 slots on the stack (boolean[] + Class + long); Jacoco sizes the method's + // max stack as max(maxStack + 3, accessorStackSize) in visitMaxs + accessorStackSize = Math.max(accessorStackSize, 4); } } } diff --git a/internal-api/build.gradle.kts b/internal-api/build.gradle.kts index 6e99abe89d2..cb6e5f4f0e2 100644 --- a/internal-api/build.gradle.kts +++ b/internal-api/build.gradle.kts @@ -127,6 +127,7 @@ extra["excludedClassesCoverage"] = listOf( "datadog.trace.api.civisibility.coverage.CoveragePerTestBridge", "datadog.trace.api.civisibility.coverage.CoveragePerTestBridge.TotalProbeCount", "datadog.trace.api.civisibility.coverage.CoveragePercentageBridge", + "datadog.trace.api.civisibility.coverage.CoverageProbes", "datadog.trace.api.civisibility.coverage.NoOpCoverageStore", "datadog.trace.api.civisibility.coverage.NoOpCoverageStore.Factory", "datadog.trace.api.civisibility.coverage.NoOpProbes", diff --git a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoveragePerTestBridge.java b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoveragePerTestBridge.java index c8f52293eec..744b50b6ac7 100644 --- a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoveragePerTestBridge.java +++ b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoveragePerTestBridge.java @@ -2,73 +2,14 @@ import datadog.trace.api.civisibility.InstrumentationTestBridge; import datadog.trace.api.civisibility.domain.TestContext; -import java.util.ArrayDeque; -import java.util.Queue; -import javax.annotation.Nonnull; -import javax.annotation.concurrent.GuardedBy; public abstract class CoveragePerTestBridge { private static final ThreadLocal COVERAGE_PROBES = new ThreadLocal<>(); - private static volatile CoverageStore.Registry COVERAGE_STORE_REGISTRY; - private static final Object COVERAGE_STORE_REGISTRY_LOCK = new Object(); - - @GuardedBy("COVERAGE_STORE_REGISTRY_LOCK") - private static final Queue DEFERRED_PROBE_COUNTS = new ArrayDeque<>(); - - public static void registerCoverageStoreRegistry( - @Nonnull CoverageStore.Registry coverageStoreRegistry) { - synchronized (COVERAGE_STORE_REGISTRY_LOCK) { - while (!DEFERRED_PROBE_COUNTS.isEmpty()) { - TotalProbeCount c = DEFERRED_PROBE_COUNTS.poll(); - coverageStoreRegistry.setTotalProbeCount(c.className, c.count); - } - COVERAGE_STORE_REGISTRY = coverageStoreRegistry; - } - } - - /** - * {@link #COVERAGE_STORE_REGISTRY} is set when CI Visibility is initialized. It is possible, that - * core/internal JDK classes are loaded and transformed by Jacoco before this happens. As the - * result this method may be called when {@link #COVERAGE_STORE_REGISTRY} is still {@code null}. - * - *

While instrumenting core/internal JDK classes with Jacoco makes little sense, we do not - * always have the control over the users' Jacoco {@code includes} setting, therefore we have to - * account for this case and support it. - * - *

If this method finds {@link #COVERAGE_STORE_REGISTRY} to be {@code null}, the probe counts - * are saved in {@link #DEFERRED_PROBE_COUNTS} to be processed when {@link - * #COVERAGE_STORE_REGISTRY} is set. - */ - public static void setTotalProbeCount(String className, int totalProbeCount) { - if (COVERAGE_STORE_REGISTRY != null) { - COVERAGE_STORE_REGISTRY.setTotalProbeCount(className, totalProbeCount); - return; - } - - synchronized (COVERAGE_STORE_REGISTRY_LOCK) { - if (COVERAGE_STORE_REGISTRY != null) { - COVERAGE_STORE_REGISTRY.setTotalProbeCount(className, totalProbeCount); - } else { - DEFERRED_PROBE_COUNTS.offer(new TotalProbeCount(className, totalProbeCount)); - } - } - } - - private static final class TotalProbeCount { - private final String className; - private final int count; - - private TotalProbeCount(String className, int count) { - this.className = className; - this.count = count; - } - } - - /* This method is referenced by name in bytecode added in jacoco instrumentation module (see datadog.trace.instrumentation.jacoco.ProbeInserterInstrumentation.InsertProbeAdvice) */ - public static void recordCoverage(Class clazz, long classId, int probeId) { - getCurrentCoverageProbes().record(clazz, classId, probeId); + /* This method is referenced by name in bytecode added in jacoco instrumentation module (see datadog.trace.instrumentation.jacoco.ProbeInserterInstrumentation.VisitCodeAdvice) */ + public static boolean[] resolveProbeArray(boolean[] jacocoProbes, Class clazz, long classId) { + return getCurrentCoverageProbes().resolveProbeArray(clazz, classId, jacocoProbes); } /* This method is referenced by name in bytecode added by coverage probes (see datadog.trace.civisibility.coverage.instrumentation.CoverageUtils#insertCoverageProbe) */ diff --git a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoverageProbes.java b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoverageProbes.java index 2bdb5ca2371..54535de4bc6 100644 --- a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoverageProbes.java +++ b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoverageProbes.java @@ -3,7 +3,20 @@ public interface CoverageProbes { void record(Class clazz); - void record(Class clazz, long classId, int probeId); + /** + * Resolves the probe array that Jacoco's instrumentation writes into at runtime. Called once per + * instrumented method invocation, allowing per-test coverage to be captured by swapping Jacoco's + * shared probe array for a test-scoped one. The default returns {@code jacocoProbes} unchanged so + * that, when no per-test store is active, Jacoco's own aggregate coverage keeps working. + * + * @param clazz the class being executed + * @param classId Jacoco's class identifier + * @param jacocoProbes Jacoco's shared probe array for the class + * @return the probe array to record coverage into + */ + default boolean[] resolveProbeArray(Class clazz, long classId, boolean[] jacocoProbes) { + return jacocoProbes; + } void recordNonCodeResource(String absolutePath); } diff --git a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoverageStore.java b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoverageStore.java index a779c699281..cc42d28aa98 100644 --- a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoverageStore.java +++ b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/CoverageStore.java @@ -13,11 +13,7 @@ public interface CoverageStore extends TestReportHolder { */ boolean report(DDTraceId testSessionId, Long testSuiteId, long testSpanId); - interface Factory extends Registry { + interface Factory { CoverageStore create(@Nullable TestIdentifier testIdentifier); } - - interface Registry { - void setTotalProbeCount(String className, int totalProbeCount); - } } diff --git a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/NoOpCoverageStore.java b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/NoOpCoverageStore.java index c2b677f416f..7c709eaff1e 100644 --- a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/NoOpCoverageStore.java +++ b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/NoOpCoverageStore.java @@ -31,10 +31,5 @@ public static final class Factory implements CoverageStore.Factory { public CoverageStore create(@Nullable TestIdentifier testIdentifier) { return INSTANCE; } - - @Override - public void setTotalProbeCount(String className, int totalProbeCount) { - // no op - } } } diff --git a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/NoOpProbes.java b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/NoOpProbes.java index cdc2142e398..dfd55855250 100644 --- a/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/NoOpProbes.java +++ b/internal-api/src/main/java/datadog/trace/api/civisibility/coverage/NoOpProbes.java @@ -8,9 +8,6 @@ private NoOpProbes() {} @Override public void record(Class clazz) {} - @Override - public void record(Class clazz, long classId, int probeId) {} - @Override public void recordNonCodeResource(String absolutePath) {} }