Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,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
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,95 @@ 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)
if (extension.nativeLibDir.isPresent) {
allArgs.add("-Djava.library.path=${extension.nativeLibDir.get().asFile.absolutePath}")
}
allArgs.addAll(testConfig.extraJvmArgs)

// 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")
}

// 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 {
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()
}
"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 @@ -423,6 +512,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
Original file line number Diff line number Diff line change
Expand Up @@ -376,10 +376,67 @@ 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(",")) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔵 LOW · robustness [GENERALIST]

A valueless override token (e.g. -Pprofiler.options=cpu) silently clears the value of a matching base option: cpu=10ms in the base becomes bare cpu in the merged result. Easy to trigger accidentally and undocumented.

Suggestion: Document this behavior in the mergeProfilerOptions Javadoc so users know that a bare key override strips any existing value. Alternatively, use putIfAbsent to avoid overwriting an existing valued key with an empty one:

else if (!part.isEmpty()) {
    options.putIfAbsent(part, "");
}

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
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.
// Skip key with empty value
if (!entry.getValue().isEmpty()) {
Comment on lines +419 to +420

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 empty-valued profiler options

When testProcessDebug is run with any -Pprofiler.options, the merge path rebuilds every base command and drops entries whose value is empty. That changes existing tests such as VirtualThreadWallClockTest, whose base command is wall=1ms,filter=; adding an unrelated override like -Pprofiler.options=fjmethodid=false silently removes filter= (and can leave a doubled comma), so the new task no longer runs the same profiler configuration plus the requested override.

Useful? React with 👍 / 👎.

result.append(entry.getKey());
result.append("=").append(entry.getValue());
}
}
return result.toString();
}

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

// Apply user-provided options from -Pprofiler.options (override test defaults)
String userOptions = System.getProperty("ddprof.test.options");
if (userOptions != null && !userOptions.isEmpty()) {
profilerCommand = mergeProfilerOptions(profilerCommand, userOptions);
Comment on lines +432 to +434

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 Apply profiler options to all profiler starts

When testProcessDebug is run without -Ptests, this system property is only consumed by AbstractProfilerTest; tests that call JavaProfiler.execute(...) directly still start the profiler with their hard-coded commands (for example ShutdownTest.runTest and OtelContextStorageModeTest). That means a process run requested with -Pprofiler.options=fjmethodid=false still exercises part of the suite with the default profiler options, so it is not a reliable way to validate that option across the task's full test set.

Useful? React with 👍 / 👎.

System.out.println("[TEST] Applied profiler.options: " + userOptions);
}

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
Loading