Skip to content

Add testProcess{Config} task to accept user options #581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
zhengyu123 wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Add testProcess{Config} task to accept user options #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
42d4d54
Create testProcess{Config} task that can pass command line profiler.o…
zhengyu123 Jun 4, 2026
6f5c7ba
Cleanup
zhengyu123 Jun 5, 2026
a13c32a
Fix
zhengyu123 Jun 8, 2026
0a80e41
Merge branch 'main' into zgu/test_cmd_opts
zhengyu123 Jul 3, 2026
710ac93
Fix
zhengyu123 Jul 3, 2026
9b9d36e
Merge branch 'main' into zgu/test_cmd_opts
zhengyu123 Jul 9, 2026
dff76a3
Merge branch 'main' into zgu/test_cmd_opts
zhengyu123 Jul 9, 2026
9e9d6a0
Fix review comments
zhengyu123 Jul 9, 2026
1d5a380
Merge branch 'zgu/test_cmd_opts' of github.com:DataDog/java-profiler …
zhengyu123 Jul 9, 2026
15dbbf7
Merge branch 'main' into zgu/test_cmd_opts
zhengyu123 Jul 10, 2026
799203b
Merge branch 'main' into zgu/test_cmd_opts
zhengyu123 Jul 15, 2026
50547f5
Roman's comment
zhengyu123 Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ Never use 'gradle' or 'gradlew' directly. Instead, use the '/build-and-summarize
./gradlew testDebug
./gradlew testAsan
./gradlew testTsan
./gradlew -Pprofiler.options=${user options} testProcessRelease
./gradlew -Pprofiler.options=${user options} testProcessDebug

# Run C++ unit tests only (via GtestPlugin)
./gradlew :ddprof-lib:gtestDebug # All debug tests
Expand Down
132 changes: 121 additions & 11 deletions build-logic/conventions/src/main/kotlin/com/datadoghq/profiler/ProfilerTestPlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,20 +399,16 @@ class ProfilerTestPlugin : Plugin<Project> {
}

// Environment variables
testConfig.environmentVariables.forEach { (key, value) ->
execTask.environment(key, value)
}

