Refactors environment variables handling to make it testable. #9586
Conversation
AlexeyKuznetsov-DD
added
type: enhancement
AlexeyKuznetsov-DD
requested review from
shatzi,
daniel-mohedano,
PerfectSlayer,
sarahchen6,
a team,
nikita-tkachenko-datadog and
bric3
and removed request for
a team and
PerfectSlayer
AlexeyKuznetsov-DD
changed the title
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 7 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.54.0-SNAPSHOT~335f9f25a7, baseline=1.54.0-SNAPSHOT~050cad8b0e
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.019 s) : 0, 1019335
Total [baseline] (10.683 s) : 0, 10683080
Agent [candidate] (1.017 s) : 0, 1016569
Total [candidate] (10.679 s) : 0, 10678696
section appsec
Agent [baseline] (1.196 s) : 0, 1195986
Total [baseline] (11.026 s) : 0, 11026422
Agent [candidate] (1.189 s) : 0, 1188585
Total [candidate] (10.995 s) : 0, 10994778
section iast
Agent [baseline] (1.154 s) : 0, 1153572
Total [baseline] (10.939 s) : 0, 10939094
Agent [candidate] (1.153 s) : 0, 1152525
Total [candidate] (10.996 s) : 0, 10996178
section profiling
Agent [baseline] (1.159 s) : 0, 1159176
Total [baseline] (11.035 s) : 0, 11035020
Agent [candidate] (1.154 s) : 0, 1153554
Total [candidate] (11.063 s) : 0, 11062764
gantt
title petclinic - break down per module: candidate=1.54.0-SNAPSHOT~335f9f25a7, baseline=1.54.0-SNAPSHOT~050cad8b0e
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.495 ms) : 0, 1495
crashtracking [candidate] (1.48 ms) : 0, 1480
BytebuddyAgent [baseline] (698.853 ms) : 0, 698853
BytebuddyAgent [candidate] (695.598 ms) : 0, 695598
GlobalTracer [baseline] (249.972 ms) : 0, 249972
GlobalTracer [candidate] (250.487 ms) : 0, 250487
AppSec [baseline] (31.324 ms) : 0, 31324
AppSec [candidate] (31.538 ms) : 0, 31538
Debugger [baseline] (6.472 ms) : 0, 6472
Debugger [candidate] (6.449 ms) : 0, 6449
Remote Config [baseline] (685.149 µs) : 0, 685
Remote Config [candidate] (722.458 µs) : 0, 722
Telemetry [baseline] (9.038 ms) : 0, 9038
Telemetry [candidate] (9.062 ms) : 0, 9062
section appsec
crashtracking [baseline] (1.474 ms) : 0, 1474
crashtracking [candidate] (1.458 ms) : 0, 1458
BytebuddyAgent [baseline] (717.479 ms) : 0, 717479
BytebuddyAgent [candidate] (712.241 ms) : 0, 712241
GlobalTracer [baseline] (241.554 ms) : 0, 241554
GlobalTracer [candidate] (239.961 ms) : 0, 239961
AppSec [baseline] (172.389 ms) : 0, 172389
AppSec [candidate] (171.575 ms) : 0, 171575
Debugger [baseline] (6.842 ms) : 0, 6842
Debugger [candidate] (6.794 ms) : 0, 6794
Remote Config [baseline] (657.777 µs) : 0, 658
Remote Config [candidate] (645.609 µs) : 0, 646
Telemetry [baseline] (9.261 ms) : 0, 9261
Telemetry [candidate] (10.107 ms) : 0, 10107
IAST [baseline] (25.01 ms) : 0, 25010
IAST [candidate] (24.678 ms) : 0, 24678
section iast
crashtracking [baseline] (1.489 ms) : 0, 1489
crashtracking [candidate] (1.473 ms) : 0, 1473
BytebuddyAgent [baseline] (815.948 ms) : 0, 815948
BytebuddyAgent [candidate] (815.89 ms) : 0, 815890
GlobalTracer [baseline] (238.757 ms) : 0, 238757
GlobalTracer [candidate] (238.26 ms) : 0, 238260
AppSec [baseline] (34.155 ms) : 0, 34155
AppSec [candidate] (34.123 ms) : 0, 34123
Debugger [baseline] (6.073 ms) : 0, 6073
Debugger [candidate] (6.09 ms) : 0, 6090
Remote Config [baseline] (607.72 µs) : 0, 608
Remote Config [candidate] (596.403 µs) : 0, 596
Telemetry [baseline] (8.417 ms) : 0, 8417
Telemetry [candidate] (8.492 ms) : 0, 8492
IAST [baseline] (26.641 ms) : 0, 26641
IAST [candidate] (26.303 ms) : 0, 26303
section profiling
crashtracking [baseline] (1.456 ms) : 0, 1456
crashtracking [candidate] (1.446 ms) : 0, 1446
BytebuddyAgent [baseline] (723.97 ms) : 0, 723970
BytebuddyAgent [candidate] (720.366 ms) : 0, 720366
GlobalTracer [baseline] (225.027 ms) : 0, 225027
GlobalTracer [candidate] (223.24 ms) : 0, 223240
AppSec [baseline] (31.428 ms) : 0, 31428
AppSec [candidate] (31.291 ms) : 0, 31291
Debugger [baseline] (7.338 ms) : 0, 7338
Debugger [candidate] (6.528 ms) : 0, 6528
Remote Config [baseline] (1.402 ms) : 0, 1402
Remote Config [candidate] (712.038 µs) : 0, 712
Telemetry [baseline] (14.76 ms) : 0, 14760
Telemetry [candidate] (15.501 ms) : 0, 15501
ProfilingAgent [baseline] (101.002 ms) : 0, 101002
ProfilingAgent [candidate] (101.837 ms) : 0, 101837
Profiling [baseline] (101.589 ms) : 0, 101589
Profiling [candidate] (102.45 ms) : 0, 102450
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.54.0-SNAPSHOT~335f9f25a7, baseline=1.54.0-SNAPSHOT~050cad8b0e
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.02 s) : 0, 1020180
Total [baseline] (8.712 s) : 0, 8711925
Agent [candidate] (1.006 s) : 0, 1006037
Total [candidate] (8.663 s) : 0, 8662762
section iast
Agent [baseline] (1.153 s) : 0, 1153209
Total [baseline] (9.306 s) : 0, 9306338
Agent [candidate] (1.149 s) : 0, 1148727
Total [candidate] (9.287 s) : 0, 9286911
gantt
title insecure-bank - break down per module: candidate=1.54.0-SNAPSHOT~335f9f25a7, baseline=1.54.0-SNAPSHOT~050cad8b0e
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.485 ms) : 0, 1485
crashtracking [candidate] (1.46 ms) : 0, 1460
BytebuddyAgent [baseline] (698.541 ms) : 0, 698541
BytebuddyAgent [candidate] (688.505 ms) : 0, 688505
GlobalTracer [baseline] (250.812 ms) : 0, 250812
GlobalTracer [candidate] (248.245 ms) : 0, 248245
AppSec [baseline] (31.605 ms) : 0, 31605
AppSec [candidate] (30.882 ms) : 0, 30882
Debugger [baseline] (6.47 ms) : 0, 6470
Debugger [candidate] (6.349 ms) : 0, 6349
Remote Config [baseline] (716.391 µs) : 0, 716
Remote Config [candidate] (675.563 µs) : 0, 676
Telemetry [baseline] (9.18 ms) : 0, 9180
Telemetry [candidate] (8.871 ms) : 0, 8871
section iast
crashtracking [baseline] (1.474 ms) : 0, 1474
crashtracking [candidate] (1.473 ms) : 0, 1473
BytebuddyAgent [baseline] (816.378 ms) : 0, 816378
BytebuddyAgent [candidate] (814.125 ms) : 0, 814125
GlobalTracer [baseline] (238.773 ms) : 0, 238773
GlobalTracer [candidate] (237.155 ms) : 0, 237155
AppSec [baseline] (33.985 ms) : 0, 33985
AppSec [candidate] (33.836 ms) : 0, 33836
Debugger [baseline] (6.085 ms) : 0, 6085
Debugger [candidate] (6.07 ms) : 0, 6070
Remote Config [baseline] (602.636 µs) : 0, 603
Remote Config [candidate] (592.328 µs) : 0, 592
Telemetry [baseline] (8.3 ms) : 0, 8300
Telemetry [candidate] (8.423 ms) : 0, 8423
IAST [baseline] (26.325 ms) : 0, 26325
IAST [candidate] (25.893 ms) : 0, 25893
LoadParameters
See matching parameters
SummaryFound 3 performance improvements and 1 performance regressions! Performance is the same for 8 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~335f9f25a7, baseline=1.54.0-SNAPSHOT~050cad8b0e
dateFormat X
axisFormat %s
section baseline
no_agent (38.436 ms) : 38121, 38751
. : milestone, 38436,
appsec (49.896 ms) : 49448, 50345
. : milestone, 49896,
code_origins (44.428 ms) : 44028, 44828
. : milestone, 44428,
iast (44.465 ms) : 44087, 44843
. : milestone, 44465,
profiling (47.19 ms) : 46756, 47625
. : milestone, 47190,
tracing (44.682 ms) : 44293, 45072
. : milestone, 44682,
section candidate
no_agent (37.191 ms) : 36898, 37484
. : milestone, 37191,
appsec (47.31 ms) : 46898, 47721
. : milestone, 47310,
code_origins (44.018 ms) : 43633, 44402
. : milestone, 44018,
iast (44.727 ms) : 44343, 45111
. : milestone, 44727,
profiling (48.585 ms) : 48130, 49040
. : milestone, 48585,
tracing (43.828 ms) : 43469, 44187
. : milestone, 43828,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~335f9f25a7, baseline=1.54.0-SNAPSHOT~050cad8b0e
dateFormat X
axisFormat %s
section baseline
no_agent (4.305 ms) : 4257, 4353
. : milestone, 4305,
iast (9.654 ms) : 9494, 9814
. : milestone, 9654,
iast_FULL (14.224 ms) : 13944, 14504
. : milestone, 14224,
iast_GLOBAL (10.669 ms) : 10477, 10860
. : milestone, 10669,
profiling (8.749 ms) : 8615, 8882
. : milestone, 8749,
tracing (8.177 ms) : 8060, 8295
. : milestone, 8177,
section candidate
no_agent (4.36 ms) : 4311, 4409
. : milestone, 4360,
iast (9.876 ms) : 9712, 10041
. : milestone, 9876,
iast_FULL (13.887 ms) : 13615, 14160
. : milestone, 13887,
iast_GLOBAL (11.19 ms) : 10988, 11391
. : milestone, 11190,
profiling (8.798 ms) : 8653, 8942
. : milestone, 8798,
tracing (7.476 ms) : 7372, 7580
. : milestone, 7476,
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~335f9f25a7, baseline=1.54.0-SNAPSHOT~050cad8b0e
dateFormat X
axisFormat %s
section baseline
no_agent (1.489 ms) : 1477, 1501
. : milestone, 1489,
appsec (3.75 ms) : 3532, 3969
. : milestone, 3750,
iast (2.223 ms) : 2159, 2287
. : milestone, 2223,
iast_GLOBAL (2.265 ms) : 2201, 2330
. : milestone, 2265,
profiling (2.088 ms) : 2035, 2140
. : milestone, 2088,
tracing (2.051 ms) : 2001, 2101
. : milestone, 2051,
section candidate
no_agent (1.488 ms) : 1477, 1500
. : milestone, 1488,
appsec (3.747 ms) : 3528, 3965
. : milestone, 3747,
iast (2.23 ms) : 2166, 2294
. : milestone, 2230,
iast_GLOBAL (2.263 ms) : 2199, 2327
. : milestone, 2263,
profiling (2.093 ms) : 2040, 2146
. : milestone, 2093,
tracing (2.03 ms) : 1981, 2079
. : milestone, 2030,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~335f9f25a7, baseline=1.54.0-SNAPSHOT~050cad8b0e
dateFormat X
axisFormat %s
section baseline
no_agent (14.949 s) : 14949000, 14949000
. : milestone, 14949000,
appsec (15.253 s) : 15253000, 15253000
. : milestone, 15253000,
iast (18.763 s) : 18763000, 18763000
. : milestone, 18763000,
iast_GLOBAL (18.078 s) : 18078000, 18078000
. : milestone, 18078000,
profiling (15.635 s) : 15635000, 15635000
. : milestone, 15635000,
tracing (15.254 s) : 15254000, 15254000
. : milestone, 15254000,
section candidate
no_agent (15.317 s) : 15317000, 15317000
. : milestone, 15317000,
appsec (15.145 s) : 15145000, 15145000
. : milestone, 15145000,
iast (18.377 s) : 18377000, 18377000
. : milestone, 18377000,
iast_GLOBAL (18.218 s) : 18218000, 18218000
. : milestone, 18218000,
profiling (15.904 s) : 15904000, 15904000
. : milestone, 15904000,
tracing (15.463 s) : 15463000, 15463000
. : milestone, 15463000,
|
| def additionalIgnoredTags = CiVisibilityTestUtils.IGNORED_TAGS + ignoredTags | ||
|
|
||
| if (System.getenv().get("GENERATE_TEST_FIXTURES") != null) { | ||
| if (EnvironmentVariables.get("GENERATE_TEST_FIXTURES") != null) { |
There was a problem hiding this comment.
I've tested locally and after the changes the env var doesn't seem to be picked up. We use it locally to generate new test fixtures when they need to be updated, so it's not actually part of the testing process itself. I think it's safe to keep as it was
There was a problem hiding this comment.
While it might be safe to keep the old way, using EnvironmentVariables can help share example of how to access environment variable.
In my opinion if the env var is not used, then maybe the while check has to go ? But this is another PR.
There was a problem hiding this comment.
It is used, but only manually by us when developing locally. We just use as quality of life. Instead of, for example, commenting out the generation code and uncommenting it when we need to generate new test fixtures, we just do so through the environment variable, which is much more convenient
There was a problem hiding this comment.
@daniel-mohedano Actually there is a workaround for such use case, just set TEST_ENV_PROPAGATE_VARS= GENERATE_TEST_FIXTURES and it will be propagated to test. Would this work? I can document it as comment or some readme file. WDYT?
There was a problem hiding this comment.
That totally works too, yeah! If you could add a comment to document the behavior, that would be helpful too. Small sidenote also, while testing I've found that we probably also want to populate the vars on datadog.trace.test.util.DDSpecification#setup. In parameterized tests the cleanup method is called after each configuration is run, so the propagated vars are only available on the first run
There was a problem hiding this comment.
While it might be safe to keep the old way, using EnvironmentVariables can help share example of how to access environment variable.
The environment component was designed to be used in the agent / client lib, not for general purpose or testing. It's expected to use the default env var and JVM checks when using tests, not the environment component. For example, you're supposed to use the JUnit @EnabledOnJre() annotation rather than using the JavaVirtualMachine for testing conditions.
I might later evolve the component in a way that is not testing friendly but tailored for agent bootstrap for example. So please keep using the default testing framework capabilities and System for testing. That is why I did not port testing code in the first place.
| if (EnvironmentVariables.get("DD_CIVISIBILITY_SMOKETEST_DEBUG_PARENT") != null) { | ||
| arguments += "-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005" | ||
| } | ||
| if (System.getenv("DD_CIVISIBILITY_SMOKETEST_DEBUG_CHILD") != null) { | ||
| if (EnvironmentVariables.get("DD_CIVISIBILITY_SMOKETEST_DEBUG_CHILD") != null) { | ||
| argMap.put(CiVisibilityConfig.CIVISIBILITY_DEBUG_PORT, "5055") | ||
| } |
There was a problem hiding this comment.
Same rationale as above, not actually part of the testing process so probably safe to keep as is.
| def additionalReplacements = ["content.meta.['test.toolchain']": "$toolchain:$toolchainVersion"] | ||
|
|
||
| if (System.getenv().get("GENERATE_TEST_FIXTURES") != null) { | ||
| if (EnvironmentVariables.get("GENERATE_TEST_FIXTURES") != null) { |
There was a problem hiding this comment.
🎯 suggestion: I saw the component usage pushed to the testing code while it was mainly designed for the tracer.
How do you expect to reconcile this usage with framework like JUnit who won’t leverage it?
| } | ||
|
|
||
| // Make it accessible from tests. | ||
| public static volatile EnvironmentVariablesProvider provider = new EnvironmentVariablesProvider(); |
There was a problem hiding this comment.
❔ question: Would there be a performance impact introducing volatile field access before env var access?
There was a problem hiding this comment.
considering the number of calls, I don't think you will see a measurable effect on this (particularly on x86).
From JDK9 you could workaround the usage of volatile qualifier by using VarHandles
| public static Map<String, String> getAll() { | ||
| try { | ||
| return unmodifiableMap(new HashMap<>(System.getenv())); | ||
| return unmodifiableMap(new HashMap<>(provider.getAll())); |
There was a problem hiding this comment.
❔ question: I wonder if I still should make a snapshot if its fine returning the original map 🤔
There was a problem hiding this comment.
I, personally, love the unmodifiableMap to make sure that map would not be modified later.
No strong opinion here...
After some thoughts I think that better not to mix testing code and library... I will revert some changes. |
| System.getenv("TEST_ENV_PROPAGATE_VARS") | ||
| ?.split(',') | ||
| ?.each { envVar -> | ||
| provider[envVar] = System.getenv(envVar) | ||
| } |
There was a problem hiding this comment.
❓ For my understanding, if there are actual env vars that needed to be accessed along with "fake" ones, I would need to set the env vars in the value of TEST_ENV_PROPAGATE_VARS?
There was a problem hiding this comment.
@mhlidd Yes, if you really need some variables from system level, just put TEST_ENV_PROPAGATE_VARS=var1,var2,...,varN as SYSTEM env var.
But I believe that this should be very rare corner case, our tests should be isolated from real env as much as possible.
The main question is what are you trying to make "testable"? All environment variables that are used as a way to control the "Java Client Library" behavior should never be accessing directly the The remaining access to environment variables should be to control tests, usually related to environment or CI setup. I'm not sure about the need a programmatic way to set them as we are controlling their value through our test setup. Is there any other environment variable access I miss that must be testable? |
@PerfectSlayer That make sense, I will chat with @mhlidd and rework my PR. |
Adding on, but Config Inversion will only restrict the usages of |
mhlidd
requested review from
smola,
manuel-alvarez-alvarez and
mtoffl01
and removed request for
a team
mhlidd
force-pushed
the
alexeyk/refactored-env-vars-to-be-testable
branch
from
54f2e28 to
348f975
Compare
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 6 unstable metrics. See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (333.161 µs) : 273, 393
. : milestone, 333,
basic (277.913 µs) : 271, 285
. : milestone, 278,
loop (8.965 ms) : 8959, 8971
. : milestone, 8965,
section candidate
noprobe (316.262 µs) : 287, 346
. : milestone, 316,
basic (275.673 µs) : 269, 282
. : milestone, 276,
loop (8.965 ms) : 8961, 8970
. : milestone, 8965,
|
|
🎯 Code Coverage 🔗 Commit SHA: 335f9f2 | Docs | Was this helpful? Give us feedback! |
| def additionalIgnoredTags = CiVisibilityTestUtils.IGNORED_TAGS + ignoredTags | ||
|
|
||
| if (System.getenv().get("GENERATE_TEST_FIXTURES") != null) { | ||
| if (EnvironmentVariables.get("GENERATE_TEST_FIXTURES") != null) { |
There was a problem hiding this comment.
While it might be safe to keep the old way, using EnvironmentVariables can help share example of how to access environment variable.
The environment component was designed to be used in the agent / client lib, not for general purpose or testing. It's expected to use the default env var and JVM checks when using tests, not the environment component. For example, you're supposed to use the JUnit @EnabledOnJre() annotation rather than using the JavaVirtualMachine for testing conditions.
I might later evolve the component in a way that is not testing friendly but tailored for agent bootstrap for example. So please keep using the default testing framework capabilities and System for testing. That is why I did not port testing code in the first place.
| env.set("DD_GIT_COMMIT_SHA", "sha1"); | ||
| env.set("DD_GIT_REPOSITORY_URL", "http://github.com"); |
There was a problem hiding this comment.
🎯 suggestion: This is typically the kind of test that should be migrated to be tested using config mocked value, not environment variables.
What Does This Do
Refactors environment variables handling to make it testable.
Motivation
Relying directly on environment variables in tests is considered a bad practice.
@PerfectSlayer introduced a shared component,
EnvironmentVariables, which is already used across the codebase.This PR makes
EnvironmentVariablestestable by introducing a provider that allows configuring initial values for environment variables in tests.Additional Notes
In one case, I had to initialize environment variables in a static block, because the test expected instrumentation to be auto-enabled by a special variable before instrumentation was loaded.
If class loading order cannot be avoided, I added support for a special environment variable:
TEST_ENV_PROPAGATE_VARS— a comma-separated list of variables that should be propagated from the real environment into the test environment.This change also removed the need for the JUnit4
@RuleAPI and the legacysystem-ruleslibrary.