Implement Config Inversion with Default Strictness of Warning
#9181
+3,236
−471
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 49 metrics, 10 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.53.0-SNAPSHOT~94f392cee2, baseline=1.53.0-SNAPSHOT~5aa25baa32
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.051 s) : 0, 1051206
Total [baseline] (8.654 s) : 0, 8653868
Agent [candidate] (1.062 s) : 0, 1062013
Total [candidate] (8.648 s) : 0, 8648313
section iast
Agent [baseline] (1.184 s) : 0, 1183906
Total [baseline] (9.351 s) : 0, 9351433
Agent [candidate] (1.194 s) : 0, 1193839
Total [candidate] (9.372 s) : 0, 9371638
gantt
title insecure-bank - break down per module: candidate=1.53.0-SNAPSHOT~94f392cee2, baseline=1.53.0-SNAPSHOT~5aa25baa32
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.46 ms) : 0, 1460
crashtracking [candidate] (1.49 ms) : 0, 1490
BytebuddyAgent [baseline] (736.18 ms) : 0, 736180
BytebuddyAgent [candidate] (745.917 ms) : 0, 745917
GlobalTracer [baseline] (243.9 ms) : 0, 243900
GlobalTracer [candidate] (245.213 ms) : 0, 245213
AppSec [baseline] (30.253 ms) : 0, 30253
AppSec [candidate] (30.587 ms) : 0, 30587
Debugger [baseline] (6.077 ms) : 0, 6077
Debugger [candidate] (6.12 ms) : 0, 6120
Remote Config [baseline] (667.728 µs) : 0, 668
Remote Config [candidate] (673.228 µs) : 0, 673
Telemetry [baseline] (11.518 ms) : 0, 11518
Telemetry [candidate] (10.747 ms) : 0, 10747
section iast
crashtracking [baseline] (1.455 ms) : 0, 1455
crashtracking [candidate] (1.488 ms) : 0, 1488
BytebuddyAgent [baseline] (853.605 ms) : 0, 853605
BytebuddyAgent [candidate] (862.363 ms) : 0, 862363
GlobalTracer [baseline] (235.01 ms) : 0, 235010
GlobalTracer [candidate] (237.442 ms) : 0, 237442
IAST [baseline] (27.165 ms) : 0, 27165
IAST [candidate] (28.7 ms) : 0, 28700
AppSec [baseline] (27.93 ms) : 0, 27930
AppSec [candidate] (28.148 ms) : 0, 28148
Debugger [baseline] (8.48 ms) : 0, 8480
Debugger [candidate] (5.715 ms) : 0, 5715
Remote Config [baseline] (612.943 µs) : 0, 613
Remote Config [candidate] (588.317 µs) : 0, 588
Telemetry [baseline] (8.506 ms) : 0, 8506
Telemetry [candidate] (8.152 ms) : 0, 8152
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.53.0-SNAPSHOT~94f392cee2, baseline=1.53.0-SNAPSHOT~5aa25baa32
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.05 s) : 0, 1050331
Total [baseline] (10.686 s) : 0, 10686264
Agent [candidate] (1.054 s) : 0, 1054429
Total [candidate] (10.772 s) : 0, 10771812
section appsec
Agent [baseline] (1.232 s) : 0, 1231740
Total [baseline] (10.905 s) : 0, 10904714
Agent [candidate] (1.235 s) : 0, 1234860
Total [candidate] (10.951 s) : 0, 10951478
section iast
Agent [baseline] (1.181 s) : 0, 1181187
Total [baseline] (10.928 s) : 0, 10928349
Agent [candidate] (1.193 s) : 0, 1192927
Total [candidate] (11.106 s) : 0, 11105819
section profiling
Agent [baseline] (1.2 s) : 0, 1200239
Total [baseline] (10.942 s) : 0, 10942117
Agent [candidate] (1.204 s) : 0, 1204447
Total [candidate] (11.018 s) : 0, 11017844
gantt
title petclinic - break down per module: candidate=1.53.0-SNAPSHOT~94f392cee2, baseline=1.53.0-SNAPSHOT~5aa25baa32
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.451 ms) : 0, 1451
crashtracking [candidate] (1.493 ms) : 0, 1493
BytebuddyAgent [baseline] (734.706 ms) : 0, 734706
BytebuddyAgent [candidate] (741.396 ms) : 0, 741396
GlobalTracer [baseline] (243.732 ms) : 0, 243732
GlobalTracer [candidate] (243.86 ms) : 0, 243860
AppSec [baseline] (30.235 ms) : 0, 30235
AppSec [candidate] (30.357 ms) : 0, 30357
Debugger [baseline] (6.069 ms) : 0, 6069
Debugger [candidate] (6.102 ms) : 0, 6102
Remote Config [baseline] (670.349 µs) : 0, 670
Remote Config [candidate] (673.252 µs) : 0, 673
Telemetry [baseline] (12.344 ms) : 0, 12344
Telemetry [candidate] (9.39 ms) : 0, 9390
section appsec
crashtracking [baseline] (1.454 ms) : 0, 1454
crashtracking [candidate] (1.487 ms) : 0, 1487
BytebuddyAgent [baseline] (760.524 ms) : 0, 760524
BytebuddyAgent [candidate] (763.932 ms) : 0, 763932
GlobalTracer [baseline] (237.302 ms) : 0, 237302
GlobalTracer [candidate] (238.042 ms) : 0, 238042
IAST [baseline] (23.653 ms) : 0, 23653
IAST [candidate] (23.736 ms) : 0, 23736
AppSec [baseline] (170.257 ms) : 0, 170257
AppSec [candidate] (171.612 ms) : 0, 171612
Debugger [baseline] (8.105 ms) : 0, 8105
Debugger [candidate] (5.752 ms) : 0, 5752
Remote Config [baseline] (645.914 µs) : 0, 646
Remote Config [candidate] (648.014 µs) : 0, 648
Telemetry [baseline] (8.597 ms) : 0, 8597
Telemetry [candidate] (8.384 ms) : 0, 8384
section iast
crashtracking [baseline] (1.448 ms) : 0, 1448
crashtracking [candidate] (1.483 ms) : 0, 1483
BytebuddyAgent [baseline] (851.458 ms) : 0, 851458
BytebuddyAgent [candidate] (861.086 ms) : 0, 861086
GlobalTracer [baseline] (234.193 ms) : 0, 234193
GlobalTracer [candidate] (235.667 ms) : 0, 235667
IAST [baseline] (29.612 ms) : 0, 29612
IAST [candidate] (32.395 ms) : 0, 32395
AppSec [baseline] (27.785 ms) : 0, 27785
AppSec [candidate] (26.483 ms) : 0, 26483
Debugger [baseline] (6.634 ms) : 0, 6634
Debugger [candidate] (5.792 ms) : 0, 5792
Remote Config [baseline] (606.805 µs) : 0, 607
Remote Config [candidate] (593.416 µs) : 0, 593
Telemetry [baseline] (8.354 ms) : 0, 8354
Telemetry [candidate] (8.267 ms) : 0, 8267
section profiling
crashtracking [baseline] (1.424 ms) : 0, 1424
crashtracking [candidate] (1.431 ms) : 0, 1431
BytebuddyAgent [baseline] (765.09 ms) : 0, 765090
BytebuddyAgent [candidate] (767.321 ms) : 0, 767321
GlobalTracer [baseline] (222.524 ms) : 0, 222524
GlobalTracer [candidate] (224.038 ms) : 0, 224038
AppSec [baseline] (30.06 ms) : 0, 30060
AppSec [candidate] (30.871 ms) : 0, 30871
Debugger [baseline] (6.245 ms) : 0, 6245
Debugger [candidate] (6.274 ms) : 0, 6274
Remote Config [baseline] (699.801 µs) : 0, 700
Remote Config [candidate] (700.543 µs) : 0, 701
Telemetry [baseline] (15.818 ms) : 0, 15818
Telemetry [candidate] (15.808 ms) : 0, 15808
ProfilingAgent [baseline] (108.48 ms) : 0, 108480
ProfilingAgent [candidate] (107.501 ms) : 0, 107501
Profiling [baseline] (109.136 ms) : 0, 109136
Profiling [candidate] (108.171 ms) : 0, 108171
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 1 performance regressions! Performance is the same for 10 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.53.0-SNAPSHOT~94f392cee2, baseline=1.53.0-SNAPSHOT~5aa25baa32
dateFormat X
axisFormat %s
section baseline
no_agent (4.421 ms) : 4370, 4471
. : milestone, 4421,
iast (9.301 ms) : 9143, 9460
. : milestone, 9301,
iast_FULL (14.042 ms) : 13764, 14319
. : milestone, 14042,
iast_GLOBAL (10.268 ms) : 10083, 10454
. : milestone, 10268,
profiling (8.502 ms) : 8365, 8639
. : milestone, 8502,
tracing (7.784 ms) : 7672, 7896
. : milestone, 7784,
section candidate
no_agent (4.319 ms) : 4271, 4368
. : milestone, 4319,
iast (9.658 ms) : 9493, 9823
. : milestone, 9658,
iast_FULL (13.845 ms) : 13570, 14119
. : milestone, 13845,
iast_GLOBAL (10.579 ms) : 10393, 10764
. : milestone, 10579,
profiling (8.845 ms) : 8705, 8984
. : milestone, 8845,
tracing (7.682 ms) : 7573, 7792
. : milestone, 7682,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.53.0-SNAPSHOT~94f392cee2, baseline=1.53.0-SNAPSHOT~5aa25baa32
dateFormat X
axisFormat %s
section baseline
no_agent (38.316 ms) : 38007, 38624
. : milestone, 38316,
appsec (48.111 ms) : 47670, 48552
. : milestone, 48111,
code_origins (46.532 ms) : 46116, 46947
. : milestone, 46532,
iast (44.89 ms) : 44515, 45266
. : milestone, 44890,
profiling (47.536 ms) : 47102, 47970
. : milestone, 47536,
tracing (42.878 ms) : 42506, 43251
. : milestone, 42878,
section candidate
no_agent (38.375 ms) : 38066, 38683
. : milestone, 38375,
appsec (47.148 ms) : 46742, 47553
. : milestone, 47148,
code_origins (45.145 ms) : 44752, 45539
. : milestone, 45145,
iast (45.089 ms) : 44687, 45491
. : milestone, 45089,
profiling (47.398 ms) : 46918, 47877
. : milestone, 47398,
tracing (42.587 ms) : 42222, 42953
. : milestone, 42587,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.53.0-SNAPSHOT~94f392cee2, baseline=1.53.0-SNAPSHOT~5aa25baa32
dateFormat X
axisFormat %s
section baseline
no_agent (14.967 s) : 14967000, 14967000
. : milestone, 14967000,
appsec (14.927 s) : 14927000, 14927000
. : milestone, 14927000,
iast (18.871 s) : 18871000, 18871000
. : milestone, 18871000,
iast_GLOBAL (18.005 s) : 18005000, 18005000
. : milestone, 18005000,
profiling (15.445 s) : 15445000, 15445000
. : milestone, 15445000,
tracing (15.035 s) : 15035000, 15035000
. : milestone, 15035000,
section candidate
no_agent (15.492 s) : 15492000, 15492000
. : milestone, 15492000,
appsec (15.25 s) : 15250000, 15250000
. : milestone, 15250000,
iast (18.607 s) : 18607000, 18607000
. : milestone, 18607000,
iast_GLOBAL (18.277 s) : 18277000, 18277000
. : milestone, 18277000,
profiling (15.236 s) : 15236000, 15236000
. : milestone, 15236000,
tracing (14.934 s) : 14934000, 14934000
. : milestone, 14934000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.53.0-SNAPSHOT~94f392cee2, baseline=1.53.0-SNAPSHOT~5aa25baa32
dateFormat X
axisFormat %s
section baseline
no_agent (1.487 ms) : 1475, 1498
. : milestone, 1487,
appsec (3.679 ms) : 3460, 3899
. : milestone, 3679,
iast (2.228 ms) : 2165, 2291
. : milestone, 2228,
iast_GLOBAL (2.253 ms) : 2190, 2317
. : milestone, 2253,
profiling (2.061 ms) : 2010, 2112
. : milestone, 2061,
tracing (2.031 ms) : 1982, 2080
. : milestone, 2031,
section candidate
no_agent (1.483 ms) : 1471, 1494
. : milestone, 1483,
appsec (3.656 ms) : 3437, 3874
. : milestone, 3656,
iast (2.217 ms) : 2154, 2281
. : milestone, 2217,
iast_GLOBAL (2.254 ms) : 2191, 2318
. : milestone, 2254,
profiling (2.078 ms) : 2026, 2130
. : milestone, 2078,
tracing (2.032 ms) : 1982, 2081
. : milestone, 2032,
|
mhlidd
force-pushed
the
mhlidd/config_inversion
branch
2 times, most recently
from
06c162d to
f7aaa15
Compare
…Configurations, and DD_ linting logger
mhlidd
requested review from
nikita-tkachenko-datadog,
AlexeyKuznetsov-DD and
dougqh
and removed request for
a team
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
mhlidd
added
type: enhancement
| @@ -7,6 +7,6 @@ apply(from = "$rootDir/gradle/java.gradle") | |||
| dependencies { | |||
| implementation(project(":components:environment")) | |||
| implementation(libs.slf4j) | |||
|
|
|||
| implementation(project(":internal-api")) | |||
There was a problem hiding this comment.
🔨 issue: I don't think :utils module should depend on bigger generic module as :internal-api. It should only be the other way around.
There was a problem hiding this comment.
The only reason I have it here is so that I can use ConfigHelper in ServerlessInfo in the utils module. Do you know of a way to access it without having utils implement internal-api?
components/environment/src/generator/java/datadog/environment/ParseSupportedConfigurations.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The overall PR feels a bit messy to review (and the git log does not help).
Would you consider splitting this PR into the config change first, and then the refactoring to use it? (You can keep this one for the config change if you avoid loosing the review comment, just exclude the refactoring changes and push them in another stack branch from which you can start a second PR)
Because the more I read about it, the more I think that you should start a dedicated module for the config thing with all the recent changes like:
- the Stable config thing (the
:component:yamlwhich is not a component), - the
java/datadog/trace/bootstrap/config/providerfrom:internal-apiwhich containsConfigSourceandConfigSource.Provider, stable config stuff -- but exclude theAgentArgsthings which are really an agent thing - the new config inversion thin.
All those features are getting mixed up between several modules -components, agent bootstrap, :internal-api, dd-trace-api (the public API) - and barely make any sense now, and are tough to review and maintain.
So what about starting something in :utils like a :utils:config?
You will not be able to make a new component right now as you will still depend of :internal-api and such decoupling would be way out of scope.
It could be useful to pair with @mtoffl01 for Config and @bric3 for the Gradle build (especially for the Generator).
| @@ -0,0 +1,45 @@ | |||
| plugins { | |||
| `java-library` | |||
| id("com.gradleup.shadow") | |||
There was a problem hiding this comment.
❔ question: What the shadow plugin is used for?
Comment on lines
7
to
9
| create("generator") { | ||
| java.srcDir("src/generator/java") | ||
| } |
There was a problem hiding this comment.
❔ question: There are some maven conventions about generated source file. Can't we use them instead?
|
|
||
| val compileGeneratorJava = tasks.named("compileGeneratorJava") | ||
|
|
||
| val generateSupportedConfigurations by tasks.registering(JavaExec::class) { |
There was a problem hiding this comment.
🔨 issue: Input and output task defintions are missing. Isn't breaking the build / task cache?
| String forwarderPath = EnvironmentVariables.get("DD_TELEMETRY_FORWARDER_PATH"); | ||
| String forwarderPath = System.getenv("DD_TELEMETRY_FORWARDER_PATH"); |
There was a problem hiding this comment.
🔨 issue: System.getenv.
You're getting rid of the security introduced. And it's during the bootstrap, so you will crash customers with this kind of change.
| import java.util.Locale; | ||
|
|
||
| /** Trace propagation styles for injecting and extracting trace propagation headers. */ | ||
| public enum ConfigInversionStrictStyle { |
There was a problem hiding this comment.
❔ question: Is this supposed to be part of our public API?
|
|
||
| @Override | ||
| public String toString() { | ||
| if (displayName == null) { |
There was a problem hiding this comment.
🔨 issue: This can't be null
gradle/configInversionLinter.gradle
Outdated
Comment on lines
46
to
49
| if (inBlockComment) { | ||
| if (trimmed.contains("*/")) inBlockComment = false | ||
| return | ||
| } |
There was a problem hiding this comment.
🧹 chore: Formatting. Isn't spotless applied here?
gradle/configInversionLinter.gradle
Outdated
Comment on lines
109
to
117
| tasks.named('spotlessCheck') { | ||
| dependsOn tasks.named('logEnvVarUsages') | ||
| dependsOn tasks.named('checkEnvironmentVariablesUsage') | ||
| } | ||
|
|
||
| tasks.named('spotlessApply') { | ||
| dependsOn tasks.named('logEnvVarUsages') | ||
| dependsOn tasks.named('checkEnvironmentVariablesUsage') | ||
| } |
There was a problem hiding this comment.
🔨 issue: This should not be hooked to spotless as it does not concern formatting. Rather the check.
Similarly, no input / output task definition. What about the build / task cache?
Comment on lines
63
to
70
| // TODO: Follow-up - Add deprecation handling | ||
| // if (configSource.getDeprecatedConfigurations().containsKey(key)) { | ||
| // String warning = "Environment variable " + key + " is deprecated. " + | ||
| // (configSource.getAliasMapping().containsKey(key) | ||
| // ? "Please use " + configSource.getAliasMapping().get(key) + " instead." | ||
| // : configSource.getDeprecatedConfigurations().get(key)); | ||
| // System.err.println(warning); | ||
| // } |
There was a problem hiding this comment.
❔ question: Leftover?
mhlidd
requested review from
ojung,
robertpi and
manuel-alvarez-alvarez
and removed request for
a team
bric3
reviewed
Aug 27, 2025
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| public class ParseSupportedConfigurations { |
There was a problem hiding this comment.
suggestion: I would make this a gradle plugin, or at least a plain gradle task.
There was a problem hiding this comment.
After chatting here's an brief outline of how it could be done
supported config generation plugin
Declare the plugin in the buildSrc/build.gradle.kts file.
gradlePlugin {
plugins {
+ create("config-generation") {
+ id = "supported-config-generator"
+ implementationClass = "datadog.gradle.plugin.config.SupportedConfigPlugin"
+ }
}
}in utils/config-utils/build.gradle.kts
plugins {
`java-library`
+ id("supported-config-generator") // id declared in buildSrc/build.gradle.kts
}I suggest this package : datadog.gradle.plugin.config
package datadog.gradle.plugin.config
import org.gradle.api.Plugin
import org.gradle.api.Project
import org.gradle.api.tasks.SourceSet
import org.gradle.api.tasks.SourceSetContainer
class SupportedConfigPlugin : Plugin<Project> {
override fun apply(targetProject: Project) {
generateSupportedCOnf(targetProject)
}
private fun generateSupportedCOnf(targetProject: Project) {
val generateTask =
targetProject.tasks.register("generateSupportedConfigurations", ParseSupportedConfigurationsTask::class.java) {
jsonFile.set(project.file("../supported-configurations.json"))
destinationDirectory.set(project.layout.buildDirectory.dir("generated/supportedConfigurations"))
className.set("datagog.config.SupportedConfigurations")
}
// Ensure Java compilation depends on the generated sources
val sourceset = targetProject.extensions.getByType(SourceSetContainer::class.java).named(SourceSet.MAIN_SOURCE_SET_NAME)
sourceset.configure {
java.srcDir(generateTask)
}
}
}No need to deal with task ordering or dependencies, by declaring the generateTask in srcDir, Gradle understand it needs to run this task before even compiling this "source set". This assumes however that the task has properly declared its inputs/outputs.
In general with modern gradle "dependsOn" is usually a "code smell".
generation task
This task is responsible to generate the code where ParseSupportedConfigurations methods can mostly be inlined. The properties (the kotlin vals) are annotated to declare the input/outputs.
package datadog.gradle.plugin.config
import org.gradle.api.DefaultTask
import org.gradle.api.file.RegularFile
import org.gradle.api.model.ObjectFactory
import org.gradle.api.provider.Property
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.OutputDirectory
import org.gradle.api.tasks.TaskAction
import javax.inject.Inject
/**
* Equivalent of
* ```
* val generateSupportedConfigurations by tasks.registering(JavaExec::class) {
* // We can run the generator with the main sourceSet runtimeClasspath
* dependsOn(compileGeneratorJava)
* mainClass.set("datadog.generator.ParseSupportedConfigurations")
* classpath = sourceSets["generator"].runtimeClasspath
*
* val outputFile = layout.buildDirectory.file("generated/sources/supported/GeneratedSupportedConfigurations.java")
* args("supported-configurations.json", outputFile.get().asFile.absolutePath)
*
* doFirst {
* outputFile.get().asFile.parentFile.mkdirs()
* }
* }
* ```
*/
abstract class ParseSupportedConfigurationsTask @Inject constructor(
private val objects: ObjectFactory
) : DefaultTask() {
@Input
val jsonFile = objects.fileProperty()
@get:OutputDirectory
val destinationDirectory = objects.directoryProperty()
@Input
val className = objects.property(String::class.java)
@TaskAction
fun generate() {
val input = jsonFile.get().asFile
val outputDir = destinationDirectory.get().asFile
val className = className.get()
outputDir.mkdirs()
// ...
// inline ParseSupportedConfigurations.main
}
// inline ParseSupportedConfigurations.generateJavaFile
}
utils/config-utils/build.gradle.kts
Outdated
| mainClass.set("datadog.generator.ParseSupportedConfigurations") | ||
| classpath = sourceSets["generator"].runtimeClasspath | ||
|
|
||
| val outputFile = layout.buildDirectory.file("generated/sources/supported/GeneratedSupportedConfigurations.java") |
There was a problem hiding this comment.
issue: This file is at the package root, shouldn't it be in something like datadog.config ?
| Map<String, String> deprecated) | ||
| throws IOException { | ||
| try (PrintWriter out = new PrintWriter(Files.newBufferedWriter(Paths.get(outputPath)))) { | ||
| out.println("package datadog.generator;"); |
There was a problem hiding this comment.
issue: package looks suspicious. Also it should be derived from the input args.
Comment on lines
17
to
21
| public static void main(String[] args) { | ||
| String supportedConfigurationsFilename = | ||
| args[0]; // e.g., "resources/supported-configurations.json" | ||
| String generatedMappingPath = args[1]; // e.g., | ||
| // "build/generated-sources/datadog/environment/GeneratedSupportedConfigurations.java" |
There was a problem hiding this comment.
suggestion: Use 3 args: the support config json document, the source output folder, the class name (from which package, simple class name, and relative file path can be derived).
| @@ -19,6 +19,7 @@ public final class Constants { | |||
| "datadog.environment", | |||
| "datadog.json", | |||
| "datadog.yaml", | |||
| "datadog.generator", | |||
There was a problem hiding this comment.
question: Is this correct? The generator code shouldn't be packaged, only the generated file which is currently not in this package anyway:
val outputFile = layout.buildDirectory.file("generated/sources/supported/GeneratedSupportedConfigurations.java")
| import datadog.trace.util.Strings; | ||
| import datadog.config.util.Strings; |
There was a problem hiding this comment.
thought: Is it needed to move Strings ?
Also, need to check with bootstrap there, as some packages maybe relocated.
| import datadog.trace.util.Strings | ||
| import Strings |
| @@ -136,6 +135,7 @@ include( | |||
| // misc | |||
| include( | |||
| ":dd-java-agent:testing", | |||
| ":utils:config-utils", | |||
There was a problem hiding this comment.
question: Thinking out loud I wonder if such module should be within :utils as it's customer facing ?
| @@ -598,3 +598,5 @@ include( | |||
| ":dd-java-agent:benchmark-integration:jetty-perftest", | |||
| ":dd-java-agent:benchmark-integration:play-perftest", | |||
| ) | |||
|
|
|||
| include("utils:config-utils") | |||
There was a problem hiding this comment.
issue: Unneeded as declared line 138 in this file.
gradle/configInversionLinter.gradle
Outdated
| import groovy.json.JsonSlurper | ||
| import java.nio.file.Path | ||
|
|
||
| tasks.register('logEnvVarUsages') { |
There was a problem hiding this comment.
suggestion: Move this as plugin in buildSrc
There was a problem hiding this comment.
I suggest a non-DSL plugin like in this #9181 (comment), so related code live in the same package.
bric3
reviewed
Aug 28, 2025
| @@ -30,6 +30,14 @@ gradlePlugin { | |||
| id = "tracer-version" | |||
| implementationClass = "datadog.gradle.plugin.version.TracerVersionPlugin" | |||
| } | |||
| create("config-generation") { | |||
There was a problem hiding this comment.
nitpick: This doesn;t have any effect really, but it just gives the same prefix to the name.
Suggested change
| create("config-generation") { | |
| create("supported-config-generation") { |
| out.println() | ||
| out.println("import java.util.*;") | ||
| out.println() | ||
| out.println("public final class GeneratedSupportedConfigurations {") |
There was a problem hiding this comment.
note: Shouldn't this be coming from the className property ?
Comment on lines
+20
to
+21
| val ownerPath = ":utils:config-utils" // <-- change to the module that contains the generated class | ||
| val generatedFile = "datadog.config.GeneratedSupportedConfigurations" |
There was a problem hiding this comment.
issue: I'm thinking this should be configured via an extension, it's a gradle configuration mechanism.
Basically, the idea is to introduce an extension that will hold the configuration for both plugins, and the tasks registered from these plugins will grab the configuration from it.
Something along the lines
open class SupportedTracerConfigurations @Inject constructor(objects: ObjectFactory) {
val configOwnerPath = objects.property<String>().convention(":utils:config-utils")
val className = objects.property<String>().convention("datadog.config.GeneratedSupportedConfigurations")
val jsonFile = objects.fileProperty().convention(project.file("src/main/resources/supported-configurations.json"))
val destinationDirectory = objects.directoryProperty().convention(project.layout.buildDirectory.dir("generated/supportedConfigurations"))
// ... other configs
// maybe excluded files in specific submodules like "internal-api/...", "dd-java-agent/..."
}Then in the plugin before registering tasks
override fun apply(target: Project) {
val extension = target.extensions.create("supportedTracerConfigurations", SupportedTracerConfigurations::class.java)
}then access the extension vals as needed.
Note, in the example above I'm leveraging .convention(...) which sets "default" values.
This extension class should hold most of the values that are spread in various places.
Comment on lines
+28
to
+36
| val owner = target.project(ownerPath) | ||
| val sourceSets = owner.extensions.getByType(SourceSetContainer::class.java) | ||
| val main = sourceSets.getByName(SourceSet.MAIN_SOURCE_SET_NAME) | ||
|
|
||
| // ensure the owner project generated/compiled the class | ||
| dependsOn( | ||
| owner.tasks.named("generateSupportedConfigurations"), | ||
| owner.tasks.named("classes") // compiles main (including generated sources) | ||
| ) |
There was a problem hiding this comment.
suggestion: I believe this can be replaced by declaring the config owner output as input of this task. E.g. something like
val mainSourceSetOutput = target.project(configOwnerPath).extensions.getByType<SourceSetContainer>().named(SourceSet.MAIN_SOURCE_SET_NAME).map { it.output } // it's a Provider
inputs.files(mainSourceSetOutput)I believe using the source set output is enough to load the class. But using the runtimeClasspath instead should work as well.
| exclude("**/build/**", "**/dd-smoke-tests/**") | ||
| } | ||
| inputs.files(javaFiles) | ||
| outputs.upToDateWhen { true} |
There was a problem hiding this comment.
typo: space
Suggested change
| outputs.upToDateWhen { true} | |
| outputs.upToDateWhen { true } |
Comment on lines
+47
to
+53
| val urls = main.runtimeClasspath.files.map { it.toURI().toURL() }.toTypedArray() | ||
| URLClassLoader(urls, javaClass.classLoader).use { cl -> | ||
| // 2) Load the generated class + read static field | ||
| val clazz = Class.forName(generatedFile, true, cl) | ||
|
|
||
| @Suppress("UNCHECKED_CAST") | ||
| val supported: Set<String> = clazz.getField("SUPPORTED").get(null) as Set<String> |
There was a problem hiding this comment.
suggestion: Since clazz is used only once I suggest to close the use block there.
Suggested change
| val urls = main.runtimeClasspath.files.map { it.toURI().toURL() }.toTypedArray() | |
| URLClassLoader(urls, javaClass.classLoader).use { cl -> | |
| // 2) Load the generated class + read static field | |
| val clazz = Class.forName(generatedFile, true, cl) | |
| @Suppress("UNCHECKED_CAST") | |
| val supported: Set<String> = clazz.getField("SUPPORTED").get(null) as Set<String> | |
| val urls = mainSourceSetOutput.get().files.map { it.toURI().toURL() }.toTypedArray() | |
| val supported: Set<String> = URLClassLoader(urls, javaClass.classLoader).use { cl -> | |
| // 2) Load the generated class + read static field | |
| val clazz = Class.forName(generatedFile, true, cl) | |
| @Suppress("UNCHECKED_CAST") | |
| clazz.getField("SUPPORTED").get(null) as Set<String> | |
| } |
Note I'm referring to mainSourceSetOutput mentioned in the comment above.
There was a problem hiding this comment.
Another option would be to use the json conf instead of loading the generated class. This would allow to skip the generation and the compilation of the generated class ?
bric3
reviewed
Aug 28, 2025
| import java.io.PrintWriter | ||
| import javax.inject.Inject | ||
|
|
||
| abstract class ParseSupportedConfigurationsTask @Inject constructor( |
There was a problem hiding this comment.
suggestion: Maybe you can also add @CacheableTask
Suggested change
| abstract class ParseSupportedConfigurationsTask @Inject constructor( | |
| @CacheableTask | |
| abstract class ParseSupportedConfigurationsTask @Inject constructor( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What Does This Do
This PR implements basic Config Inversion, aiming to document all DD/OTEL environment variables used in the repo in a
supported-configurations.jsonfile.Components:
ParseSupportedConfigurations.java&GeneratedSupportedConfigurations.javasupported-configurations.jsonat build-time and stores the values in a static fileGeneratedSupportedConfigurations.javaConfigInversionStrictStyle.javaStrictwhich does not allow any usage of an unknown DD/OTEL environment variable,Warningwhich allows the usage but sends telemetry about unknown environment variables, andTestwhich allows the usage of unknown environment variables without sending telemetry.WarningConfigHelper.javasupported-configurations.jsonto ensure the environment variable queried for is "known"ConfigInversionStrictStylethat is set.System.getenv(String)andSystem.getenv()to the ForbiddenApis list to encourage the use of Environment component and ConfigHelperDD_FOO_BAR)EnvironmentVariables.get()andEnvironmentVariables.getAll(). This is to encourage the usage ofConfigHelperto enforce the documentation of unknown environment variables.Motivation
This PR supports the general Config Inversion theme that has already been implemented in dd-trace-js and currently being implemented in dd-trace-go and dd-trace-rb. Here is the RFC that documents what this project entails.
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]