Skip to content

Commit c88f535

Browse files
nikita-tkachenko-datadognikita-tkachenko-datadog
authored
Fix root coverage packages calculation in large repositories (DataDog#6317)
1 parent d1eda60 commit c88f535

7 files changed

Lines changed: 61 additions & 29 deletions

File tree

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilitySystem.java‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -177,10 +177,17 @@ private static DDBuildSystemSession.Factory buildSystemSessionFactory(
177177
ResourceResolver resourceResolver =
178178
new ConventionBasedResourceResolver(
179179
fileSystem, config.getCiVisibilityResourceFolderNames());
180-
RepoIndexBuilder indexBuilder =
181-
new RepoIndexBuilder(repoRoot, packageResolver, resourceResolver, fileSystem);
182180

183-
SourcePathResolver sourcePathResolver = getSourcePathResolver(repoRoot, indexBuilder);
181+
// scanning only the project folder and not the entire repo to
182+
// save time and to properly determine root packages for coverage instrumentation
183+
// (when there are multiple projects in the repo, root package resolution cannot establish
184+
// common roots)
185+
String sourcePathResolutionRoot = projectRoot.toString();
186+
RepoIndexBuilder indexBuilder =
187+
new RepoIndexBuilder(
188+
config, sourcePathResolutionRoot, packageResolver, resourceResolver, fileSystem);
189+
SourcePathResolver sourcePathResolver =
190+
getSourcePathResolver(sourcePathResolutionRoot, indexBuilder);
184191
Codeowners codeowners = getCodeowners(repoRoot);
185192

186193
MethodLinesResolver methodLinesResolver =
@@ -307,7 +314,7 @@ private static DDTestFrameworkSession.Factory testFrameworkSessionFactory(
307314
new ConventionBasedResourceResolver(
308315
fileSystem, config.getCiVisibilityResourceFolderNames());
309316
RepoIndexProvider indexProvider =
310-
new RepoIndexBuilder(repoRoot, packageResolver, resourceResolver, fileSystem);
317+
new RepoIndexBuilder(config, repoRoot, packageResolver, resourceResolver, fileSystem);
311318

312319
BackendApi backendApi = backendApiFactory.createBackendApi();
313320
GitDataUploader gitDataUploader =
@@ -389,7 +396,7 @@ private static CIVisibility.SessionFactory apiSessionFactory(
389396
new ConventionBasedResourceResolver(
390397
fileSystem, config.getCiVisibilityResourceFolderNames());
391398
RepoIndexProvider indexProvider =
392-
new RepoIndexBuilder(repoRoot, packageResolver, resourceResolver, fileSystem);
399+
new RepoIndexBuilder(config, repoRoot, packageResolver, resourceResolver, fileSystem);
393400
SourcePathResolver sourcePathResolver = getSourcePathResolver(repoRoot, indexProvider);
394401

395402
return new DDTestSessionImpl(

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/PackageTree.java‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package datadog.trace.civisibility.source.index;
22

3+
import datadog.trace.api.Config;
34
import java.nio.file.Path;
45
import java.util.ArrayDeque;
56
import java.util.ArrayList;
@@ -18,10 +19,10 @@
1819
* packages a/b, a/b/c, a/b/d, all of which contain classes, only a/b will be retained - a/b/c and
1920
* a/b/d will be discarded as redundant).
2021
*
21-
* <p>The length of the list is limited by {@link #MAX_CHILDREN_LEAVES}. If there are more packages
22-
* present than the limit, coarsening will happen (for instance, if there are packages a/b/c, a/b/d,
23-
* b/c/d, b/c/e/f and the limit is 2, the packages will be coarsened and a/b, b/c will be retained
24-
* as the result).
22+
* <p>The limit on the length of the list is configurable. If there are more packages present than
23+
* the limit, coarsening will happen (for instance, if there are packages a/b/c, a/b/d, b/c/d,
24+
* b/c/e/f and the limit is 2, the packages will be coarsened and a/b, b/c will be retained as the
25+
* result).
2526
*
2627
* <p>The intent is to be as specific as possible without exceeding the limit, so coarsening applies
2728
* first to the "longest" package names (not in terms of the actual string length, but in terms of
@@ -30,10 +31,14 @@
3031
*/
3132
public class PackageTree {
3233

33-
private static final int MAX_CHILDREN_LEAVES = 25;
34-
3534
private final Node root = new Node(null, "");
3635

36+
private final int rootPackagesLimit;
37+
38+
public PackageTree(Config config) {
39+
rootPackagesLimit = config.getCiVisibilityCoverageRootPackagesLimit();
40+
}
41+
3742
void add(Path packagePath) {
3843
if (packagePath.toString().isEmpty()) {
3944
return;
@@ -44,14 +49,14 @@ void add(Path packagePath) {
4449
List<String> asList() {
4550
truncateIfNeeded(root);
4651

47-
List<String> childrenPackages = new ArrayList<>(MAX_CHILDREN_LEAVES);
52+
List<String> childrenPackages = new ArrayList<>(rootPackagesLimit);
4853
for (Node child : root.children.values()) {
4954
child.stringify(childrenPackages, "");
5055
}
5156
return childrenPackages;
5257
}
5358

54-
private static void truncateIfNeeded(Node root) {
59+
private void truncateIfNeeded(Node root) {
5560
Deque<List<Node>> nodesByDepth = new ArrayDeque<>();
5661

5762
List<Node> current = Collections.singletonList(root);
@@ -73,7 +78,7 @@ private static void truncateIfNeeded(Node root) {
7378
nodes.sort(Comparator.comparingInt(node -> -node.leafChildren));
7479

7580
for (Node node : nodes) {
76-
if (root.leafChildren <= MAX_CHILDREN_LEAVES) {
81+
if (root.leafChildren <= rootPackagesLimit) {
7782
// stop as soon as we have truncated enough, even if it's mid-level
7883
return;
7984
} else {

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexBuilder.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package datadog.trace.civisibility.source.index;
22

3+
import datadog.trace.api.Config;
34
import datadog.trace.util.ClassNameTrie;
45
import java.io.File;
56
import java.io.IOException;
@@ -20,6 +21,7 @@ public class RepoIndexBuilder implements RepoIndexProvider {
2021

2122
private static final Logger log = LoggerFactory.getLogger(RepoIndexBuilder.class);
2223

24+
private final Config config;
2325
private final String repoRoot;
2426
private final PackageResolver packageResolver;
2527
private final ResourceResolver resourceResolver;
@@ -29,10 +31,12 @@ public class RepoIndexBuilder implements RepoIndexProvider {
2931
private volatile RepoIndex index;
3032

3133
public RepoIndexBuilder(
34+
Config config,
3235
String repoRoot,
3336
PackageResolver packageResolver,
3437
ResourceResolver resourceResolver,
3538
FileSystem fileSystem) {
39+
this.config = config;
3640
this.repoRoot = repoRoot;
3741
this.packageResolver = packageResolver;
3842
this.resourceResolver = resourceResolver;
@@ -60,7 +64,7 @@ private RepoIndex doGetIndex() {
6064

6165
Path repoRootPath = fileSystem.getPath(repoRoot);
6266
RepoIndexingFileVisitor repoIndexingFileVisitor =
63-
new RepoIndexingFileVisitor(packageResolver, resourceResolver, repoRootPath);
67+
new RepoIndexingFileVisitor(config, packageResolver, resourceResolver, repoRootPath);
6468

6569
long startTime = System.currentTimeMillis();
6670
try {
@@ -100,13 +104,16 @@ private static final class RepoIndexingFileVisitor implements FileVisitor<Path>
100104
private final Path repoRoot;
101105

102106
private RepoIndexingFileVisitor(
103-
PackageResolver packageResolver, ResourceResolver resourceResolver, Path repoRoot) {
107+
Config config,
108+
PackageResolver packageResolver,
109+
ResourceResolver resourceResolver,
110+
Path repoRoot) {
104111
this.packageResolver = packageResolver;
105112
this.resourceResolver = resourceResolver;
106113
this.repoRoot = repoRoot;
107114
trieBuilder = new ClassNameTrie.Builder();
108115
sourceRoots = new LinkedHashSet<>();
109-
packageTree = new PackageTree();
116+
packageTree = new PackageTree(config);
110117
indexingStats = new RepoIndexingStats();
111118
}
112119

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexSourcePathResolver.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,14 @@ public RepoIndexSourcePathResolver(String repoRoot, RepoIndexProvider indexProvi
2020
}
2121

2222
RepoIndexSourcePathResolver(
23+
Config config,
2324
String repoRoot,
2425
PackageResolver packageResolver,
2526
ResourceResolver resourceResolver,
2627
FileSystem fileSystem) {
2728
this.repoRoot = repoRoot;
2829
this.indexProvider =
29-
new RepoIndexBuilder(repoRoot, packageResolver, resourceResolver, fileSystem);
30+
new RepoIndexBuilder(config, repoRoot, packageResolver, resourceResolver, fileSystem);
3031
}
3132

3233
@Nullable

‎dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/source/index/RepoIndexSourcePathResolverTest.groovy‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package datadog.trace.civisibility.source.index
22

33
import com.google.common.jimfs.Configuration
44
import com.google.common.jimfs.Jimfs
5+
import datadog.trace.api.Config
56
import groovy.transform.PackageScope
67
import spock.lang.Specification
78

@@ -10,6 +11,7 @@ import java.nio.file.Path
1011

1112
class RepoIndexSourcePathResolverTest extends Specification {
1213

14+
def config = Stub(Config)
1315
def packageResolver = Stub(PackageResolver)
1416
def resourceResolver = Stub(ResourceResolver)
1517
def fileSystem = Jimfs.newFileSystem(Configuration.unix())
@@ -20,7 +22,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
2022
def expectedSourcePath = givenSourceFile(RepoIndexSourcePathResolverTest, repoRoot + "/src")
2123

2224
when:
23-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
25+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
2426

2527
then:
2628
sourcePathResolver.getSourcePath(RepoIndexSourcePathResolverTest) == expectedSourcePath
@@ -31,7 +33,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
3133
def expectedSourcePath = givenSourceFile(RepoIndexSourcePathResolverTest, repoRoot + "/src")
3234

3335
when:
34-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
36+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
3537

3638
then:
3739
sourcePathResolver.getSourcePath(InnerClass) == expectedSourcePath
@@ -42,7 +44,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
4244
def expectedSourcePath = givenSourceFile(RepoIndexSourcePathResolverTest, repoRoot + "/src")
4345

4446
when:
45-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
47+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
4648

4749
then:
4850
sourcePathResolver.getSourcePath(InnerClass.NestedInnerClass) == expectedSourcePath
@@ -53,7 +55,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
5355
def expectedSourcePath = givenSourceFile(RepoIndexSourcePathResolverTest, repoRoot + "/src")
5456

5557
when:
56-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
58+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
5759
def r = new Runnable() {
5860
void run() {}
5961
}
@@ -67,7 +69,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
6769
def expectedSourcePath = givenSourceFile(RepoIndexSourcePathResolverTest, repoRoot + "/src")
6870

6971
when:
70-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
72+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
7173

7274
then:
7375
sourcePathResolver.getSourcePath(PackagePrivateClass) == expectedSourcePath
@@ -78,7 +80,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
7880
def expectedSourcePath = givenSourceFile(RepoIndexSourcePathResolverTest, repoRoot + "/src")
7981

8082
when:
81-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
83+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
8284

8385
then:
8486
sourcePathResolver.getSourcePath(PackagePrivateClass.NestedIntoPackagePrivateClass) == expectedSourcePath
@@ -89,7 +91,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
8991
def expectedSourcePath = givenSourceFile(RepoIndexSourcePathResolverTest, repoRoot + "/src")
9092

9193
when:
92-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
94+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
9395

9496
then:
9597
sourcePathResolver.getSourcePath(PublicClassWhoseNameDoesNotCorrespondToFileName) == expectedSourcePath
@@ -99,7 +101,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
99101
setup:
100102

101103
when:
102-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
104+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
103105

104106
then:
105107
sourcePathResolver.getSourcePath(RepoIndexSourcePathResolver) == null
@@ -109,7 +111,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
109111
setup:
110112

111113
when:
112-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
114+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
113115

114116
then:
115117
sourcePathResolver.getSourcePath(PackagePrivateClass) == null
@@ -124,7 +126,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
124126
Files.write(classPath, "STUB CLASS BODY".getBytes())
125127

126128
when:
127-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
129+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
128130

129131
then:
130132
sourcePathResolver.getSourcePath(RepoIndexSourcePathResolverTest) == null
@@ -138,7 +140,7 @@ class RepoIndexSourcePathResolverTest extends Specification {
138140
givenRepoFile(fileSystem.getPath(repoRoot, "README.md"))
139141

140142
when:
141-
def sourcePathResolver = new RepoIndexSourcePathResolver(repoRoot, packageResolver, resourceResolver, fileSystem)
143+
def sourcePathResolver = new RepoIndexSourcePathResolver(config, repoRoot, packageResolver, resourceResolver, fileSystem)
142144

143145
then:
144146
sourcePathResolver.getSourcePath(RepoIndexSourcePathResolverTest) == expectedSourcePathOne

‎dd-trace-api/src/main/java/datadog/trace/api/config/CiVisibilityConfig.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ public final class CiVisibilityConfig {
6262
public static final String CIVISIBILITY_JVM_INFO_CACHE_SIZE = "civisibility.jvm.info.cache.size";
6363
public static final String CIVISIBILITY_COVERAGE_SEGMENTS_ENABLED =
6464
"civisibility.coverage.segments.enabled";
65+
public static final String CIVISIBILITY_COVERAGE_ROOT_PACKAGES_LIMIT =
66+
"civisibility.coverage.root.packages.limit";
6567
public static final String CIVISIBILITY_INJECTED_TRACER_VERSION =
6668
"civisibility.injected.tracer.version";
6769
public static final String CIVISIBILITY_RESOURCE_FOLDER_NAMES =

‎internal-api/src/main/java/datadog/trace/api/Config.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@
153153
import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_CODE_COVERAGE_REPORT_DUMP_DIR;
154154
import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_COMPILER_PLUGIN_AUTO_CONFIGURATION_ENABLED;
155155
import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_COMPILER_PLUGIN_VERSION;
156+
import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_COVERAGE_ROOT_PACKAGES_LIMIT;
156157
import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_COVERAGE_SEGMENTS_ENABLED;
157158
import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_DEBUG_PORT;
158159
import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_GIT_COMMAND_TIMEOUT_MILLIS;
@@ -736,6 +737,7 @@ static class HostNameHolder {
736737
private final int ciVisibilityModuleExecutionSettingsCacheSize;
737738
private final int ciVisibilityJvmInfoCacheSize;
738739
private final boolean ciVisibilityCoverageSegmentsEnabled;
740+
private final int ciVisibilityCoverageRootPackagesLimit;
739741
private final String ciVisibilityInjectedTracerVersion;
740742
private final List<String> ciVisibilityResourceFolderNames;
741743

@@ -1690,6 +1692,8 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins
16901692
ciVisibilityJvmInfoCacheSize = configProvider.getInteger(CIVISIBILITY_JVM_INFO_CACHE_SIZE, 8);
16911693
ciVisibilityCoverageSegmentsEnabled =
16921694
configProvider.getBoolean(CIVISIBILITY_COVERAGE_SEGMENTS_ENABLED, false);
1695+
ciVisibilityCoverageRootPackagesLimit =
1696+
configProvider.getInteger(CIVISIBILITY_COVERAGE_ROOT_PACKAGES_LIMIT, 30);
16931697
ciVisibilityInjectedTracerVersion =
16941698
configProvider.getString(CIVISIBILITY_INJECTED_TRACER_VERSION);
16951699
ciVisibilityResourceFolderNames =
@@ -2834,6 +2838,10 @@ public boolean isCiVisibilityCoverageSegmentsEnabled() {
28342838
return ciVisibilityCoverageSegmentsEnabled;
28352839
}
28362840

2841+
public int getCiVisibilityCoverageRootPackagesLimit() {
2842+
return ciVisibilityCoverageRootPackagesLimit;
2843+
}
2844+
28372845
public String getCiVisibilityInjectedTracerVersion() {
28382846
return ciVisibilityInjectedTracerVersion;
28392847
}

0 commit comments

Comments
 (0)