Skip to content

Commit da6a1b6

Browse files
nikita-tkachenko-datadognikita-tkachenko-datadog
authored
Fix Gradle Launcher instrumentation to not interfere with Gradle Test Kit (#8465)
1 parent 6a2bd8d commit da6a1b6

File tree

10 files changed

+686
-30
lines changed

10 files changed

+686
-30
lines changed

‎dd-java-agent/agent-ci-visibility/src/testFixtures/groovy/datadog/trace/civisibility/CiVisibilityTestUtils.groovy‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,12 @@ abstract class CiVisibilityTestUtils {
103103
try {
104104
JSONAssert.assertEquals(expectedEvents, actualEvents, comparisonMode)
105105
} catch (AssertionError e) {
106+
if (ciRun) {
107+
// When running in CI the assertion error message does not contain the actual diff,
108+
// so we print the events to the console to help debug the issue
109+
println "Expected events: $expectedEvents"
110+
println "Actual events: $actualEvents"
111+
}
106112
throw new org.opentest4j.AssertionFailedError("Events mismatch", expectedEvents, actualEvents, e)
107113
}
108114

@@ -111,6 +117,12 @@ abstract class CiVisibilityTestUtils {
111117
try {
112118
JSONAssert.assertEquals(expectedCoverages, actualCoverages, comparisonMode)
113119
} catch (AssertionError e) {
120+
if (ciRun) {
121+
// When running in CI the assertion error message does not contain the actual diff,
122+
// so we print the events to the console to help debug the issue
123+
println "Expected coverages: $expectedCoverages"
124+
println "Actual coverages: $actualCoverages"
125+
}
114126
throw new org.opentest4j.AssertionFailedError("Coverages mismatch", expectedCoverages, actualCoverages, e)
115127
}
116128

‎dd-java-agent/instrumentation/gradle-8.3/src/main/groovy/datadog/trace/instrumentation/gradle/GradleDaemonInjectionUtils.java‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package datadog.trace.instrumentation.gradle;
22

3+
import static datadog.trace.util.Strings.propertyNameToSystemPropertyName;
4+
35
import datadog.trace.api.Config;
6+
import datadog.trace.api.config.CiVisibilityConfig;
47
import java.io.File;
58
import java.nio.file.Path;
69
import java.util.HashMap;
@@ -11,17 +14,30 @@ public class GradleDaemonInjectionUtils {
1114

1215
public static Map<String, String> addJavaagentToGradleDaemonProperties(
1316
Map<String, String> jvmOptions) {
17+
Properties systemProperties = System.getProperties();
18+
if (systemProperties.containsKey(
19+
propertyNameToSystemPropertyName(
20+
CiVisibilityConfig.CIVISIBILITY_INJECTED_TRACER_VERSION))) {
21+
// This Gradle launcher is started by a process that is itself instrumented,
22+
// most likely this is a Gradle build using Gradle Test Kit to fork another Gradle instance
23+
// (e.g. to test a Gradle plugin).
24+
// We don't want to instrument/trace such "nested" Gradle instances
25+
return jvmOptions;
26+
}
27+
1428
File agentJar = Config.get().getCiVisibilityAgentJarFile();
1529
Path agentJarPath = agentJar.toPath();
16-
1730
StringBuilder agentArg = new StringBuilder("-javaagent:").append(agentJarPath).append('=');
1831

19-
Properties systemProperties = System.getProperties();
2032
for (Map.Entry<Object, Object> e : systemProperties.entrySet()) {
2133
String propertyName = (String) e.getKey();
2234
Object propertyValue = e.getValue();
2335
if (propertyName.startsWith(Config.PREFIX)) {
24-
agentArg.append(propertyName).append('=').append(propertyValue).append(',');
36+
agentArg
37+
.append(propertyName)
38+
.append("='")
39+
.append(String.valueOf(propertyValue).replace("'", "'\\''"))
40+
.append("',");
2541
}
2642
}
2743

‎dd-java-agent/instrumentation/gradle-8.3/src/main/groovy/datadog/trace/instrumentation/gradle/GradleServiceValidationInstrumentation.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
package datadog.trace.instrumentation.gradle;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
4+
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
5+
6+
import com.google.auto.service.AutoService;
7+
import datadog.trace.agent.tooling.Instrumenter;
8+
import datadog.trace.agent.tooling.InstrumenterModule;
9+
import datadog.trace.api.Config;
10+
import java.util.Set;
11+
import net.bytebuddy.asm.Advice;
12+
13+
/**
14+
* We inject {@link CiVisibilityGradleListener} into Gradle and register it with {@code
15+
* Scope.Build.class} scope.
16+
*
17+
* <p>A class named {@code org.gradle.internal.service.ServiceScopeValidator} checks that every
18+
* service registered with a scope is annotated with {@code @ServiceScope(<ScopeClass>.class)}.
19+
*
20+
* <p>We cannot annotate our service, as {@code Scope.Build.class} is a recent addition: we need to
21+
* support older Gradle versions, besides this class is absent in the Gradle API version that the
22+
* tracer currently uses.
23+
*
24+
* <p>To suppress validation for our service we patch a "workaround" class that is used internally
25+
* by Gradle.
26+
*/
27+
@AutoService(InstrumenterModule.class)
28+
public class GradleServiceValidationInstrumentation extends InstrumenterModule.CiVisibility
29+
implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice {
30+
31+
public GradleServiceValidationInstrumentation() {
32+
super("gradle", "gradle-build-scope-services");
33+
}
34+
35+
@Override
36+
public String instrumentedType() {
37+
return "org.gradle.internal.service.ServiceScopeValidatorWorkarounds";
38+
}
39+
40+
@Override
41+
public boolean isApplicable(Set<TargetSystem> enabledSystems) {
42+
return super.isApplicable(enabledSystems)
43+
&& Config.get().isCiVisibilityBuildInstrumentationEnabled();
44+
}
45+
46+
@Override
47+
public void methodAdvice(MethodTransformer transformer) {
48+
transformer.applyAdvice(
49+
named("shouldSuppressValidation").and(takesArgument(0, Class.class)),
50+
getClass().getName() + "$SuppressValidation");
51+
}
52+
53+
public static class SuppressValidation {
54+
@Advice.OnMethodExit(suppress = Throwable.class)
55+
public static void suppressValidationForCiVisService(
56+
@Advice.Argument(0) final Class<?> validatedClass,
57+
@Advice.Return(readOnly = false) boolean suppressValidation) {
58+
if (validatedClass.getName().endsWith("CiVisibilityGradleListener")) {
59+
suppressValidation = true;
60+
}
61+
}
62+
}
63+
}

‎dd-smoke-tests/gradle/src/test/groovy/datadog/smoketest/GradleDaemonSmokeTest.groovy‎

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,17 @@
11
package datadog.smoketest
22

3-
43
import datadog.trace.api.Config
54
import datadog.trace.api.Platform
65
import datadog.trace.api.config.CiVisibilityConfig
76
import datadog.trace.api.config.GeneralConfig
7+
import datadog.trace.api.config.TraceInstrumentationConfig
88
import datadog.trace.util.Strings
99
import org.gradle.testkit.runner.BuildResult
1010
import org.gradle.testkit.runner.GradleRunner
1111
import org.gradle.testkit.runner.TaskOutcome
1212
import org.gradle.util.DistributionLocator
1313
import org.gradle.util.GradleVersion
14-
import org.gradle.wrapper.Download
15-
import org.gradle.wrapper.Install
16-
import org.gradle.wrapper.PathAssembler
17-
import org.gradle.wrapper.WrapperConfiguration
14+
import org.gradle.wrapper.*
1815
import spock.lang.IgnoreIf
1916
import spock.lang.Shared
2017
import spock.lang.TempDir
@@ -49,34 +46,35 @@ class GradleDaemonSmokeTest extends AbstractGradleTest {
4946
runGradleTest(gradleVersion, projectName, false, successExpected, false, expectedTraces, expectedCoverages)
5047

5148
where:
52-
gradleVersion | projectName | successExpected | expectedTraces | expectedCoverages
53-
"3.0" | "test-succeed-old-gradle" | true | 5 | 1
54-
"7.6.4" | "test-succeed-legacy-instrumentation" | true | 5 | 1
55-
"7.6.4" | "test-succeed-multi-module-legacy-instrumentation" | true | 7 | 2
56-
"7.6.4" | "test-succeed-multi-forks-legacy-instrumentation" | true | 6 | 2
57-
"7.6.4" | "test-skip-legacy-instrumentation" | true | 2 | 0
58-
"7.6.4" | "test-failed-legacy-instrumentation" | false | 4 | 0
59-
"7.6.4" | "test-corrupted-config-legacy-instrumentation" | false | 1 | 0
49+
gradleVersion | projectName | successExpected | expectedTraces | expectedCoverages
50+
"3.0" | "test-succeed-old-gradle" | true | 5 | 1
51+
"7.6.4" | "test-succeed-legacy-instrumentation" | true | 5 | 1
52+
"7.6.4" | "test-succeed-multi-module-legacy-instrumentation" | true | 7 | 2
53+
"7.6.4" | "test-succeed-multi-forks-legacy-instrumentation" | true | 6 | 2
54+
"7.6.4" | "test-skip-legacy-instrumentation" | true | 2 | 0
55+
"7.6.4" | "test-failed-legacy-instrumentation" | false | 4 | 0
56+
"7.6.4" | "test-corrupted-config-legacy-instrumentation" | false | 1 | 0
6057
}
6158

6259
def "test #projectName, v#gradleVersion, configCache: #configurationCache"() {
6360
runGradleTest(gradleVersion, projectName, configurationCache, successExpected, flakyRetries, expectedTraces, expectedCoverages)
6461

6562
where:
66-
gradleVersion | projectName | configurationCache | successExpected | flakyRetries | expectedTraces | expectedCoverages
67-
"8.3" | "test-succeed-new-instrumentation" | false | true | false | 5 | 1
68-
"8.9" | "test-succeed-new-instrumentation" | false | true | false | 5 | 1
69-
LATEST_GRADLE_VERSION | "test-succeed-new-instrumentation" | false | true | false | 5 | 1
70-
"8.3" | "test-succeed-new-instrumentation" | true | true | false | 5 | 1
71-
"8.9" | "test-succeed-new-instrumentation" | true | true | false | 5 | 1
72-
LATEST_GRADLE_VERSION | "test-succeed-new-instrumentation" | true | true | false | 5 | 1
73-
LATEST_GRADLE_VERSION | "test-succeed-multi-module-new-instrumentation" | false | true | false | 7 | 2
74-
LATEST_GRADLE_VERSION | "test-succeed-multi-forks-new-instrumentation" | false | true | false | 6 | 2
75-
LATEST_GRADLE_VERSION | "test-skip-new-instrumentation" | false | true | false | 2 | 0
76-
LATEST_GRADLE_VERSION | "test-failed-new-instrumentation" | false | false | false | 4 | 0
77-
LATEST_GRADLE_VERSION | "test-corrupted-config-new-instrumentation" | false | false | false | 1 | 0
78-
LATEST_GRADLE_VERSION | "test-succeed-junit-5" | false | true | false | 5 | 1
79-
LATEST_GRADLE_VERSION | "test-failed-flaky-retries" | false | false | true | 8 | 0
63+
gradleVersion | projectName | configurationCache | successExpected | flakyRetries | expectedTraces | expectedCoverages
64+
"8.3" | "test-succeed-new-instrumentation" | false | true | false | 5 | 1
65+
"8.9" | "test-succeed-new-instrumentation" | false | true | false | 5 | 1
66+
LATEST_GRADLE_VERSION | "test-succeed-new-instrumentation" | false | true | false | 5 | 1
67+
"8.3" | "test-succeed-new-instrumentation" | true | true | false | 5 | 1
68+
"8.9" | "test-succeed-new-instrumentation" | true | true | false | 5 | 1
69+
LATEST_GRADLE_VERSION | "test-succeed-new-instrumentation" | true | true | false | 5 | 1
70+
LATEST_GRADLE_VERSION | "test-succeed-multi-module-new-instrumentation" | false | true | false | 7 | 2
71+
LATEST_GRADLE_VERSION | "test-succeed-multi-forks-new-instrumentation" | false | true | false | 6 | 2
72+
LATEST_GRADLE_VERSION | "test-skip-new-instrumentation" | false | true | false | 2 | 0
73+
LATEST_GRADLE_VERSION | "test-failed-new-instrumentation" | false | false | false | 4 | 0
74+
LATEST_GRADLE_VERSION | "test-corrupted-config-new-instrumentation" | false | false | false | 1 | 0
75+
LATEST_GRADLE_VERSION | "test-succeed-junit-5" | false | true | false | 5 | 1
76+
LATEST_GRADLE_VERSION | "test-failed-flaky-retries" | false | false | true | 8 | 0
77+
LATEST_GRADLE_VERSION | "test-succeed-gradle-plugin-test" | false | true | false | 5 | 0
8078
}
8179

8280
private runGradleTest(String gradleVersion, String projectName, boolean configurationCache, boolean successExpected, boolean flakyRetries, int expectedTraces, int expectedCoverages) {
@@ -131,6 +129,16 @@ class GradleDaemonSmokeTest extends AbstractGradleTest {
131129
"${Strings.propertyNameToSystemPropertyName(CiVisibilityConfig.CIVISIBILITY_AGENTLESS_ENABLED)}=true," +
132130
"${Strings.propertyNameToSystemPropertyName(CiVisibilityConfig.CIVISIBILITY_GIT_UPLOAD_ENABLED)}=false," +
133131
"${Strings.propertyNameToSystemPropertyName(CiVisibilityConfig.CIVISIBILITY_CIPROVIDER_INTEGRATION_ENABLED)}=false," +
132+
/*
133+
* Some of the smoke tests (in particular the one with the Gradle plugin), are using Gradle Test Kit for their tests.
134+
* Gradle Test Kit needs to do a "chmod" when starting a Gradle Daemon.
135+
* This "chmod" operation is traced by datadog.trace.instrumentation.java.lang.ProcessImplInstrumentation and is reported as a span.
136+
* The problem is that the "chmod" only happens when running in CI (could be due to differences in OS or FS permissions),
137+
* so when running the tests locally, the "chmod" span is not there.
138+
* This causes the tests to fail because the number of reported traces is different.
139+
* To avoid this discrepancy between local and CI runs, we disable tracing instrumentations.
140+
*/
141+
"${Strings.propertyNameToSystemPropertyName(TraceInstrumentationConfig.TRACE_ENABLED)}=false," +
134142
"${Strings.propertyNameToSystemPropertyName(CiVisibilityConfig.CIVISIBILITY_JACOCO_PLUGIN_VERSION)}=$JACOCO_PLUGIN_VERSION," +
135143
"${Strings.propertyNameToSystemPropertyName(CiVisibilityConfig.CIVISIBILITY_AGENTLESS_URL)}=${mockBackend.intakeUrl}"
136144

‎dd-smoke-tests/gradle/src/test/resources/test-succeed-gradle-plugin-test/build.gradleTest‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
plugins {
2+
id 'java'
3+
id 'java-gradle-plugin'
4+
}
5+
6+
gradlePlugin {
7+
plugins {
8+
mySimplePlugin {
9+
id = 'datadog.smoke.helloplugin'
10+
implementationClass = 'datadog.smoke.HelloPlugin'
11+
}
12+
}
13+
}
14+
15+
repositories {
16+
mavenLocal()
17+
mavenCentral()
18+
}
19+
20+
dependencies {
21+
testImplementation gradleTestKit()
22+
23+
testImplementation 'org.junit.jupiter:junit-jupiter-engine:5.10.2'
24+
}
25+
26+
tasks.withType(Test).configureEach {
27+
useJUnitPlatform()
28+
}

‎dd-smoke-tests/gradle/src/test/resources/test-succeed-gradle-plugin-test/coverages.ftl‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
[ {
2+
"files" : [ {
3+
"bitmap" : "AAAAx98=",
4+
"filename" : "src/test/java/datadog/smoke/HelloPluginFunctionalTest.java"
5+
} ],
6+
"span_id" : ${content_span_id_4},
7+
"test_session_id" : ${content_test_session_id},
8+
"test_suite_id" : ${content_test_suite_id}
9+
} ]

0 commit comments

Comments
 (0)