Skip to content

Commit 5181a21

Browse files
bric3jpbempeldevflow.devflow-routing-intake
authored
Make Config / InstrumenterConfig modifiable via a load-time test agent (#11397)
test: replace WithConfigExtension's retransform agent with a premain agent The JUnit 5 WithConfigExtension used ByteBuddy retransformation at @BeforeAll to make Config and InstrumenterConfig INSTANCE fields public/volatile/non-final. Retransformation is not guaranteed when the class has been loaded before the extension runs (e.g. through CoreTracer), which caused intermittent failures on at least HotSpot 1.8, IBM J9, and OpenJ9 Semeru jobs. This commit switches to a load-time javaagent that rewrites the INSTANCE fields upon class cload. It introduces in particular `dd-trace-java.modifiable-config` convention plugin whose role is to configure the test jvm with the java agent to every Test task. This convention plugin is applied automatically via dd-trace-java.configure-tests which is itself applied via java_no_deps.gradle. `WithConfigExtension` no longer installs a ByteBuddy agent programmatically; instead it just verifies the fields are already modifiable. refactor: simplify WithConfigExtension now that load-time agent guarantees writable Config fields With modifiable-config-agent attached to every test JVM, the `INSTANCE` fields of `Config` and `InstrumenterConfig` are always `public`/`volatile`/non-`final` by the time `WithConfigExtension` is loaded. The runtime state machine that previously guarded the ByteBuddy retransform is therefore dead code. Collapse it: - Move reflective lookups of the INSTANCE fields and default constructors into a static {} block that first calls ensureConfigInstrumentationHasBeenApplied() and then caches the handles as static finals. Failure throws ExceptionInInitializerError, which surfaces as a clear test-class load failure pointing at the Gradle convention plugin. - Remove the isConfigInstanceModifiable / configModificationFailed / configTransformerInstalled flags and every guard they fed. - Drop makeConfigInstanceModifiable(), checkConfigTransformation(), and checkWritable() — all redundant with the static-init check. - Drop the corresponding JUnit assert* static imports. - beforeAll/beforeEach/afterEach/afterAll now call rebuildConfig() unconditionally. - Update the class Javadoc to reflect the new mechanism. Co-Authored-By: Claude <noreply@anthropic.com> ci: trigger non-default JVM tests [ci: NON_DEFAULT_JVMS] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> revert: Mark PendingTraceBufferTest tests flaky (#11383) chore: make spotless happy [ci: NON_DEFAULT_JVMS] fix(build): skip modifiable-config agent when Test JVM already has -javaagent The `dd-trace-java.modifiable-config` convention plugin unconditionally attached a second `-javaagent:modifiable-config-agent.jar` to every Test JVM. That breaks the few modules whose tests already launch with the real dd-java-agent attached (currently only `:dd-java-agent`'s integration tests, via the `doFirst` at `dd-java-agent/build.gradle:454-457`): * `AgentBootstrap.installAgentJar` tries the class' CodeSource first, then `getAgentFileFromJavaagentArg`, then a ClassLoader-resource lookup. On the failing matrix the CodeSource is null for the bootstrap class, so it falls through to the `-javaagent:` arg. * `getAgentFileFromJavaagentArg` refuses to pick when more than one `-javaagent:` is present ("multiple javaagents specified") and returns null. * The ClassLoader fallback then throws `IllegalArgumentException: URI is not absolute` for the bootstrap class URL. Net effect: the tracing agent never installs and suites like `OpenTracingTest` / `ShadowPackageRenamingTest` fail across non-default JVMs (ibm8, semeru11/17, zulu11, tip). Alternatives considered and rejected: * Manifest scan in `getAgentFileFromJavaagentArg` to pick the entry whose `Main-Class` is `AgentBootstrap`: meaningful bootstrap- performance cost (open every `-javaagent:` jar, read MANIFEST.MF) on a hot path that runs on every JVM startup. * `-Ddd.agent.jar.path` / `DD_AGENT_JAR_PATH`: no equivalent knob exists today, adds an attacker-controllable file path, and only helps when the CLI is already ambiguous — not worth the surface. Skipping is safe for `:dd-java-agent`'s tests: nothing under `dd-java-agent/src/test` (Java or Groovy) references `Config` / `InstrumenterConfig` `INSTANCE`, and nothing uses `WithConfigExtension`. The tests rely on the real agent's startup to populate `Config.INSTANCE`, not on swapping the singleton mid-test. Implementation: defer the attach to a `doFirst` that inspects `allJvmArgs` for an existing non-modifiable-config `-javaagent:` entry. Gradle prepends each new `doFirst`, so the plugin's check runs after the project's own agent-attach `doFirst` and sees the real arg before the JVM forks. When skipping, emit an `INFO`-level log so the decision is visible under `--info`. [ci: NON_DEFAULT_JVMS] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Increase timeout to pass for ibm8 filling the delaying buffer for test bufferFullYieldsImmediateWrite takes more than 5 seconds on ibm8 and CI [ci: NON_DEFAULT_JVMS] Merge branch 'master' into bdu/moves-with-config-extension-agent-to-commandline-via-convention-plugin Co-authored-by: jpbempel <jean-philippe.bempel@datadoghq.com> Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent c2e8fe0 commit 5181a21

