Implement Config Inversion with Default Strictness of Warning
#9539
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
mhlidd
force-pushed
the
mhlidd/config_inversion_base
branch
2 times, most recently
from
1a576ea to
512b2c6
Compare
BenchmarksStartupParameters
See matching parameters
SummaryFound 4 performance improvements and 12 performance regressions! Performance is the same for 32 metrics, 11 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.54.0-SNAPSHOT~89ee457a98, baseline=1.54.0-SNAPSHOT~3a389f573c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.022 s) : 0, 1021747
Total [baseline] (10.65 s) : 0, 10649722
Agent [candidate] (1.058 s) : 0, 1058230
Total [candidate] (10.726 s) : 0, 10725816
section appsec
Agent [baseline] (1.207 s) : 0, 1207053
Total [baseline] (11.059 s) : 0, 11058636
Agent [candidate] (1.234 s) : 0, 1234191
Total [candidate] (11.006 s) : 0, 11006487
section iast
Agent [baseline] (1.164 s) : 0, 1164440
Total [baseline] (11.031 s) : 0, 11031442
Agent [candidate] (1.194 s) : 0, 1193656
Total [candidate] (11.024 s) : 0, 11024232
section profiling
Agent [baseline] (1.167 s) : 0, 1166584
Total [baseline] (11.051 s) : 0, 11051225
Agent [candidate] (1.216 s) : 0, 1215782
Total [candidate] (10.973 s) : 0, 10973489
gantt
title petclinic - break down per module: candidate=1.54.0-SNAPSHOT~89ee457a98, baseline=1.54.0-SNAPSHOT~3a389f573c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.461 ms) : 0, 1461
crashtracking [candidate] (1.461 ms) : 0, 1461
BytebuddyAgent [baseline] (688.297 ms) : 0, 688297
BytebuddyAgent [candidate] (730.171 ms) : 0, 730171
GlobalTracer [baseline] (258.924 ms) : 0, 258924
GlobalTracer [candidate] (251.669 ms) : 0, 251669
AppSec [baseline] (31.646 ms) : 0, 31646
AppSec [candidate] (30.443 ms) : 0, 30443
Debugger [baseline] (6.356 ms) : 0, 6356
Debugger [candidate] (6.331 ms) : 0, 6331
Remote Config [baseline] (685.747 µs) : 0, 686
Remote Config [candidate] (697.578 µs) : 0, 698
Telemetry [baseline] (13.315 ms) : 0, 13315
Telemetry [candidate] (16.323 ms) : 0, 16323
section appsec
crashtracking [baseline] (1.484 ms) : 0, 1484
crashtracking [candidate] (1.451 ms) : 0, 1451
BytebuddyAgent [baseline] (716.979 ms) : 0, 716979
BytebuddyAgent [candidate] (755.897 ms) : 0, 755897
GlobalTracer [baseline] (253.118 ms) : 0, 253118
GlobalTracer [candidate] (245.301 ms) : 0, 245301
AppSec [baseline] (169.683 ms) : 0, 169683
AppSec [candidate] (171.372 ms) : 0, 171372
Debugger [baseline] (6.173 ms) : 0, 6173
Debugger [candidate] (5.997 ms) : 0, 5997
Remote Config [baseline] (633.855 µs) : 0, 634
Remote Config [candidate] (628.299 µs) : 0, 628
Telemetry [baseline] (12.484 ms) : 0, 12484
Telemetry [candidate] (8.533 ms) : 0, 8533
IAST [baseline] (25.196 ms) : 0, 25196
IAST [candidate] (23.763 ms) : 0, 23763
section iast
crashtracking [baseline] (1.47 ms) : 0, 1470
crashtracking [candidate] (1.458 ms) : 0, 1458
BytebuddyAgent [baseline] (814.294 ms) : 0, 814294
BytebuddyAgent [candidate] (854.438 ms) : 0, 854438
GlobalTracer [baseline] (252.115 ms) : 0, 252115
GlobalTracer [candidate] (244.856 ms) : 0, 244856
AppSec [baseline] (30.478 ms) : 0, 30478
AppSec [candidate] (28.795 ms) : 0, 28795
Debugger [baseline] (6.205 ms) : 0, 6205
Debugger [candidate] (6.068 ms) : 0, 6068
Remote Config [baseline] (610.816 µs) : 0, 611
Remote Config [candidate] (588.84 µs) : 0, 589
Telemetry [baseline] (8.357 ms) : 0, 8357
Telemetry [candidate] (8.301 ms) : 0, 8301
IAST [baseline] (29.664 ms) : 0, 29664
IAST [candidate] (28.064 ms) : 0, 28064
section profiling
crashtracking [baseline] (1.446 ms) : 0, 1446
crashtracking [candidate] (1.441 ms) : 0, 1441
BytebuddyAgent [baseline] (720.532 ms) : 0, 720532
BytebuddyAgent [candidate] (763.102 ms) : 0, 763102
GlobalTracer [baseline] (236.066 ms) : 0, 236066
GlobalTracer [candidate] (234.572 ms) : 0, 234572
AppSec [baseline] (31.283 ms) : 0, 31283
AppSec [candidate] (30.726 ms) : 0, 30726
Debugger [baseline] (6.489 ms) : 0, 6489
Debugger [candidate] (13.926 ms) : 0, 13926
Remote Config [baseline] (718.525 µs) : 0, 719
Remote Config [candidate] (727.421 µs) : 0, 727
Telemetry [baseline] (16.519 ms) : 0, 16519
Telemetry [candidate] (9.437 ms) : 0, 9437
ProfilingAgent [baseline] (102.212 ms) : 0, 102212
ProfilingAgent [candidate] (110.414 ms) : 0, 110414
Profiling [baseline] (102.807 ms) : 0, 102807
Profiling [candidate] (111.014 ms) : 0, 111014
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.54.0-SNAPSHOT~89ee457a98, baseline=1.54.0-SNAPSHOT~3a389f573c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.018 s) : 0, 1018264
Total [baseline] (8.637 s) : 0, 8637085
Agent [candidate] (1.061 s) : 0, 1061256
Total [candidate] (8.671 s) : 0, 8671278
section iast
Agent [baseline] (1.153 s) : 0, 1153149
Total [baseline] (9.349 s) : 0, 9349351
Agent [candidate] (1.206 s) : 0, 1205572
Total [candidate] (9.337 s) : 0, 9336748
gantt
title insecure-bank - break down per module: candidate=1.54.0-SNAPSHOT~89ee457a98, baseline=1.54.0-SNAPSHOT~3a389f573c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.464 ms) : 0, 1464
crashtracking [candidate] (1.468 ms) : 0, 1468
BytebuddyAgent [baseline] (685.953 ms) : 0, 685953
BytebuddyAgent [candidate] (733.308 ms) : 0, 733308
GlobalTracer [baseline] (257.883 ms) : 0, 257883
GlobalTracer [candidate] (252.131 ms) : 0, 252131
AppSec [baseline] (31.551 ms) : 0, 31551
AppSec [candidate] (30.665 ms) : 0, 30665
Debugger [baseline] (6.306 ms) : 0, 6306
Debugger [candidate] (6.378 ms) : 0, 6378
Remote Config [baseline] (681.995 µs) : 0, 682
Remote Config [candidate] (687.973 µs) : 0, 688
Telemetry [baseline] (13.464 ms) : 0, 13464
Telemetry [candidate] (15.639 ms) : 0, 15639
section iast
crashtracking [baseline] (1.463 ms) : 0, 1463
crashtracking [candidate] (1.474 ms) : 0, 1474
BytebuddyAgent [baseline] (808.159 ms) : 0, 808159
BytebuddyAgent [candidate] (865.263 ms) : 0, 865263
GlobalTracer [baseline] (248.81 ms) : 0, 248810
GlobalTracer [candidate] (246.805 ms) : 0, 246805
AppSec [baseline] (26.474 ms) : 0, 26474
AppSec [candidate] (29.195 ms) : 0, 29195
Debugger [baseline] (6.093 ms) : 0, 6093
Debugger [candidate] (6.115 ms) : 0, 6115
Remote Config [baseline] (608.406 µs) : 0, 608
Remote Config [candidate] (606.902 µs) : 0, 607
Telemetry [baseline] (8.06 ms) : 0, 8060
Telemetry [candidate] (8.328 ms) : 0, 8328
IAST [baseline] (32.547 ms) : 0, 32547
IAST [candidate] (26.637 ms) : 0, 26637
LoadParameters
See matching parameters
SummaryFound 3 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~89ee457a98, baseline=1.54.0-SNAPSHOT~3a389f573c
dateFormat X
axisFormat %s
section baseline
no_agent (4.384 ms) : 4335, 4432
. : milestone, 4384,
iast (9.592 ms) : 9433, 9752
. : milestone, 9592,
iast_FULL (14.988 ms) : 14688, 15289
. : milestone, 14988,
iast_GLOBAL (10.179 ms) : 10003, 10355
. : milestone, 10179,
profiling (9.054 ms) : 8913, 9194
. : milestone, 9054,
tracing (7.58 ms) : 7466, 7694
. : milestone, 7580,
section candidate
no_agent (4.326 ms) : 4277, 4374
. : milestone, 4326,
iast (9.296 ms) : 9141, 9450
. : milestone, 9296,
iast_FULL (14.074 ms) : 13792, 14355
. : milestone, 14074,
iast_GLOBAL (9.96 ms) : 9789, 10132
. : milestone, 9960,
profiling (8.72 ms) : 8584, 8856
. : milestone, 8720,
tracing (7.655 ms) : 7539, 7771
. : milestone, 7655,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~89ee457a98, baseline=1.54.0-SNAPSHOT~3a389f573c
dateFormat X
axisFormat %s
section baseline
no_agent (37.297 ms) : 36998, 37596
. : milestone, 37297,
appsec (48.467 ms) : 48042, 48893
. : milestone, 48467,
code_origins (46.308 ms) : 45900, 46716
. : milestone, 46308,
iast (46.017 ms) : 45613, 46422
. : milestone, 46017,
profiling (48.823 ms) : 48309, 49337
. : milestone, 48823,
tracing (44.61 ms) : 44230, 44989
. : milestone, 44610,
section candidate
no_agent (37.275 ms) : 36977, 37572
. : milestone, 37275,
appsec (47.161 ms) : 46748, 47573
. : milestone, 47161,
code_origins (45.561 ms) : 45171, 45952
. : milestone, 45561,
iast (44.137 ms) : 43754, 44520
. : milestone, 44137,
profiling (49.636 ms) : 49150, 50121
. : milestone, 49636,
tracing (44.501 ms) : 44120, 44882
. : milestone, 44501,
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 tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~89ee457a98, baseline=1.54.0-SNAPSHOT~3a389f573c
dateFormat X
axisFormat %s
section baseline
no_agent (1.475 ms) : 1463, 1486
. : milestone, 1475,
appsec (2.463 ms) : 2413, 2514
. : milestone, 2463,
iast (2.203 ms) : 2141, 2266
. : milestone, 2203,
iast_GLOBAL (2.255 ms) : 2192, 2318
. : milestone, 2255,
profiling (2.052 ms) : 2001, 2103
. : milestone, 2052,
tracing (2.028 ms) : 1978, 2077
. : milestone, 2028,
section candidate
no_agent (1.477 ms) : 1465, 1488
. : milestone, 1477,
appsec (3.66 ms) : 3445, 3875
. : milestone, 3660,
iast (2.203 ms) : 2141, 2266
. : milestone, 2203,
iast_GLOBAL (2.24 ms) : 2177, 2302
. : milestone, 2240,
profiling (2.086 ms) : 2033, 2139
. : milestone, 2086,
tracing (2.031 ms) : 1982, 2080
. : milestone, 2031,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~89ee457a98, baseline=1.54.0-SNAPSHOT~3a389f573c
dateFormat X
axisFormat %s
section baseline
no_agent (14.968 s) : 14968000, 14968000
. : milestone, 14968000,
appsec (14.844 s) : 14844000, 14844000
. : milestone, 14844000,
iast (18.524 s) : 18524000, 18524000
. : milestone, 18524000,
iast_GLOBAL (17.845 s) : 17845000, 17845000
. : milestone, 17845000,
profiling (15.449 s) : 15449000, 15449000
. : milestone, 15449000,
tracing (14.897 s) : 14897000, 14897000
. : milestone, 14897000,
section candidate
no_agent (15.085 s) : 15085000, 15085000
. : milestone, 15085000,
appsec (14.989 s) : 14989000, 14989000
. : milestone, 14989000,
iast (18.576 s) : 18576000, 18576000
. : milestone, 18576000,
iast_GLOBAL (18.148 s) : 18148000, 18148000
. : milestone, 18148000,
profiling (15.452 s) : 15452000, 15452000
. : milestone, 15452000,
tracing (14.879 s) : 14879000, 14879000
. : milestone, 14879000,
|
|
🎯 Code Coverage 🔗 Commit SHA: 89ee457 | Docs | Was this helpful? Give us feedback! |
mhlidd
changed the title
Warning
mhlidd
requested review from
AlexeyKuznetsov-DD and
mcculls
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
mhlidd
changed the base branch from
master
to
mhlidd/config_inversion_plugins
mhlidd
force-pushed
the
mhlidd/config_inversion_base
branch
from
aa18112 to
606dfd0
Compare
mhlidd
changed the base branch from
mhlidd/config_inversion_plugins
to
mhlidd/migrate_config-utils_tests
mhlidd
force-pushed
the
mhlidd/config_inversion_base
branch
from
606dfd0 to
7cea99b
Compare
There was a problem hiding this comment.
👏 praise: Thanks for splitting the PR related to your config changes, that really helps to review 👍
🎯 suggestion: I would recommend trying to decrease the overall (cognitive) complexity by refactoring using new few methods to de-duplicate code and give more meaning.
For example this should have its own function with a meaningful name:
if (key.startsWith("DD_")
|| key.startsWith("OTEL_")
|| configSource.getAliasMapping().containsKey(key))| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class ConfigHelper { |
There was a problem hiding this comment.
🎯 suggestion: Helper class usually are final and have a private constructor.
There was a problem hiding this comment.
I was also wondering if this could be made into a singleton as well... and if so there would be no reason to have static methods under it. Not sure if that is the right or wrong direction to go in
There was a problem hiding this comment.
Usually, an helper class does not have a state. So if you have a state but only one instance, that would be a singleton.
Here might be a big ambiguous because most of the state is immutable, right? It's more like a big static data table and calling the helper methods won't mutate it. But as you have the StrictStyle thing that can change the behavior, I would with a singleton.
Keep in mind helper class / methods should be easier to test than singleton that usually are painful to deal with on test scenario.
utils/config-utils/src/main/java/datadog/trace/config/inversion/ConfigInversionStrictStyle.java
Outdated
Show resolved
Hide resolved
utils/config-utils/src/main/java/datadog/trace/config/inversion/ConfigHelper.java
Outdated
Show resolved
Hide resolved
utils/config-utils/src/main/java/datadog/trace/config/inversion/ConfigHelper.java
Show resolved
Hide resolved
| configs.put(key, value); | ||
| // If this environment variable is the alias of another, and we haven't processed the | ||
| // original environment variable yet, handle it here. | ||
| } else if (configSource.getAliasMapping().containsKey(key) |
There was a problem hiding this comment.
🎯 suggestion: The lookup is done twice. There should be a way to cache it.
Edit: three time if you consider the get()
There was a problem hiding this comment.
In this case and the one below, would caching the value mainly be for readability and code cleanliness? The lookup itself should take constant time.
There was a problem hiding this comment.
The main concern would be performance. 3 constant time access is still worth than 1, especially since they are all done from the same method.
The readability is a bonus where it will provide a meaningful variable name and avoid calls like:
List<String> aliasList = configSource.getAliases().get(configSource.getAliasMapping().get(key));
For example, what I would write would be:
List<String> aliases = source.aliases.get(mapping);
configSource->source: yet another "config" does not bring any info from aConfigInversionclass in config package and config modulegetAliases()->aliases(or maybealiases()): there is no need to respect the bean convention if there is noset. Java records even get rid of thegetXxxconvention as they are immutableconfigSource.getAliasMapping().get(key)->mapping: introducing meaningful variable name would decrease cognitive load.
An ever better step would be to have an aliases(String key) method from the ConfigSource class.
Doing so, you don't expose internal collections but intent on the data they hold.
It then becomes:
List<String> aliases = source.aliases(mapping);
| + (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: Is it expected to log on System.err?
There was a problem hiding this comment.
I changed it to a log just now. (I remember there was something weird with logging when I first implemented but that might have been due to something separate.)
There was a problem hiding this comment.
Yes, there is issue with the initializing the logging framework too early. This will depend on the usage that's done of the config helper. It might break in bootstrap for example, while reporting to system.err won't be great once we bootstrap.
There might be some new design constraints here... 🤔
utils/config-utils/src/main/java/datadog/trace/config/inversion/ConfigHelper.java
Outdated
Show resolved
Hide resolved
utils/config-utils/src/main/java/datadog/trace/config/inversion/ConfigInversionStrictStyle.java
Outdated
Show resolved
Hide resolved
dougqh
reviewed
Sep 19, 2025
utils/config-utils/src/main/java/datadog/trace/config/inversion/ConfigHelper.java
Outdated
Show resolved
Hide resolved
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:
ConfigInversionStrictStyle.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.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]