From 2bbf77c1fe6b323a68ba4916627ea86169bb0d48 Mon Sep 17 00:00:00 2001 From: Nikita Tkachenko Date: Fri, 5 May 2023 18:16:55 +0200 Subject: [PATCH] Make embedded git.properties have lower priority than other git metadata sources --- .../civisibility/git/CILocalGitInfoBuilder.java | 5 +++++ .../civisibility/git/CIProviderGitInfoBuilder.java | 5 +++++ .../trace/api/git/EmbeddedGitInfoBuilder.java | 5 +++++ .../java/datadog/trace/api/git/GitInfoBuilder.java | 2 ++ .../java/datadog/trace/api/git/GitInfoProvider.java | 13 +++++++++---- .../trace/api/git/UserSuppliedGitInfoBuilder.java | 5 +++++ .../trace/api/git/GitInfoProviderTest.groovy | 12 +++++++++--- 7 files changed, 40 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CILocalGitInfoBuilder.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CILocalGitInfoBuilder.java index dd1a5927fd6..36e748cc302 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CILocalGitInfoBuilder.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CILocalGitInfoBuilder.java @@ -21,4 +21,9 @@ public GitInfo build(@Nullable String repositoryPath) { return new LocalFSGitInfoExtractor() .headCommit(Paths.get(repositoryPath, gitFolderName).toFile().getAbsolutePath()); } + + @Override + public int order() { + return 2; + } } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CIProviderGitInfoBuilder.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CIProviderGitInfoBuilder.java index 695e399b664..cb55f4c1e5a 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CIProviderGitInfoBuilder.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/CIProviderGitInfoBuilder.java @@ -16,4 +16,9 @@ public GitInfo build(@Nullable String repositoryPath) { CIProviderInfo ciProviderInfo = ciProviderInfoFactory.createCIProviderInfo(currentPath); return ciProviderInfo.buildCIGitInfo(); } + + @Override + public int order() { + return 1; + } } diff --git a/internal-api/src/main/java/datadog/trace/api/git/EmbeddedGitInfoBuilder.java b/internal-api/src/main/java/datadog/trace/api/git/EmbeddedGitInfoBuilder.java index b40a399ab30..b3cbb3180d9 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/EmbeddedGitInfoBuilder.java +++ b/internal-api/src/main/java/datadog/trace/api/git/EmbeddedGitInfoBuilder.java @@ -67,4 +67,9 @@ public GitInfo build(@Nullable String repositoryPath) { committerTime), gitProperties.getProperty("git.commit.message.full"))); } + + @Override + public int order() { + return Integer.MAX_VALUE; + } } diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitInfoBuilder.java b/internal-api/src/main/java/datadog/trace/api/git/GitInfoBuilder.java index e92a9f5514e..a498407532b 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitInfoBuilder.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitInfoBuilder.java @@ -4,4 +4,6 @@ public interface GitInfoBuilder { GitInfo build(@Nullable String repositoryPath); + + int order(); } diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java index efb86e327f6..58f33e90fa8 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java @@ -4,9 +4,11 @@ import datadog.trace.api.cache.DDCaches; import datadog.trace.util.Strings; import java.nio.file.Paths; +import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; import java.util.List; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.Function; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -21,7 +23,7 @@ public class GitInfoProvider { INSTANCE.registerGitInfoBuilder(new EmbeddedGitInfoBuilder()); } - private final Collection builders = new CopyOnWriteArrayList<>(); + private volatile Collection builders = Collections.emptyList(); // in regular cases git info has to be built only once, // but there is a rare exception: @@ -100,8 +102,11 @@ private static String firstNonBlankWithMatchingCommit( return null; } - public void registerGitInfoBuilder(GitInfoBuilder builder) { - builders.add(builder); + public synchronized void registerGitInfoBuilder(GitInfoBuilder builder) { + List updatedBuilders = new ArrayList<>(builders); + updatedBuilders.add(builder); + updatedBuilders.sort(Comparator.comparingInt(GitInfoBuilder::order)); + builders = updatedBuilders; gitInfoCache.clear(); } } diff --git a/internal-api/src/main/java/datadog/trace/api/git/UserSuppliedGitInfoBuilder.java b/internal-api/src/main/java/datadog/trace/api/git/UserSuppliedGitInfoBuilder.java index faf9be9f86f..6cb4a6b293e 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/UserSuppliedGitInfoBuilder.java +++ b/internal-api/src/main/java/datadog/trace/api/git/UserSuppliedGitInfoBuilder.java @@ -98,4 +98,9 @@ public GitInfo build(@Nullable String repositoryPath) { return gitInfo; } + + @Override + public int order() { + return 0; + } } diff --git a/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy index 1dc46f7182a..78f32d943ef 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy @@ -32,12 +32,13 @@ class GitInfoProviderTest extends Specification { ) def gitInfoBuilderB = givenABuilderReturning( - new GitInfo(null, "branch", "tag", new CommitInfo("sha")) + new GitInfo(null, "branch", "tag", new CommitInfo("sha")), 2 ) def gitInfoProvider = new GitInfoProvider() - gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) + // registering provider with higher order first, to check that the registration logic will do proper reordering after the second registration gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderB) + gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) when: def actualGitInfo = gitInfoProvider.getGitInfo(REPO_PATH) @@ -238,8 +239,13 @@ class GitInfoProviderTest extends Specification { } private GitInfoBuilder givenABuilderReturning(GitInfo gitInfo) { + givenABuilderReturning(gitInfo, 1) + } + + private GitInfoBuilder givenABuilderReturning(GitInfo gitInfo, int order) { def gitInfoBuilder = Stub(GitInfoBuilder) gitInfoBuilder.build(REPO_PATH) >> gitInfo - return gitInfoBuilder + gitInfoBuilder.order() >> order + gitInfoBuilder } }