9 files changed

Lines changed: 211 additions & 144 deletions

File tree

buildSrc/build.gradle.kts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ dependencies {
101101

102102
tasks.compileKotlin {
103103
dependsOn(":call-site-instrumentation-plugin:build")
104+
dependsOn(":modifiable-config-agent:build")
104105
}
105106

106107
testing {
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
plugins {
2+
java
3+
id("com.gradleup.shadow") version "8.3.9"
4+
}
5+
6+
java {
7+
sourceCompatibility = JavaVersion.VERSION_1_8
8+
targetCompatibility = JavaVersion.VERSION_1_8
9+
}
10+
11+
apply {
12+
from("$rootDir/../gradle/repositories.gradle")
13+
}
14+
15+
dependencies {
16+
implementation(libs.asm)
17+
}
18+
19+
tasks {
20+
shadowJar {
21+
archiveClassifier.set("")
22+
relocate("org.objectweb.asm", "datadog.trace.agent.test.config.shaded.asm")
23+
manifest {
24+
attributes(
25+
mapOf(
26+
"Premain-Class" to "datadog.trace.agent.test.config.ModifiableConfigAgent",
27+
"Can-Retransform-Classes" to "false",
28+
"Can-Redefine-Classes" to "false",
29+
),
30+
)
31+
}
32+
}
33+
34+
jar {
35+
enabled = false
36+
}
37+
38+
build {
39+
dependsOn(shadowJar)
40+
}
41+
}
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
package datadog.trace.agent.test.config;
2+
3+
import java.lang.instrument.ClassFileTransformer;
4+
import java.lang.instrument.IllegalClassFormatException;
5+
import java.lang.instrument.Instrumentation;
6+
import java.security.ProtectionDomain;
7+
import org.objectweb.asm.ClassReader;
8+
import org.objectweb.asm.ClassVisitor;
9+
import org.objectweb.asm.ClassWriter;
10+
import org.objectweb.asm.FieldVisitor;
11+
import org.objectweb.asm.Opcodes;
12+
13+
/**
14+
* Test-only Java agent that rewrites the {@code INSTANCE} field of {@code
15+
* datadog.trace.api.Config} and {@code datadog.trace.api.InstrumenterConfig} to be public,
16+
* volatile, and non-final, so tests can swap the singleton with a freshly-built instance.
17+
*
18+
* <p>Unlike a JUnit 5 extension that uses ByteBuddy to retransform the classes, this agent runs
19+
* before any class is loaded, so the rewrite is guaranteed regardless of which class touches the
20+
* config first.
21+
*/
22+
public final class ModifiableConfigAgent {
23+
24+
private static final String CONFIG = "datadog/trace/api/Config";
25+
private static final String INST_CONFIG = "datadog/trace/api/InstrumenterConfig";
26+
private static final String INSTANCE = "INSTANCE";
27+
28+
private ModifiableConfigAgent() {}
29+
30+
public static void premain(String args, Instrumentation inst) {
31+
inst.addTransformer(new InstanceFieldRewriter(), false);
32+
}
33+
34+
static final class InstanceFieldRewriter implements ClassFileTransformer {
35+
@Override
36+
public byte[] transform(
37+
ClassLoader loader,
38+
String className,
39+
Class<?> classBeingRedefined,
40+
ProtectionDomain protectionDomain,
41+
byte[] classfileBuffer)
42+
throws IllegalClassFormatException {
43+
if (className == null) {
44+
return null;
45+
}
46+
if (!CONFIG.equals(className) && !INST_CONFIG.equals(className)) {
47+
return null;
48+
}
49+
try {
50+
ClassReader reader = new ClassReader(classfileBuffer);
51+
ClassWriter writer = new ClassWriter(reader, 0);
52+
reader.accept(new InstanceFieldClassVisitor(writer), 0);
53+
return writer.toByteArray();
54+
} catch (Throwable t) {
55+
System.err.println(
56+
"[modifiable-config-agent] failed to rewrite " + className + ": " + t);
57+
return null;
58+
}
59+
}
60+
}
61+
62+
static final class InstanceFieldClassVisitor extends ClassVisitor {
63+
InstanceFieldClassVisitor(ClassVisitor cv) {
64+
super(Opcodes.ASM9, cv);
65+
}
66+
67+
@Override
68+
public FieldVisitor visitField(
69+
int access, String name, String descriptor, String signature, Object value) {
70+
if (INSTANCE.equals(name)) {
71+
int rewritten =
72+
(access & ~(Opcodes.ACC_PRIVATE | Opcodes.ACC_PROTECTED | Opcodes.ACC_FINAL))
73+
| Opcodes.ACC_PUBLIC
74+
| Opcodes.ACC_VOLATILE;
75+
return super.visitField(rewritten, name, descriptor, signature, value);
76+
}
77+
return super.visitField(access, name, descriptor, signature, value);
78+
}
79+
}
80+
}

buildSrc/settings.gradle.kts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
include(":call-site-instrumentation-plugin")
2+
include(":modifiable-config-agent")
23

34
dependencyResolutionManagement {
45
versionCatalogs {

buildSrc/src/main/kotlin/dd-trace-java.configure-tests.gradle.kts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ import org.gradle.testing.base.TestingExtension
1010
import java.time.Duration
1111
import java.time.temporal.ChronoUnit
1212

13+
plugins {
14+
id("dd-trace-java.modifiable-config")
15+
}
16+
1317
// Need concrete implementation of BuildService in Kotlin
1418
abstract class ForkedTestLimit : BuildService<BuildServiceParameters.None>
1519
// Forked tests will fail with OOM if the memory is set too high. Gitlab allows at least a limit of 3.
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import org.gradle.api.tasks.testing.Test
2+
import org.gradle.kotlin.dsl.withType
3+
4+
/**
5+
* Attaches the modifiable-config Java agent to every Test task so that
6+
* `datadog.trace.api.Config` and `datadog.trace.api.InstrumenterConfig` have
7+
* their `INSTANCE` field rewritten as public/volatile/non-final at load time.
8+
*
9+
* This is the load-time counterpart to
10+
* `datadog.trace.junit.utils.config.WithConfigExtension`, which uses ByteBuddy
11+
* to retransform the same classes at runtime. Retransformation does not always
12+
* succeed when the class has been touched before the JUnit lifecycle starts
13+
* (e.g. through `CoreTracer`); the agent guarantees the rewrite by intercepting
14+
* the class before it is defined.
15+
*
16+
* The agent jar is built by the `:modifiable-config-agent` buildSrc subproject
17+
* and is produced eagerly because the parent `buildSrc` compilation depends on
18+
* it.
19+
*/
20+
val agentJar = rootProject.layout.projectDirectory
21+
.file("buildSrc/modifiable-config-agent/build/libs/modifiable-config-agent.jar")
22+
.asFile
23+
24+
tasks.withType<Test>().configureEach {
25+
inputs.file(agentJar).withPathSensitivity(org.gradle.api.tasks.PathSensitivity.NONE)
26+
// Attach lazily so we can skip when the Test JVM already has another -javaagent.
27+
// doFirst prepends, so this runs after any -javaagent registered later via doFirst.
28+
doFirst {
29+
val foreignAgent = allJvmArgs.firstOrNull {
30+
it.startsWith("-javaagent:") && !it.contains("modifiable-config-agent")
31+
}
32+
if (foreignAgent != null) {
33+
logger.info(
34+
"[modifiable-config] skipping attach for {} — another -javaagent already present: {}",
35+
path,
36+
foreignAgent,
37+
)
38+
} else {
39+
jvmArgs("-javaagent:${agentJar.absolutePath}")
40+
}
41+
}
42+
}

dd-trace-core/src/test/java/datadog/trace/core/PendingTraceBufferTest.java

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import datadog.trace.core.propagation.PropagationTags;
3636
import datadog.trace.core.scopemanager.ContinuableScopeManager;
3737
import datadog.trace.test.util.DDJavaSpecification;
38-
import datadog.trace.test.util.Flaky;
3938
import java.io.ByteArrayInputStream;
4039
import java.io.ByteArrayOutputStream;
4140
import java.io.IOException;
@@ -57,7 +56,7 @@
5756
import org.junit.jupiter.api.Test;
5857
import org.junit.jupiter.api.Timeout;
5958

60-
@Timeout(5)
59+
@Timeout(10)
6160
public class PendingTraceBufferTest extends DDJavaSpecification {
6261

6362
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
@@ -196,7 +195,6 @@ void prioritySamplingIsAlwaysSent() {
196195
assertTrue(metadataChecker.hasSamplingPriority);
197196
}
198197

199-
@Flaky("Flaky on ibm8, semeru8, and semeru17 JVMs after migration from Groovy to JUnit")
200198
@Test
201199
void bufferFullYieldsImmediateWrite() {
202200
int capacity = delayingBuffer.getQueue().capacity();
@@ -221,7 +219,6 @@ void bufferFullYieldsImmediateWrite() {
221219
assertEquals(0, pendingTrace.getIsEnqueued());
222220
}
223221

224-
@Flaky("Flaky on ibm8, semeru8, and semeru17 JVMs after migration from Groovy to JUnit")
225222
@Test
226223
void longRunningTraceBufferFullDoesNotTriggerWrite() {
227224
int capacity = delayingBuffer.getQueue().capacity();
@@ -381,7 +378,6 @@ public boolean writeOnBufferFull() {
381378
assertEquals(3, counter.get());
382379
}
383380

384-
@Flaky("Flaky on semeru17 JVM after migration from Groovy to JUnit")
385381
@Test
386382
void samePendingTraceIsNotEnqueuedMultipleTimes() {
387383
when(tracer.getPartialFlushMinSpans()).thenReturn(10000);

utils/junit-utils/build.gradle.kts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@ plugins {
55
apply(from = "$rootDir/gradle/java.gradle")
66

77
dependencies {
8-
api(libs.bytebuddy)
9-
api(libs.bytebuddyagent)
108
api(libs.forbiddenapis)
119
api(project(":components:environment"))
1210

0 commit comments

Comments
 (0)