// CRITICAL FIX: Remove LD_LIBRARY_PATH to let RPATH work correctly
// CRITICAL FIX: Filter out LD_LIBRARY_PATH to let RPATH work correctly
// The test JDK's launcher has RPATH set to find its own libraries ($ORIGIN/../lib/jli)
// But LD_LIBRARY_PATH overrides RPATH and causes it to load the wrong libjli.so
// Solution: Unset LD_LIBRARY_PATH entirely to let RPATH take precedence
execTask.doFirst {
val currentLdLibPath = (execTask.environment["LD_LIBRARY_PATH"] as? String) ?: System.getenv("LD_LIBRARY_PATH")
if (!currentLdLibPath.isNullOrEmpty()) {
project.logger.info("Removing LD_LIBRARY_PATH to prevent cross-JDK library conflicts (was: $currentLdLibPath)")
execTask.environment.remove("LD_LIBRARY_PATH")
// Solution: Never propagate LD_LIBRARY_PATH so RPATH takes precedence
testConfig.environmentVariables.forEach { (key, value) ->
if (key == "LD_LIBRARY_PATH") {
project.logger.info("Removing LD_LIBRARY_PATH to prevent cross-JDK library conflicts (was: $value)")
return@forEach
}
Comment on lines +406 to 410

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove inherited LD_LIBRARY_PATH from Exec tests

When the normal musl Exec test path runs with LD_LIBRARY_PATH already set in the CI shell, this loop only filters values coming from testConfig.environmentVariables; Gradle Exec tasks inherit the current process environment by default, so the ambient LD_LIBRARY_PATH remains in execTask.environment. The previous doFirst removed the inherited value as well, and without that removal $JAVA_TEST_HOME/bin/java can again load the wrong libjli.so, which is the cross-JDK failure this block is meant to prevent.

Useful? React with 👍 / 👎.

@rkennke rkennke Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that has been introduced by the change that I have requested. I think the previous version was actually more-correct. Sorry for that noise.

execTask.environment(key, value)
}

// Sanitizer conditions
Expand All @@ -429,6 +425,116 @@ class ProfilerTestPlugin : Plugin<Project> {
}
}

/**
* Create Exec-based test task that always runs in a separate process.
* Available on all platforms. Supports -Pprofiler.options for overriding profiler settings.
* Task name: testProcess<Config> (e.g., testProcessDebug, testProcessRelease)
*/
private fun createProcessTestTask(
project: Project,
extension: ProfilerTestExtension,
testConfig: TestTaskConfiguration,
testCfg: Configuration,
sourceSets: SourceSetContainer
) {
val taskName = "testProcess${testConfig.configName.replaceFirstChar { it.uppercase() }}"
project.tasks.register(taskName, Exec::class.java) {
val execTask = this
execTask.description = "Runs tests in separate process with ${testConfig.configName} library (supports -Pprofiler.options)"
execTask.group = "verification"
execTask.onlyIf { testConfig.isActive && !project.hasProperty("skip-tests") }

// Dependencies
execTask.dependsOn(project.tasks.named("compileTestJava"))
execTask.dependsOn(testCfg)
execTask.dependsOn(sourceSets.getByName("test").output)

// Configure at execution time to capture properties
execTask.doFirst {
execTask.executable = PlatformUtils.testJavaExecutable()

val allArgs = mutableListOf<String>()

// JVM args
allArgs.addAll(testConfig.standardJvmArgs)
// Version-gated at execution time, when the real test JVM (JAVA_TEST_HOME) is resolvable.
allArgs.addAll(carrierExportJvmArgs(project))
if (extension.nativeLibDir.isPresent) {
allArgs.add("-Djava.library.path=${extension.nativeLibDir.get().asFile.absolutePath}")
}
allArgs.addAll(testConfig.extraJvmArgs)
Comment thread
zhengyu123 marked this conversation as resolved.
allArgs.addAll(testConfig.configJvmArgs)

// System properties
testConfig.systemProperties.forEach { (key, value) ->
allArgs.add("-D$key=$value")
}

// Test filter from -Ptests property
val testsFilter = project.findProperty("tests") as String?
if (testsFilter != null) {
allArgs.add("-Dtest.filter=$testsFilter")
}

// Profiler options from -Pprofiler.options property
val profilerOptions = project.findProperty("profiler.options") as String?
if (profilerOptions != null) {
allArgs.add("-Dddprof.test.options=$profilerOptions")

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Forward profiler options to spawned test JVMs

This only adds ddprof.test.options to the ProfilerTestRunner JVM. Fresh evidence beyond the earlier direct execute cases: tests such as JavaProfilerTest.vmStackwalkerCrashRecoveryTest go through AbstractProcessProfilerTest.launch, which builds a new java command from explicit jvmArgs and a hard-coded profiler command, so testProcessDebug -Pprofiler.options=fjmethodid=false -Ptests=com.datadoghq.profiler.JavaProfilerTest.vmStackwalkerCrashRecoveryTest still profiles the child process without the requested override. Please propagate the property or merge the options before launching ExternalLauncher.

Useful? React with 👍 / 👎.

@rkennke rkennke Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, that is a true concern which should be addressed?

}

// Classpath
allArgs.add("-cp")
allArgs.add(testConfig.testClasspath.asPath)

// Use custom test runner
allArgs.add("com.datadoghq.profiler.test.ProfilerTestRunner")

execTask.args = allArgs
}

// Environment variables
testConfig.environmentVariables.forEach { (key, value) ->
execTask.environment(key, value)
}

// Remove LD_LIBRARY_PATH to let RPATH work correctly
execTask.doFirst {
Comment thread
zhengyu123 marked this conversation as resolved.
val currentLdLibPath = (execTask.environment["LD_LIBRARY_PATH"] as? String) ?: System.getenv("LD_LIBRARY_PATH")
if (!currentLdLibPath.isNullOrEmpty()) {
project.logger.info("Removing LD_LIBRARY_PATH to prevent cross-JDK library conflicts (was: $currentLdLibPath)")
execTask.environment.remove("LD_LIBRARY_PATH")
}
}

// Sanitizer conditions
when (testConfig.configName) {
"asan" -> {
execTask.onlyIf {
PlatformUtils.locateLibasan() != null &&
!PlatformUtils.isTestJvmJ9()
}
// Unlike the Test task's asan variant (setForkEvery(25) above), this task
// is a single un-forked Exec process running the whole suite in one JVM —
// there is no forkEvery equivalent for Exec. An unfiltered run would
// reintroduce the late-suite OOM under ASan's -Xmx512m cap that forkEvery
// was added to avoid, so require -Ptests to bound the run instead.
execTask.doFirst {
if (project.findProperty("tests") == null) {
throw GradleException(
"$taskName requires -Ptests=<ClassName|ClassName.method> for the " +
"asan config: it runs in a single process with no forkEvery " +
"equivalent, and JFR/JMC allocations accumulate under ASan's " +
"-Xmx512m heap cap until an unfiltered run OOMs late in the suite. " +
"For full-suite ASan coverage, use the forked testAsan task instead."
)
}
}
}
"tsan" -> execTask.onlyIf { false }
}
}
}

private fun generateMultiConfigTasks(project: Project, extension: ProfilerTestExtension) {
val nativeBuildExt = project.rootProject.extensions.findByType(NativeBuildExtension::class.java)
?: return // No native build extension, nothing to generate
Expand Down Expand Up @@ -496,6 +602,10 @@ class ProfilerTestPlugin : Plugin<Project> {
createTestTask(project, extension, testConfig, testCfg, sourceSets)
}

// Create process-based test task (always uses Exec, available on all platforms)
// Supports -Pprofiler.options for overriding profiler settings
createProcessTestTask(project, extension, testConfig, testCfg, sourceSets)

// Create application tasks for specified configs
if (configName in applicationConfigs && appMainClass.isNotEmpty()) {
// Create main configuration
Expand Down
75 changes: 73 additions & 2 deletions ddprof-test/src/test/java/com/datadoghq/profiler/AbstractProfilerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,10 +376,81 @@ public final void registerCurrentThreadForWallClockProfiling() {
profiler.addThread();
}

/**
* Merges two profiler command strings. Options in overrides take precedence
* over options in base for matching keys.
*
* @param base The base profiler command (from test's getProfilerCommand())
* @param overrides The override options (from -Pprofiler.options)
* @return Merged command string with overrides taking precedence
*/
private static String mergeProfilerOptions(String base, String overrides) {
Comment thread
zhengyu123 marked this conversation as resolved.
if (overrides == null || overrides.isEmpty()) {
return base;
}

// Parse base options into ordered map
Map<String, String> options = new java.util.LinkedHashMap<>();
for (String part : base.split(",")) {
int eq = part.indexOf('=');
if (eq > 0) {
options.put(part.substring(0, eq), part.substring(eq + 1));
} else if (!part.isEmpty()) {
options.put(part, "");
}
}

// Apply overrides
for (String part : overrides.split(",")) {
Comment thread
zhengyu123 marked this conversation as resolved.
int eq = part.indexOf('=');
if (eq > 0) {
options.put(part.substring(0, eq), part.substring(eq + 1));
} else if (!part.isEmpty()) {
options.put(part, "");
}
}

// Rebuild command, preserving keys with empty values (e.g. "filter=")
// so untouched base options round-trip exactly.
StringBuilder result = new StringBuilder();
for (Map.Entry<String, String> entry : options.entrySet()) {
if (result.length() > 0) {
result.append(",");
}
Comment thread
zhengyu123 marked this conversation as resolved.
result.append(entry.getKey());
result.append("=").append(entry.getValue());

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve no-value profiler flags

When either the base command or -Pprofiler.options contains a flag without an equals sign, this rebuild path stores it as an empty string and then always emits key=. The native parser distinguishes those forms: for example memory uses the default allocation interval while memory= parses to 0, and jvmtistacks enables with no value but jvmtistacks= disables it. As a result, testProcessDebug -Pprofiler.options=memory does not run the requested profiler configuration and can turn on every-allocation sampling instead.

Useful? React with 👍 / 👎.

@rkennke rkennke Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this also sounds like a valid concern that's not addressed?

}
return result.toString();
}

/**
* Applies user-provided options from -Pprofiler.options (via the
* "ddprof.test.options" system property) to a profiler command string,
* overriding any matching keys already present in it.
*
* <p>Tests that build their profiler command directly (rather than through
* {@link #getProfilerCommand()}/{@link #getAmendedProfilerCommand()}) must
* call this before {@code JavaProfiler.execute(...)} so that
Comment thread
zhengyu123 marked this conversation as resolved.
* -Pprofiler.options reliably applies across the whole test set, not just
* to subclasses of this class.
*
* @param profilerCommand the command to apply overrides to
* @return the command with overrides applied, or unchanged if none were requested
*/
public static String applyProfilerOptionOverrides(String profilerCommand) {
String userOptions = System.getProperty("ddprof.test.options");
if (userOptions != null && !userOptions.isEmpty()) {
profilerCommand = mergeProfilerOptions(profilerCommand, userOptions);
Comment thread
zhengyu123 marked this conversation as resolved.
System.out.println("[TEST] Applied profiler.options: " + userOptions);
}
return profilerCommand;
}

private String getAmendedProfilerCommand() {
String profilerCommand = getProfilerCommand();
String profilerCommand = applyProfilerOptionOverrides(getProfilerCommand());

String testCstack = (String)testParams.get("cstack");
if (testCstack != null) {
if (testCstack != null && !profilerCommand.contains("cstack=")) {
profilerCommand += ",cstack=" + testCstack;
Comment thread
zhengyu123 marked this conversation as resolved.
} else if(!(ALLOW_NATIVE_CSTACKS || profilerCommand.contains("cstack="))) {
profilerCommand += ",cstack=fp";
Expand Down
16 changes: 11 additions & 5 deletions ddprof-test/src/test/java/com/datadoghq/profiler/context/OtelContextStorageModeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.datadoghq.profiler.context;

import com.datadoghq.profiler.AbstractProfilerTest;
import com.datadoghq.profiler.JavaProfiler;
import com.datadoghq.profiler.ThreadContext;
import org.junit.jupiter.api.AfterEach;
Expand Down Expand Up @@ -58,7 +59,8 @@ public void cleanup() {
public void testOtelStorageModeContext() throws Exception {
Path jfrFile = Files.createTempFile("otel-ctx-otel", ".jfr");

profiler.execute(String.format("start,cpu=1ms,attributes=tag1;tag2;tag3,jfr,file=%s", jfrFile.toAbsolutePath()));
profiler.execute(String.format("start,%s,jfr,file=%s",
AbstractProfilerTest.applyProfilerOptionOverrides("cpu=1ms,attributes=tag1;tag2;tag3"), jfrFile.toAbsolutePath()));
profilerStarted = true;

long localRootSpanId = 0x1111222233334444L;
Expand All @@ -84,7 +86,8 @@ public void testOtelStorageModeContext() throws Exception {
public void testOtelModeCustomAttributes() throws Exception {
Path jfrFile = Files.createTempFile("otel-ctx-attrs", ".jfr");

profiler.execute(String.format("start,cpu=1ms,attributes=http.route;db.system,jfr,file=%s", jfrFile.toAbsolutePath()));
profiler.execute(String.format("start,%s,jfr,file=%s",
AbstractProfilerTest.applyProfilerOptionOverrides("cpu=1ms,attributes=http.route;db.system"), jfrFile.toAbsolutePath()));
profilerStarted = true;

long localRootSpanId = 0x1111222233334444L;
Expand Down Expand Up @@ -114,7 +117,8 @@ public void testOtelModeCustomAttributes() throws Exception {
public void testOtelModeAttributeOverflow() throws Exception {
Path jfrFile = Files.createTempFile("otel-ctx-overflow", ".jfr");

profiler.execute(String.format("start,cpu=1ms,attributes=k0;k1;k2;k3;k4,jfr,file=%s", jfrFile.toAbsolutePath()));
profiler.execute(String.format("start,%s,jfr,file=%s",
AbstractProfilerTest.applyProfilerOptionOverrides("cpu=1ms,attributes=k0;k1;k2;k3;k4"), jfrFile.toAbsolutePath()));
profilerStarted = true;

profiler.setContext(0x2L, 0x1L, 0L, 0x3L);
Expand Down Expand Up @@ -203,7 +207,8 @@ public void testThreadIsolation() throws InterruptedException {
@Test
public void testSpanTransitionClearsAttributes() throws Exception {
Path jfrFile = Files.createTempFile("otel-ctx-transition", ".jfr");
profiler.execute(String.format("start,cpu=1ms,attributes=http.route,jfr,file=%s", jfrFile.toAbsolutePath()));
profiler.execute(String.format("start,%s,jfr,file=%s",
AbstractProfilerTest.applyProfilerOptionOverrides("cpu=1ms,attributes=http.route"), jfrFile.toAbsolutePath()));
profilerStarted = true;

// Span A: set a custom attribute
Expand Down Expand Up @@ -246,7 +251,8 @@ public void testRepeatedContextWrites() {
@Test
public void testAttributeCacheIsolation() throws Exception {
Path jfrFile = Files.createTempFile("otel-attr-cache-iso", ".jfr");
profiler.execute(String.format("start,cpu=1ms,attributes=attr0,jfr,file=%s", jfrFile.toAbsolutePath()));
profiler.execute(String.format("start,%s,jfr,file=%s",
AbstractProfilerTest.applyProfilerOptionOverrides("cpu=1ms,attributes=attr0"), jfrFile.toAbsolutePath()));
profilerStarted = true;

final String valueA = "FB"; // hashCode = 2236, slot 188
Expand Down
6 changes: 4 additions & 2 deletions ddprof-test/src/test/java/com/datadoghq/profiler/lifecycle/StartWithChurningThreadsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package com.datadoghq.profiler.lifecycle;

import com.datadoghq.profiler.AbstractProfilerTest;
import com.datadoghq.profiler.JavaProfiler;
import com.datadoghq.profiler.Platform;

Expand Down Expand Up @@ -48,7 +49,7 @@ public class StartWithChurningThreadsTest {
private static final int CYCLES = 500;
// filter= disables thread filtering so all threads receive SIGVTALRM,
// maximising the chance of racing execute() with active thread churn.
private static final String PROFILER_CMD = "start,wall=1ms,filter=,jfr,file=";
private static final String PROFILER_OPTIONS = "wall=1ms,filter=";

@Timeout(120)
@Test
Expand All @@ -60,6 +61,7 @@ public void startRacesThreadStartEnd() throws Exception {
JavaProfiler profiler = JavaProfiler.getInstance();
Path recordings = Files.createTempDirectory("lifecycle-cell2-");
Queue<Throwable> errors = new LinkedBlockingQueue<>();
String profilerCmd = "start," + AbstractProfilerTest.applyProfilerOptionOverrides(PROFILER_OPTIONS) + ",jfr,file=";

AtomicBoolean running = new AtomicBoolean(true);
// Latch: churn threads signal they are running before execute() is called,
Expand All @@ -84,7 +86,7 @@ public void startRacesThreadStartEnd() throws Exception {
Path jfr = Files.createTempFile(recordings, "c2-" + cycle + "-", ".jfr");
try {
// execute() races with active thread churn
profiler.execute(PROFILER_CMD + jfr.toAbsolutePath());
profiler.execute(profilerCmd + jfr.toAbsolutePath());
Thread.sleep(cycle % 3);
profiler.stop();
} catch (Throwable t) {
Expand Down
6 changes: 4 additions & 2 deletions ddprof-test/src/test/java/com/datadoghq/profiler/lifecycle/StopWithChurningThreadsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package com.datadoghq.profiler.lifecycle;

import com.datadoghq.profiler.AbstractProfilerTest;
import com.datadoghq.profiler.JavaProfiler;
import com.datadoghq.profiler.Platform;

Expand Down Expand Up @@ -49,7 +50,7 @@ public class StopWithChurningThreadsTest {
// wall=1ms maximises signals in-flight during teardown; filter= disables
// thread filtering so all threads receive SIGVTALRM, increasing collision
// probability with ThreadEnd.
private static final String PROFILER_CMD = "start,wall=1ms,filter=,jfr,file=";
private static final String PROFILER_OPTIONS = "wall=1ms,filter=";

@Timeout(120)
@Test
Expand All @@ -61,6 +62,7 @@ public void stopRacesThreadEnd() throws Exception {
JavaProfiler profiler = JavaProfiler.getInstance();
Path recordings = Files.createTempDirectory("lifecycle-cell1-");
Queue<Throwable> errors = new LinkedBlockingQueue<>();
String profilerCmd = "start," + AbstractProfilerTest.applyProfilerOptionOverrides(PROFILER_OPTIONS) + ",jfr,file=";

AtomicBoolean running = new AtomicBoolean(true);
CountDownLatch churnRunning = new CountDownLatch(CHURN_THREADS);
Expand All @@ -83,7 +85,7 @@ public void stopRacesThreadEnd() throws Exception {
for (int cycle = 0; cycle < CYCLES; cycle++) {
Path jfr = Files.createTempFile(recordings, "c1-" + cycle + "-", ".jfr");
try {
profiler.execute(PROFILER_CMD + jfr.toAbsolutePath());
profiler.execute(profilerCmd + jfr.toAbsolutePath());
// Minimal delay: hit stop() while signals are still in-flight
// and threads are actively dying. Varies per cycle to avoid
// always landing in the same phase of the signal period.
Expand Down
2 changes: 1 addition & 1 deletion ddprof-test/src/test/java/com/datadoghq/profiler/memleak/CleanupAfterClassUnloadTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public void testNoSigsegvAfterClassUnloadAndMethodCleanup() throws Exception {

try {
profiler.execute(
"start,cpu=1ms,jfr,mcleanup=true,file=" + baseFile.toAbsolutePath());
"start," + applyProfilerOptionOverrides(getProfilerCommand()) + ",jfr,mcleanup=true,file=" + baseFile.toAbsolutePath());
Thread.sleep(200); // Let the profiler stabilize

// 1. Load a class, execute its methods to appear in CPU profiling stack traces,
Expand Down
Loading
Loading