-
Notifications
You must be signed in to change notification settings - Fork 314
Add Docs on How to Add a Configuration #9835
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
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
|
🎯 Code Coverage 🔗 Commit SHA: 3266507 | Docs | Was this helpful? Give us feedback! |
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 55 metrics, 10 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.55.0-SNAPSHOT~3266507f6a, baseline=1.55.0-SNAPSHOT~f69e04325c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.033 s) : 0, 1032995
Total [baseline] (10.799 s) : 0, 10798981
Agent [candidate] (1.023 s) : 0, 1023478
Total [candidate] (10.695 s) : 0, 10695165
section appsec
Agent [baseline] (1.212 s) : 0, 1212069
Total [baseline] (10.931 s) : 0, 10931219
Agent [candidate] (1.196 s) : 0, 1196437
Total [candidate] (10.925 s) : 0, 10925185
section iast
Agent [baseline] (1.165 s) : 0, 1164697
Total [baseline] (11.145 s) : 0, 11145010
Agent [candidate] (1.159 s) : 0, 1158504
Total [candidate] (11.074 s) : 0, 11073677
section profiling
Agent [baseline] (1.181 s) : 0, 1181279
Total [baseline] (10.91 s) : 0, 10909520
Agent [candidate] (1.166 s) : 0, 1166382
Total [candidate] (10.903 s) : 0, 10903322
gantt
title petclinic - break down per module: candidate=1.55.0-SNAPSHOT~3266507f6a, baseline=1.55.0-SNAPSHOT~f69e04325c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.455 ms) : 0, 1455
crashtracking [candidate] (1.455 ms) : 0, 1455
BytebuddyAgent [baseline] (704.072 ms) : 0, 704072
BytebuddyAgent [candidate] (696.464 ms) : 0, 696464
GlobalTracer [baseline] (245.556 ms) : 0, 245556
GlobalTracer [candidate] (244.36 ms) : 0, 244360
AppSec [baseline] (32.361 ms) : 0, 32361
AppSec [candidate] (32.594 ms) : 0, 32594
Debugger [baseline] (6.371 ms) : 0, 6371
Debugger [candidate] (6.376 ms) : 0, 6376
Remote Config [baseline] (678.295 µs) : 0, 678
Remote Config [candidate] (674.446 µs) : 0, 674
Telemetry [baseline] (15.352 ms) : 0, 15352
Telemetry [candidate] (9.337 ms) : 0, 9337
Flare Poller [baseline] (5.932 ms) : 0, 5932
Flare Poller [candidate] (10.944 ms) : 0, 10944
section appsec
crashtracking [baseline] (1.475 ms) : 0, 1475
crashtracking [candidate] (1.457 ms) : 0, 1457
BytebuddyAgent [baseline] (733.196 ms) : 0, 733196
BytebuddyAgent [candidate] (718.982 ms) : 0, 718982
GlobalTracer [baseline] (237.061 ms) : 0, 237061
GlobalTracer [candidate] (235.341 ms) : 0, 235341
IAST [baseline] (25.31 ms) : 0, 25310
IAST [candidate] (24.882 ms) : 0, 24882
AppSec [baseline] (174.794 ms) : 0, 174794
AppSec [candidate] (175.254 ms) : 0, 175254
Debugger [baseline] (5.982 ms) : 0, 5982
Debugger [candidate] (6.165 ms) : 0, 6165
Remote Config [baseline] (631.283 µs) : 0, 631
Remote Config [candidate] (635.941 µs) : 0, 636
Telemetry [baseline] (8.519 ms) : 0, 8519
Telemetry [candidate] (8.608 ms) : 0, 8608
Flare Poller [baseline] (3.916 ms) : 0, 3916
Flare Poller [candidate] (3.935 ms) : 0, 3935
section iast
crashtracking [baseline] (1.48 ms) : 0, 1480
crashtracking [candidate] (1.475 ms) : 0, 1475
BytebuddyAgent [baseline] (826.273 ms) : 0, 826273
BytebuddyAgent [candidate] (821.483 ms) : 0, 821483
GlobalTracer [baseline] (234.344 ms) : 0, 234344
GlobalTracer [candidate] (232.458 ms) : 0, 232458
IAST [baseline] (30.096 ms) : 0, 30096
IAST [candidate] (26.774 ms) : 0, 26774
AppSec [baseline] (31.605 ms) : 0, 31605
AppSec [candidate] (35.111 ms) : 0, 35111
Debugger [baseline] (6.141 ms) : 0, 6141
Debugger [candidate] (6.141 ms) : 0, 6141
Remote Config [baseline] (602.666 µs) : 0, 603
Remote Config [candidate] (600.734 µs) : 0, 601
Telemetry [baseline] (8.471 ms) : 0, 8471
Telemetry [candidate] (8.646 ms) : 0, 8646
Flare Poller [baseline] (4.281 ms) : 0, 4281
Flare Poller [candidate] (4.238 ms) : 0, 4238
section profiling
crashtracking [baseline] (1.468 ms) : 0, 1468
crashtracking [candidate] (1.467 ms) : 0, 1467
BytebuddyAgent [baseline] (731.722 ms) : 0, 731722
BytebuddyAgent [candidate] (722.121 ms) : 0, 722121
GlobalTracer [baseline] (221.57 ms) : 0, 221570
GlobalTracer [candidate] (219.174 ms) : 0, 219174
AppSec [baseline] (32.517 ms) : 0, 32517
AppSec [candidate] (32.353 ms) : 0, 32353
Debugger [baseline] (12.462 ms) : 0, 12462
Debugger [candidate] (7.346 ms) : 0, 7346
Remote Config [baseline] (707.733 µs) : 0, 708
Remote Config [candidate] (690.36 µs) : 0, 690
Telemetry [baseline] (10.483 ms) : 0, 10483
Telemetry [candidate] (15.237 ms) : 0, 15237
Flare Poller [baseline] (4.167 ms) : 0, 4167
Flare Poller [candidate] (4.099 ms) : 0, 4099
ProfilingAgent [baseline] (110.233 ms) : 0, 110233
ProfilingAgent [candidate] (109.955 ms) : 0, 109955
Profiling [baseline] (110.913 ms) : 0, 110913
Profiling [candidate] (110.586 ms) : 0, 110586
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.55.0-SNAPSHOT~3266507f6a, baseline=1.55.0-SNAPSHOT~f69e04325c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.042 s) : 0, 1041655
Total [baseline] (8.671 s) : 0, 8671328
Agent [candidate] (1.025 s) : 0, 1025021
Total [candidate] (8.675 s) : 0, 8675095
section iast
Agent [baseline] (1.172 s) : 0, 1171637
Total [baseline] (9.41 s) : 0, 9410494
Agent [candidate] (1.153 s) : 0, 1153444
Total [candidate] (9.337 s) : 0, 9336990
gantt
title insecure-bank - break down per module: candidate=1.55.0-SNAPSHOT~3266507f6a, baseline=1.55.0-SNAPSHOT~f69e04325c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.494 ms) : 0, 1494
crashtracking [candidate] (1.45 ms) : 0, 1450
BytebuddyAgent [baseline] (710.333 ms) : 0, 710333
BytebuddyAgent [candidate] (698.603 ms) : 0, 698603
GlobalTracer [baseline] (247.355 ms) : 0, 247355
GlobalTracer [candidate] (245.018 ms) : 0, 245018
AppSec [baseline] (32.762 ms) : 0, 32762
AppSec [candidate] (32.761 ms) : 0, 32761
Debugger [baseline] (6.504 ms) : 0, 6504
Debugger [candidate] (6.381 ms) : 0, 6381
Remote Config [baseline] (692.22 µs) : 0, 692
Remote Config [candidate] (680.585 µs) : 0, 681
Telemetry [baseline] (14.408 ms) : 0, 14408
Telemetry [candidate] (9.433 ms) : 0, 9433
Flare Poller [baseline] (6.621 ms) : 0, 6621
Flare Poller [candidate] (9.511 ms) : 0, 9511
section iast
crashtracking [baseline] (1.468 ms) : 0, 1468
crashtracking [candidate] (1.481 ms) : 0, 1481
BytebuddyAgent [baseline] (830.494 ms) : 0, 830494
BytebuddyAgent [candidate] (816.045 ms) : 0, 816045
GlobalTracer [baseline] (235.715 ms) : 0, 235715
GlobalTracer [candidate] (232.125 ms) : 0, 232125
IAST [baseline] (32.354 ms) : 0, 32354
IAST [candidate] (26.88 ms) : 0, 26880
AppSec [baseline] (30.48 ms) : 0, 30480
AppSec [candidate] (35.567 ms) : 0, 35567
Debugger [baseline] (6.262 ms) : 0, 6262
Debugger [candidate] (6.203 ms) : 0, 6203
Remote Config [baseline] (647.601 µs) : 0, 648
Remote Config [candidate] (611.938 µs) : 0, 612
Telemetry [baseline] (8.643 ms) : 0, 8643
Telemetry [candidate] (8.774 ms) : 0, 8774
Flare Poller [baseline] (4.229 ms) : 0, 4229
Flare Poller [candidate] (4.327 ms) : 0, 4327
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 3 performance regressions! Performance is the same for 8 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~3266507f6a, baseline=1.55.0-SNAPSHOT~f69e04325c
dateFormat X
axisFormat %s
section baseline
no_agent (4.258 ms) : 4205, 4311
. : milestone, 4258,
iast (9.794 ms) : 9630, 9958
. : milestone, 9794,
iast_FULL (13.836 ms) : 13563, 14109
. : milestone, 13836,
iast_GLOBAL (10.771 ms) : 10581, 10961
. : milestone, 10771,
profiling (9.0 ms) : 8859, 9140
. : milestone, 9000,
tracing (7.774 ms) : 7663, 7886
. : milestone, 7774,
section candidate
no_agent (4.3 ms) : 4252, 4349
. : milestone, 4300,
iast (9.902 ms) : 9735, 10069
. : milestone, 9902,
iast_FULL (14.791 ms) : 14493, 15089
. : milestone, 14791,
iast_GLOBAL (10.965 ms) : 10768, 11162
. : milestone, 10965,
profiling (8.794 ms) : 8649, 8939
. : milestone, 8794,
tracing (8.089 ms) : 7971, 8206
. : milestone, 8089,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~3266507f6a, baseline=1.55.0-SNAPSHOT~f69e04325c
dateFormat X
axisFormat %s
section baseline
no_agent (37.44 ms) : 37134, 37745
. : milestone, 37440,
appsec (48.637 ms) : 48210, 49064
. : milestone, 48637,
code_origins (43.325 ms) : 42941, 43709
. : milestone, 43325,
iast (44.298 ms) : 43912, 44685
. : milestone, 44298,
profiling (49.075 ms) : 48627, 49523
. : milestone, 49075,
tracing (44.668 ms) : 44297, 45039
. : milestone, 44668,
section candidate
no_agent (38.306 ms) : 37989, 38622
. : milestone, 38306,
appsec (47.352 ms) : 46928, 47776
. : milestone, 47352,
code_origins (43.511 ms) : 43125, 43896
. : milestone, 43511,
iast (45.985 ms) : 45591, 46380
. : milestone, 45985,
profiling (46.344 ms) : 45929, 46758
. : milestone, 46344,
tracing (44.719 ms) : 44330, 45109
. : milestone, 44719,
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.55.0-SNAPSHOT~3266507f6a, baseline=1.55.0-SNAPSHOT~f69e04325c
dateFormat X
axisFormat %s
section baseline
no_agent (14.981 s) : 14981000, 14981000
. : milestone, 14981000,
appsec (14.521 s) : 14521000, 14521000
. : milestone, 14521000,
iast (18.521 s) : 18521000, 18521000
. : milestone, 18521000,
iast_GLOBAL (17.775 s) : 17775000, 17775000
. : milestone, 17775000,
profiling (15.485 s) : 15485000, 15485000
. : milestone, 15485000,
tracing (15.267 s) : 15267000, 15267000
. : milestone, 15267000,
section candidate
no_agent (14.96 s) : 14960000, 14960000
. : milestone, 14960000,
appsec (15.054 s) : 15054000, 15054000
. : milestone, 15054000,
iast (18.589 s) : 18589000, 18589000
. : milestone, 18589000,
iast_GLOBAL (18.101 s) : 18101000, 18101000
. : milestone, 18101000,
profiling (15.332 s) : 15332000, 15332000
. : milestone, 15332000,
tracing (15.261 s) : 15261000, 15261000
. : milestone, 15261000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.55.0-SNAPSHOT~3266507f6a, baseline=1.55.0-SNAPSHOT~f69e04325c
dateFormat X
axisFormat %s
section baseline
no_agent (1.476 ms) : 1465, 1488
. : milestone, 1476,
appsec (3.68 ms) : 3466, 3894
. : milestone, 3680,
iast (2.214 ms) : 2151, 2278
. : milestone, 2214,
iast_GLOBAL (2.259 ms) : 2194, 2323
. : milestone, 2259,
profiling (2.083 ms) : 2029, 2136
. : milestone, 2083,
tracing (2.05 ms) : 1999, 2100
. : milestone, 2050,
section candidate
no_agent (1.473 ms) : 1462, 1485
. : milestone, 1473,
appsec (3.651 ms) : 3439, 3863
. : milestone, 3651,
iast (2.214 ms) : 2151, 2278
. : milestone, 2214,
iast_GLOBAL (2.247 ms) : 2183, 2311
. : milestone, 2247,
profiling (2.055 ms) : 2003, 2106
. : milestone, 2055,
tracing (2.036 ms) : 1987, 2086
. : milestone, 2036,
|
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.
🔍 nitpick: Config files are sometimes refer as file ("File.java") and sometimes as class ("File"). It could benefit from uniformity.
-- Have to take a break, will continue the review when I get back --
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.
👏 praise: Thanks for adding some doc about our config system!
❔ question: I could not find mention about the config_norm_rules.json. Should it be part of the doc too or does this part is supposed to go away?
docs/add_new_configurations.md
Outdated
| Configurations are separated into different files based on the product they are related to. e.g. `CiVisiblity` related configurations live in `CiVisibilityConfig`, `Tracer` related in `TracerConfig`, etc. | ||
| Default values for configurations live in `ConfigDefaults.java`. | ||
|
|
||
| Configuration values are read in `Config.java`, which utilizes helper methods in `ConfigProvider.java` to fetch the final value for a configuration after searching through all Configuration Sources and determining the final value based on Source priority. |
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.
🎯 suggestion: It could be nice to show a list of the various configuration sources and what their purpose are. Typically, the one from CI Vis is not trivial.
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.
@mtoffl01 and I discussed that we wait to add documentation for Sources until Stable/Declarative/Hands-Off Config has a finalized term to be used publicly.
Documentation about sources will likely be a new README that is linked from this page.
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.
I think it's alright to document this now in dd-trace-java docs; in our conversations, I meant that public docs (docs.datadoghq.com) must wait.
The name for this file-based config stuff is definitely, officially Declarative Config 😁 .
Happy to help craft the sources doc if need be.
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.
What could actually be good is if we mention the sources here, and link to the public docs with more info. I don't think devs generally need to be super concerned about the sources when adding new configurations, but a link could always be good to provide more info. WDYT?
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.
All suggestions, no blockers. Thanks for doing this!
docs/add_new_configurations.md
Outdated
| Configurations are separated into different files based on the product they are related to. e.g. `CiVisiblity` related configurations live in `CiVisibilityConfig`, `Tracer` related in `TracerConfig`, etc. | ||
| Default values for configurations live in `ConfigDefaults.java`. | ||
|
|
||
| Configuration values are read in `Config.java`, which utilizes helper methods in `ConfigProvider.java` to fetch the final value for a configuration after searching through all Configuration Sources and determining the final value based on Source priority. |
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.
I think it's alright to document this now in dd-trace-java docs; in our conversations, I meant that public docs (docs.datadoghq.com) must wait.
The name for this file-based config stuff is definitely, officially Declarative Config 😁 .
Happy to help craft the sources doc if need be.
| } | ||
| } | ||
| ``` | ||
| 8. If the configuration is unique to all tracing libraries, add it to the [Feature Parity Dashboard](https://feature-parity.us1.prod.dog/#/configurations?viewType=configurations). This ensures that we have good documentation of all configurations supported in the library. |
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.
| 8. If the configuration is unique to all tracing libraries, add it to the [Feature Parity Dashboard](https://feature-parity.us1.prod.dog/#/configurations?viewType=configurations). This ensures that we have good documentation of all configurations supported in the library. | |
| 8. If the configuration is unique to this library, add it to the [Feature Parity Dashboard](https://feature-parity.us1.prod.dog/#/configurations?viewType=configurations). This ensures that we have good documentation of all configurations supported in the library. |
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.
This was intentional. If we are adding a config to Java that exists in other libraries, we expect it to already be documented in the FPD
|
|
||
| ## Verifying the Configuration | ||
|
|
||
| To verify a configuration has been correctly added, developers can modify existing test cases in `ConfigTest.groovy` to set the value of the configuration with various sources and confirm the internal value is correctly set in `Config.java`. |
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.
Would be sick to have a new test populate for the new config automatically... maybe someday. 😄
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 sure that "automated generated tests" will have real value if it's only testing "getter" behavior to comply with the code coverage policy.
To me, it make sense to write tests when there is a specific logic at play, like this will be enabled if both X and Y are enable and Z contains xxx.
On thing I would like at mid term is to have:
- Config declaration per product -- like we have today TracerConfig, CiVisibilityConfig
- Such config would declare keys, aliases, type (and parser if needed)
- This config declaration would be used to generate the
Configclass(es).
It does not have to be a god class with all config from it, it can be split by products too.
But this would get rid of the contructor / getter / toString / testing boilerplate...
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.
Sounds like this could be something Config Inversion/Registry could be very useful for with our new v2 format. :)
What Does This Do
There is currently no documentation in the tracer on how to add a new configuration in
dd-trace-java. With the addition of Config Inversion, docs are necessary to describe how to properly create new configurations in the tracer.Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]