-
Notifications
You must be signed in to change notification settings - Fork 311
Migrate internal apis to environment component #9291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
return Stream.of("TMP", "TEMP", "USERPROFILE") | ||
.map(EnvironmentVariables::get) | ||
.filter(Objects::nonNull) | ||
.filter(((Predicate<String>) String::isEmpty).negate()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 notes: This behavior change (empty --> !empty) is expected. It’s a fix to the PidHelper
on Windows platform and was confirmed with the author first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add some comment about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarification, what kind of comment do you expect here?
One of the next step for the environment component will be to handle the "temp dir" computation.
So feedback about it would be useful for this part too 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment that will explain this non obvious empty --> !empty
on Windows.
Code coverage: total 57.22%, base diff 0.09%, patch 36.00% (view details) This comment will be updated automatically if new data arrives.🔗 Commit SHA: 29fea0d | Docs | Was this helpful? Give us feedback! |
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.331 µs) : 276, 391
. : milestone, 333,
basic (279.563 µs) : 273, 286
. : milestone, 280,
loop (8.969 ms) : 8964, 8975
. : milestone, 8969,
section candidate
noprobe (314.144 µs) : 288, 340
. : milestone, 314,
basic (277.345 µs) : 271, 284
. : milestone, 277,
loop (8.976 ms) : 8971, 8982
. : milestone, 8976,
|
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 11 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.51.1-SNAPSHOT~2f67106fb9, baseline=1.51.1-SNAPSHOT~023e5251a6
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.051 s) : 0, 1050842
Total [baseline] (10.741 s) : 0, 10740941
Agent [candidate] (1.05 s) : 0, 1049681
Total [candidate] (10.777 s) : 0, 10777472
section appsec
Agent [baseline] (1.23 s) : 0, 1229542
Total [baseline] (10.788 s) : 0, 10787583
Agent [candidate] (1.219 s) : 0, 1219245
Total [candidate] (10.802 s) : 0, 10802252
section iast
Agent [baseline] (1.171 s) : 0, 1170536
Total [baseline] (10.873 s) : 0, 10873419
Agent [candidate] (1.173 s) : 0, 1173117
Total [candidate] (10.861 s) : 0, 10861292
section profiling
Agent [baseline] (1.198 s) : 0, 1197770
Total [baseline] (10.963 s) : 0, 10962949
Agent [candidate] (1.21 s) : 0, 1210381
Total [candidate] (10.876 s) : 0, 10875553
gantt
title petclinic - break down per module: candidate=1.51.1-SNAPSHOT~2f67106fb9, baseline=1.51.1-SNAPSHOT~023e5251a6
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.45 ms) : 0, 1450
crashtracking [candidate] (1.442 ms) : 0, 1442
BytebuddyAgent [baseline] (736.456 ms) : 0, 736456
BytebuddyAgent [candidate] (736.469 ms) : 0, 736469
GlobalTracer [baseline] (243.517 ms) : 0, 243517
GlobalTracer [candidate] (242.917 ms) : 0, 242917
AppSec [baseline] (30.32 ms) : 0, 30320
AppSec [candidate] (30.285 ms) : 0, 30285
Debugger [baseline] (6.088 ms) : 0, 6088
Debugger [candidate] (6.106 ms) : 0, 6106
Remote Config [baseline] (643.123 µs) : 0, 643
Remote Config [candidate] (653.781 µs) : 0, 654
Telemetry [baseline] (10.581 ms) : 0, 10581
Telemetry [candidate] (9.92 ms) : 0, 9920
section appsec
crashtracking [baseline] (1.441 ms) : 0, 1441
crashtracking [candidate] (1.434 ms) : 0, 1434
BytebuddyAgent [baseline] (759.744 ms) : 0, 759744
BytebuddyAgent [candidate] (753.087 ms) : 0, 753087
GlobalTracer [baseline] (236.849 ms) : 0, 236849
GlobalTracer [candidate] (234.715 ms) : 0, 234715
IAST [baseline] (23.872 ms) : 0, 23872
IAST [candidate] (23.531 ms) : 0, 23531
AppSec [baseline] (169.627 ms) : 0, 169627
AppSec [candidate] (168.661 ms) : 0, 168661
Debugger [baseline] (7.906 ms) : 0, 7906
Debugger [candidate] (7.888 ms) : 0, 7888
Remote Config [baseline] (619.119 µs) : 0, 619
Remote Config [candidate] (615.742 µs) : 0, 616
Telemetry [baseline] (8.275 ms) : 0, 8275
Telemetry [candidate] (8.279 ms) : 0, 8279
section iast
crashtracking [baseline] (1.42 ms) : 0, 1420
crashtracking [candidate] (1.429 ms) : 0, 1429
BytebuddyAgent [baseline] (845.144 ms) : 0, 845144
BytebuddyAgent [candidate] (846.797 ms) : 0, 846797
GlobalTracer [baseline] (231.16 ms) : 0, 231160
GlobalTracer [candidate] (232.508 ms) : 0, 232508
IAST [baseline] (29.518 ms) : 0, 29518
IAST [candidate] (28.107 ms) : 0, 28107
AppSec [baseline] (27.889 ms) : 0, 27889
AppSec [candidate] (28.624 ms) : 0, 28624
Debugger [baseline] (5.751 ms) : 0, 5751
Debugger [candidate] (5.82 ms) : 0, 5820
Remote Config [baseline] (579.556 µs) : 0, 580
Remote Config [candidate] (604.096 µs) : 0, 604
Telemetry [baseline] (8.082 ms) : 0, 8082
Telemetry [candidate] (8.189 ms) : 0, 8189
section profiling
crashtracking [baseline] (1.406 ms) : 0, 1406
crashtracking [candidate] (1.422 ms) : 0, 1422
BytebuddyAgent [baseline] (762.948 ms) : 0, 762948
BytebuddyAgent [candidate] (770.978 ms) : 0, 770978
GlobalTracer [baseline] (222.546 ms) : 0, 222546
GlobalTracer [candidate] (224.264 ms) : 0, 224264
AppSec [baseline] (30.154 ms) : 0, 30154
AppSec [candidate] (30.669 ms) : 0, 30669
Debugger [baseline] (6.326 ms) : 0, 6326
Debugger [candidate] (6.415 ms) : 0, 6415
Remote Config [baseline] (727.194 µs) : 0, 727
Remote Config [candidate] (712.013 µs) : 0, 712
Telemetry [baseline] (15.776 ms) : 0, 15776
Telemetry [candidate] (15.414 ms) : 0, 15414
ProfilingAgent [baseline] (108.238 ms) : 0, 108238
ProfilingAgent [candidate] (110.386 ms) : 0, 110386
Profiling [baseline] (108.895 ms) : 0, 108895
Profiling [candidate] (111.033 ms) : 0, 111033
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.51.1-SNAPSHOT~2f67106fb9, baseline=1.51.1-SNAPSHOT~023e5251a6
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.05 s) : 0, 1050475
Total [baseline] (8.633 s) : 0, 8633355
Agent [candidate] (1.053 s) : 0, 1053084
Total [candidate] (8.626 s) : 0, 8625618
section iast
Agent [baseline] (1.17 s) : 0, 1169802
Total [baseline] (9.343 s) : 0, 9343127
Agent [candidate] (1.172 s) : 0, 1171921
Total [candidate] (9.333 s) : 0, 9332578
gantt
title insecure-bank - break down per module: candidate=1.51.1-SNAPSHOT~2f67106fb9, baseline=1.51.1-SNAPSHOT~023e5251a6
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.442 ms) : 0, 1442
crashtracking [candidate] (1.459 ms) : 0, 1459
BytebuddyAgent [baseline] (735.654 ms) : 0, 735654
BytebuddyAgent [candidate] (736.157 ms) : 0, 736157
GlobalTracer [baseline] (241.605 ms) : 0, 241605
GlobalTracer [candidate] (243.411 ms) : 0, 243411
AppSec [baseline] (30.075 ms) : 0, 30075
AppSec [candidate] (30.254 ms) : 0, 30254
Debugger [baseline] (6.067 ms) : 0, 6067
Debugger [candidate] (6.043 ms) : 0, 6043
Remote Config [baseline] (650.448 µs) : 0, 650
Remote Config [candidate] (657.043 µs) : 0, 657
Telemetry [baseline] (13.938 ms) : 0, 13938
Telemetry [candidate] (14.036 ms) : 0, 14036
section iast
crashtracking [baseline] (1.424 ms) : 0, 1424
crashtracking [candidate] (1.429 ms) : 0, 1429
BytebuddyAgent [baseline] (844.889 ms) : 0, 844889
BytebuddyAgent [candidate] (846.222 ms) : 0, 846222
GlobalTracer [baseline] (231.324 ms) : 0, 231324
GlobalTracer [candidate] (231.093 ms) : 0, 231093
AppSec [baseline] (26.885 ms) : 0, 26885
AppSec [candidate] (25.247 ms) : 0, 25247
Debugger [baseline] (6.715 ms) : 0, 6715
Debugger [candidate] (6.631 ms) : 0, 6631
Remote Config [baseline] (588.776 µs) : 0, 589
Remote Config [candidate] (602.735 µs) : 0, 603
Telemetry [baseline] (8.033 ms) : 0, 8033
Telemetry [candidate] (8.09 ms) : 0, 8090
IAST [baseline] (28.973 ms) : 0, 28973
IAST [candidate] (31.756 ms) : 0, 31756
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 1 performance regressions! Performance is the same for 9 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.51.1-SNAPSHOT~2f67106fb9, baseline=1.51.1-SNAPSHOT~023e5251a6
dateFormat X
axisFormat %s
section baseline
no_agent (37.047 ms) : 36752, 37341
. : milestone, 37047,
appsec (48.478 ms) : 48043, 48914
. : milestone, 48478,
code_origins (45.036 ms) : 44647, 45426
. : milestone, 45036,
iast (44.933 ms) : 44548, 45317
. : milestone, 44933,
profiling (46.934 ms) : 46500, 47368
. : milestone, 46934,
tracing (43.661 ms) : 43294, 44028
. : milestone, 43661,
section candidate
no_agent (36.9 ms) : 36601, 37199
. : milestone, 36900,
appsec (46.115 ms) : 45713, 46517
. : milestone, 46115,
code_origins (46.915 ms) : 46487, 47343
. : milestone, 46915,
iast (44.816 ms) : 44426, 45206
. : milestone, 44816,
profiling (48.137 ms) : 47658, 48615
. : milestone, 48137,
tracing (44.502 ms) : 44137, 44867
. : milestone, 44502,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.51.1-SNAPSHOT~2f67106fb9, baseline=1.51.1-SNAPSHOT~023e5251a6
dateFormat X
axisFormat %s
section baseline
no_agent (4.328 ms) : 4280, 4376
. : milestone, 4328,
iast (9.31 ms) : 9156, 9465
. : milestone, 9310,
iast_FULL (14.207 ms) : 13925, 14490
. : milestone, 14207,
iast_GLOBAL (10.339 ms) : 10157, 10520
. : milestone, 10339,
profiling (9.016 ms) : 8880, 9152
. : milestone, 9016,
tracing (7.785 ms) : 7666, 7903
. : milestone, 7785,
section candidate
no_agent (4.37 ms) : 4322, 4419
. : milestone, 4370,
iast (9.454 ms) : 9297, 9612
. : milestone, 9454,
iast_FULL (14.42 ms) : 14131, 14709
. : milestone, 14420,
iast_GLOBAL (10.477 ms) : 10294, 10660
. : milestone, 10477,
profiling (8.737 ms) : 8603, 8872
. : milestone, 8737,
tracing (7.444 ms) : 7334, 7555
. : milestone, 7444,
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~2f67106fb9, baseline=1.51.1-SNAPSHOT~023e5251a6
dateFormat X
axisFormat %s
section baseline
no_agent (15.373 s) : 15373000, 15373000
. : milestone, 15373000,
appsec (14.815 s) : 14815000, 14815000
. : milestone, 14815000,
iast (18.498 s) : 18498000, 18498000
. : milestone, 18498000,
iast_GLOBAL (17.738 s) : 17738000, 17738000
. : milestone, 17738000,
profiling (15.464 s) : 15464000, 15464000
. : milestone, 15464000,
tracing (15.04 s) : 15040000, 15040000
. : milestone, 15040000,
section candidate
no_agent (14.965 s) : 14965000, 14965000
. : milestone, 14965000,
appsec (14.748 s) : 14748000, 14748000
. : milestone, 14748000,
iast (18.301 s) : 18301000, 18301000
. : milestone, 18301000,
iast_GLOBAL (18.242 s) : 18242000, 18242000
. : milestone, 18242000,
profiling (15.519 s) : 15519000, 15519000
. : milestone, 15519000,
tracing (14.915 s) : 14915000, 14915000
. : milestone, 14915000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.51.1-SNAPSHOT~2f67106fb9, baseline=1.51.1-SNAPSHOT~023e5251a6
dateFormat X
axisFormat %s
section baseline
no_agent (1.475 ms) : 1464, 1487
. : milestone, 1475,
appsec (3.651 ms) : 3435, 3867
. : milestone, 3651,
iast (2.203 ms) : 2141, 2265
. : milestone, 2203,
iast_GLOBAL (2.239 ms) : 2176, 2301
. : milestone, 2239,
profiling (2.041 ms) : 1991, 2091
. : milestone, 2041,
tracing (2.018 ms) : 1970, 2066
. : milestone, 2018,
section candidate
no_agent (1.475 ms) : 1463, 1486
. : milestone, 1475,
appsec (3.592 ms) : 3379, 3805
. : milestone, 3592,
iast (2.189 ms) : 2127, 2251
. : milestone, 2189,
iast_GLOBAL (2.238 ms) : 2175, 2301
. : milestone, 2238,
profiling (2.04 ms) : 1990, 2090
. : milestone, 2040,
tracing (2.029 ms) : 1980, 2077
. : milestone, 2029,
|
d6e835b
to
c63c1f8
Compare
3d69117
to
2f67106
Compare
Stacked PR broke when rebasing from GH following the Gradle smoke test fix. |
public static CiEnvironment local() { | ||
Map<String, String> env; | ||
try { | ||
env = System.getenv(); | ||
} catch (SecurityException e) { | ||
env = Collections.emptyMap(); | ||
} | ||
return new CiEnvironmentImpl(env); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% but maybe this class can be refactored to use datadog.environment.EnvironmentVariables
instead of caching System.getenv()
in private map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, I did not expose the "get all" API in the environment components.
So I only added safe access from the code that use it.
Do you think it would be better to offer the "get all" API in SystemProperties
and EnvironmentVariable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be better to offer the "get all" API in SystemProperties and EnvironmentVariable?
I like this, since the same security exception could occur with get all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as we have special dedicated classes to work with env and props, better to have one to rule them all :)
Properties systemProperties = System.getProperties(); | ||
for (Map.Entry<Object, Object> e : systemProperties.entrySet()) { | ||
String propertyName = (String) e.getKey(); | ||
Object propertyValue = e.getValue(); | ||
if ((propertyName.startsWith(Config.PREFIX) | ||
|| propertyName.startsWith("datadog.slf4j.simpleLogger.defaultLogLevel")) | ||
&& propertyValue != null) { | ||
propagatedSystemProperties.put(propertyName, propertyValue.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, can it be refactored to use SystemProperties
?
if (systemProperties.containsKey( | ||
propertyNameToSystemPropertyName( | ||
CiVisibilityConfig.CIVISIBILITY_INJECTED_TRACER_VERSION))) { | ||
if (SystemProperties.get(propertyNameToSystemPropertyName(CIVISIBILITY_INJECTED_TRACER_VERSION)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this long SystemProperties.get(propertyNameToSystemPropertyName(..)
can be part of SystemProperties
with some shorter and more convenient method name?
Just thinking out loud, if we can improve readability here and similar places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would bring config concern to the environment component.
I would rather avoiding it to keep bot separated.
return Stream.of("TMP", "TEMP", "USERPROFILE") | ||
.map(EnvironmentVariables::get) | ||
.filter(Objects::nonNull) | ||
.filter(((Predicate<String>) String::isEmpty).negate()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add some comment about that?
c63c1f8
to
3925256
Compare
2f67106
to
29fea0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. FYI I will also migrate most of the calls to Environment
component to call ConfigHelper
when the Config Inversion PR is ready.
What Does This Do
This PR migrate internal-api module to the new environment component
Motivation
Additional Notes
Environment component related stacked PRs:
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: LANGPLAT-458