Migrate Config system to environment component #9191
Merged
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
PerfectSlayer
added
the
tag: no release notes
This was referenced Jul 17, 2025
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 47 metrics, 12 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.51.1-SNAPSHOT~476cb5fd01, baseline=1.51.1-SNAPSHOT~938e591e2c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.051 s) : 0, 1050598
Total [baseline] (8.617 s) : 0, 8616646
Agent [candidate] (1.043 s) : 0, 1042645
Total [candidate] (8.617 s) : 0, 8617118
section iast
Agent [baseline] (1.172 s) : 0, 1171872
Total [baseline] (9.356 s) : 0, 9355918
Agent [candidate] (1.181 s) : 0, 1181108
Total [candidate] (9.398 s) : 0, 9398111
gantt
title insecure-bank - break down per module: candidate=1.51.1-SNAPSHOT~476cb5fd01, baseline=1.51.1-SNAPSHOT~938e591e2c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.438 ms) : 0, 1438
crashtracking [candidate] (1.427 ms) : 0, 1427
BytebuddyAgent [baseline] (735.023 ms) : 0, 735023
BytebuddyAgent [candidate] (729.102 ms) : 0, 729102
GlobalTracer [baseline] (242.832 ms) : 0, 242832
GlobalTracer [candidate] (241.423 ms) : 0, 241423
AppSec [baseline] (30.761 ms) : 0, 30761
AppSec [candidate] (30.305 ms) : 0, 30305
Debugger [baseline] (6.095 ms) : 0, 6095
Debugger [candidate] (6.011 ms) : 0, 6011
Remote Config [baseline] (663.294 µs) : 0, 663
Remote Config [candidate] (663.538 µs) : 0, 664
Telemetry [baseline] (12.725 ms) : 0, 12725
Telemetry [candidate] (12.763 ms) : 0, 12763
section iast
crashtracking [baseline] (1.424 ms) : 0, 1424
crashtracking [candidate] (1.451 ms) : 0, 1451
BytebuddyAgent [baseline] (846.343 ms) : 0, 846343
BytebuddyAgent [candidate] (852.536 ms) : 0, 852536
GlobalTracer [baseline] (231.124 ms) : 0, 231124
GlobalTracer [candidate] (232.273 ms) : 0, 232273
IAST [baseline] (31.369 ms) : 0, 31369
IAST [candidate] (30.193 ms) : 0, 30193
AppSec [baseline] (25.493 ms) : 0, 25493
AppSec [candidate] (27.873 ms) : 0, 27873
Debugger [baseline] (6.696 ms) : 0, 6696
Debugger [candidate] (6.834 ms) : 0, 6834
Remote Config [baseline] (586.968 µs) : 0, 587
Remote Config [candidate] (592.705 µs) : 0, 593
Telemetry [baseline] (8.012 ms) : 0, 8012
Telemetry [candidate] (8.211 ms) : 0, 8211
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.51.1-SNAPSHOT~476cb5fd01, baseline=1.51.1-SNAPSHOT~938e591e2c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.039 s) : 0, 1039134
Total [baseline] (10.822 s) : 0, 10821691
Agent [candidate] (1.052 s) : 0, 1051706
Total [candidate] (10.854 s) : 0, 10853521
section appsec
Agent [baseline] (1.215 s) : 0, 1215432
Total [baseline] (10.774 s) : 0, 10773931
Agent [candidate] (1.22 s) : 0, 1219928
Total [candidate] (10.808 s) : 0, 10808011
section iast
Agent [baseline] (1.175 s) : 0, 1175359
Total [baseline] (10.929 s) : 0, 10928983
Agent [candidate] (1.176 s) : 0, 1175723
Total [candidate] (10.902 s) : 0, 10901508
section profiling
Agent [baseline] (1.189 s) : 0, 1189215
Total [baseline] (10.864 s) : 0, 10864485
Agent [candidate] (1.206 s) : 0, 1205758
Total [candidate] (10.946 s) : 0, 10945861
gantt
title petclinic - break down per module: candidate=1.51.1-SNAPSHOT~476cb5fd01, baseline=1.51.1-SNAPSHOT~938e591e2c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.427 ms) : 0, 1427
crashtracking [candidate] (1.443 ms) : 0, 1443
BytebuddyAgent [baseline] (727.892 ms) : 0, 727892
BytebuddyAgent [candidate] (736.996 ms) : 0, 736996
GlobalTracer [baseline] (241.209 ms) : 0, 241209
GlobalTracer [candidate] (243.231 ms) : 0, 243231
AppSec [baseline] (30.33 ms) : 0, 30330
AppSec [candidate] (30.729 ms) : 0, 30729
Debugger [baseline] (6.032 ms) : 0, 6032
Debugger [candidate] (6.085 ms) : 0, 6085
Remote Config [baseline] (655.944 µs) : 0, 656
Remote Config [candidate] (654.752 µs) : 0, 655
Telemetry [baseline] (10.615 ms) : 0, 10615
Telemetry [candidate] (11.431 ms) : 0, 11431
section appsec
crashtracking [baseline] (1.44 ms) : 0, 1440
crashtracking [candidate] (1.426 ms) : 0, 1426
BytebuddyAgent [baseline] (749.137 ms) : 0, 749137
BytebuddyAgent [candidate] (753.054 ms) : 0, 753054
GlobalTracer [baseline] (234.418 ms) : 0, 234418
GlobalTracer [candidate] (234.677 ms) : 0, 234677
IAST [baseline] (23.545 ms) : 0, 23545
IAST [candidate] (23.605 ms) : 0, 23605
AppSec [baseline] (166.557 ms) : 0, 166557
AppSec [candidate] (168.385 ms) : 0, 168385
Debugger [baseline] (8.703 ms) : 0, 8703
Debugger [candidate] (6.509 ms) : 0, 6509
Remote Config [baseline] (614.712 µs) : 0, 615
Remote Config [candidate] (624.324 µs) : 0, 624
Telemetry [baseline] (9.774 ms) : 0, 9774
Telemetry [candidate] (10.544 ms) : 0, 10544
section iast
crashtracking [baseline] (1.462 ms) : 0, 1462
crashtracking [candidate] (1.429 ms) : 0, 1429
BytebuddyAgent [baseline] (848.52 ms) : 0, 848520
BytebuddyAgent [candidate] (848.107 ms) : 0, 848107
GlobalTracer [baseline] (231.6 ms) : 0, 231600
GlobalTracer [candidate] (232.773 ms) : 0, 232773
IAST [baseline] (30.738 ms) : 0, 30738
IAST [candidate] (27.602 ms) : 0, 27602
AppSec [baseline] (25.614 ms) : 0, 25614
AppSec [candidate] (29.179 ms) : 0, 29179
Debugger [baseline] (7.636 ms) : 0, 7636
Debugger [candidate] (6.712 ms) : 0, 6712
Remote Config [baseline] (596.084 µs) : 0, 596
Remote Config [candidate] (590.55 µs) : 0, 591
Telemetry [baseline] (8.13 ms) : 0, 8130
Telemetry [candidate] (8.249 ms) : 0, 8249
section profiling
crashtracking [baseline] (1.398 ms) : 0, 1398
crashtracking [candidate] (1.417 ms) : 0, 1417
BytebuddyAgent [baseline] (757.829 ms) : 0, 757829
BytebuddyAgent [candidate] (768.113 ms) : 0, 768113
GlobalTracer [baseline] (220.173 ms) : 0, 220173
GlobalTracer [candidate] (222.795 ms) : 0, 222795
AppSec [baseline] (30.096 ms) : 0, 30096
AppSec [candidate] (30.992 ms) : 0, 30992
Debugger [baseline] (6.23 ms) : 0, 6230
Debugger [candidate] (7.132 ms) : 0, 7132
Remote Config [baseline] (678.778 µs) : 0, 679
Remote Config [candidate] (685.138 µs) : 0, 685
Telemetry [baseline] (15.831 ms) : 0, 15831
Telemetry [candidate] (15.402 ms) : 0, 15402
ProfilingAgent [baseline] (107.841 ms) : 0, 107841
ProfilingAgent [candidate] (109.217 ms) : 0, 109217
Profiling [baseline] (108.464 ms) : 0, 108464
Profiling [candidate] (109.934 ms) : 0, 109934
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 4 performance regressions! Performance is the same for 6 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.51.1-SNAPSHOT~476cb5fd01, baseline=1.51.1-SNAPSHOT~76639fbbfd
dateFormat X
axisFormat %s
section baseline
no_agent (35.546 ms) : 35266, 35826
. : milestone, 35546,
appsec (46.821 ms) : 46414, 47229
. : milestone, 46821,
code_origins (45.269 ms) : 44872, 45667
. : milestone, 45269,
iast (44.138 ms) : 43758, 44518
. : milestone, 44138,
profiling (49.326 ms) : 48839, 49813
. : milestone, 49326,
tracing (44.24 ms) : 43873, 44606
. : milestone, 44240,
section candidate
no_agent (38.161 ms) : 37852, 38470
. : milestone, 38161,
appsec (46.623 ms) : 46207, 47038
. : milestone, 46623,
code_origins (46.211 ms) : 45802, 46619
. : milestone, 46211,
iast (46.29 ms) : 45898, 46683
. : milestone, 46290,
profiling (50.591 ms) : 50107, 51075
. : milestone, 50591,
tracing (42.425 ms) : 42062, 42787
. : milestone, 42425,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.51.1-SNAPSHOT~476cb5fd01, baseline=1.51.1-SNAPSHOT~76639fbbfd
dateFormat X
axisFormat %s
section baseline
no_agent (4.482 ms) : 4422, 4541
. : milestone, 4482,
iast (9.308 ms) : 9159, 9457
. : milestone, 9308,
iast_FULL (14.001 ms) : 13723, 14279
. : milestone, 14001,
iast_GLOBAL (10.042 ms) : 9862, 10222
. : milestone, 10042,
profiling (8.944 ms) : 8799, 9090
. : milestone, 8944,
tracing (7.592 ms) : 7480, 7703
. : milestone, 7592,
section candidate
no_agent (4.241 ms) : 4189, 4293
. : milestone, 4241,
iast (9.351 ms) : 9198, 9505
. : milestone, 9351,
iast_FULL (14.017 ms) : 13738, 14296
. : milestone, 14017,
iast_GLOBAL (9.852 ms) : 9682, 10023
. : milestone, 9852,
profiling (9.395 ms) : 9237, 9553
. : milestone, 9395,
tracing (8.152 ms) : 8022, 8281
. : milestone, 8152,
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.51.1-SNAPSHOT~476cb5fd01, baseline=1.51.1-SNAPSHOT~938e591e2c
dateFormat X
axisFormat %s
section baseline
no_agent (15.63 s) : 15630000, 15630000
. : milestone, 15630000,
appsec (14.899 s) : 14899000, 14899000
. : milestone, 14899000,
iast (18.709 s) : 18709000, 18709000
. : milestone, 18709000,
iast_GLOBAL (18.065 s) : 18065000, 18065000
. : milestone, 18065000,
profiling (15.714 s) : 15714000, 15714000
. : milestone, 15714000,
tracing (14.937 s) : 14937000, 14937000
. : milestone, 14937000,
section candidate
no_agent (14.921 s) : 14921000, 14921000
. : milestone, 14921000,
appsec (14.856 s) : 14856000, 14856000
. : milestone, 14856000,
iast (18.207 s) : 18207000, 18207000
. : milestone, 18207000,
iast_GLOBAL (17.823 s) : 17823000, 17823000
. : milestone, 17823000,
profiling (15.492 s) : 15492000, 15492000
. : milestone, 15492000,
tracing (15.025 s) : 15025000, 15025000
. : milestone, 15025000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.51.1-SNAPSHOT~476cb5fd01, baseline=1.51.1-SNAPSHOT~938e591e2c
dateFormat X
axisFormat %s
section baseline
no_agent (1.477 ms) : 1466, 1489
. : milestone, 1477,
appsec (3.58 ms) : 3367, 3792
. : milestone, 3580,
iast (2.2 ms) : 2137, 2263
. : milestone, 2200,
iast_GLOBAL (2.242 ms) : 2179, 2306
. : milestone, 2242,
profiling (2.067 ms) : 2015, 2119
. : milestone, 2067,
tracing (2.014 ms) : 1965, 2063
. : milestone, 2014,
section candidate
no_agent (1.48 ms) : 1469, 1492
. : milestone, 1480,
appsec (3.616 ms) : 3401, 3831
. : milestone, 3616,
iast (2.204 ms) : 2141, 2267
. : milestone, 2204,
iast_GLOBAL (2.257 ms) : 2193, 2321
. : milestone, 2257,
profiling (2.052 ms) : 2001, 2103
. : milestone, 2052,
tracing (2.016 ms) : 1967, 2065
. : milestone, 2016,
|
PerfectSlayer
force-pushed
the
bbujon/environment-step2
branch
from
78ad76f to
9d287cb
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
b6c331b to
2512a49
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step2
branch
from
9d287cb to
d374062
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
2512a49 to
fe762c4
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step2
branch
from
3cca925 to
8647cfd
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
fe762c4 to
41de4a2
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step2
branch
from
8647cfd to
e085472
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
41de4a2 to
42d109e
Compare
dd-java-agent/src/test/groovy/datadog/trace/agent/InitializationTelemetryTest.groovy
Outdated
Show resolved
Hide resolved
AlexeyKuznetsov-DD
requested changes
Jul 18, 2025
| @@ -205,7 +204,7 @@ private static Properties loadOtelConfigFile() { | |||
| String path = getProperty("otel.javaagent.configuration-file"); | |||
| if (null != path && !path.isEmpty()) { | |||
| if (path.charAt(0) == '~') { | |||
| path = System.getProperty("user.home") + path.substring(1); | |||
| path = SystemProperties.getOrDefault("user.home", "") + path.substring(1); | |||
There was a problem hiding this comment.
Just a minor question: why getOrDefault ? is it possible not to have user.home?
There was a problem hiding this comment.
If the security manager prevents access to this property, SystemProperties.get() will return null.
So using getOrDefault(key, '') should avoid NPE or "null" in path.
There was a problem hiding this comment.
Got it. But just curious, how previous code worked?
And would it make sense to build a path that will be not correct?
I have a feeling that we need method in SystemProperties class that will throw IllegalStateException in case when we must have a value.
Here instead of NPE we will introduce logic error.
There was a problem hiding this comment.
Got it. But just curious, how previous code worked?
I think the whole OTel config source init fails, then config init fails.
And would it make sense to build a path that will be not correct?
That would be a question for @mcculls 😉
I have a feeling that we need method in SystemProperties class that will throw IllegalStateException in case when we must have a value.
We can still check if a value is missing using null check and no default value. The question stays: how should we deal with this case then if we don't want to have a logic error?
There was a problem hiding this comment.
I finally went with a safer approach: only do the ~ substitution if env.home property is set and retrieved.
I applied the same logic to the ConfigProvider (around line 475)
There was a problem hiding this comment.
The OTel code doesn't handle this condition: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigurationFile.java#L57 - so we can choose.
TBH I doubt that anyone except maybe developers uses the ~ prefix for OTel configuration files, instead preferring the more stable absolute path. This scenario is specific to someone who wants to use an external config file, declares the path to that file using ~, and has a security manager installed that disallows system property access.
This is so unlikely I think using "" is ok as a fallback - but TBH I would also be fine with letting it throw an exception, because this falls into the category of an exceptional situation where it might be safer to not try to second guess...
There was a problem hiding this comment.
Got it. But just curious, how previous code worked?
I doubt the tracer would have got this far - if such a strict security manager was installed and the tracer hadn't been granted this permission, then it would have bailed out earlier.
There was a problem hiding this comment.
Should I revert to an IllegalStateException then?
There was a problem hiding this comment.
I'm fine with your current approach of just not doing the substitution
AlexeyKuznetsov-DD
requested changes
Jul 18, 2025
| @@ -205,7 +204,7 @@ private static Properties loadOtelConfigFile() { | |||
| String path = getProperty("otel.javaagent.configuration-file"); | |||
| if (null != path && !path.isEmpty()) { | |||
| if (path.charAt(0) == '~') { | |||
| path = System.getProperty("user.home") + path.substring(1); | |||
| path = SystemProperties.getOrDefault("user.home", "") + path.substring(1); | |||
There was a problem hiding this comment.
Got it. But just curious, how previous code worked?
And would it make sense to build a path that will be not correct?
I have a feeling that we need method in SystemProperties class that will throw IllegalStateException in case when we must have a value.
Here instead of NPE we will introduce logic error.
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
42d109e to
68fc6ca
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
2 times, most recently
from
e7cb973 to
a3d54e9
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
a3d54e9 to
9ae7f6a
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
9ae7f6a to
d61ab39
Compare
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
d61ab39 to
d83f869
Compare
mhlidd
approved these changes
Jul 28, 2025
There was a problem hiding this comment.
LGTM 🚀 ! I'm wondering if it's worthwhile to have a followup PR add a ForbiddenAPIs rule to prevent the usages of System.setProperty() and System.clearProperty() to encourage the usage of the Environment component. This is something I'm already doing for System.getenv() for Config Inversion :).
|
Yes, this is something we should be doing. |
I don't plan on doing it as a part of the Config Inversion effort as it only affects Environment Variables. I can make a separate PR to address System Properties once this is merged though! |
mcculls
reviewed
Jul 28, 2025
dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java
Outdated
Show resolved
Hide resolved
mcculls
approved these changes
Jul 28, 2025
PerfectSlayer
force-pushed
the
bbujon/environment-step3
branch
from
d83f869 to
476cb5f
Compare
AlexeyKuznetsov-DD
approved these changes
Jul 28, 2025
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 migrates the config system to the environment component.
It additionally improves the environment component with:
SystemProperties.clear()nullhandling onSystemPropertiesfor thesetandclearmethods.Motivation
Safely interact with the env var and system properties by handling security manager exception and possible NPE.
Additional Notes
Environment component related stacked PRs:
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: LANGPLAT-458