chore(config): typed config foundation#1957
Conversation
Binary Size Analysis (Agent Data Plane)Baseline: 5364030 · Comparison: 52d4d61 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
There was a problem hiding this comment.
Pull request overview
This PR lays the groundwork for moving ADP from map-based config access to a strongly-typed config translation pipeline by introducing a typed target model (SalukiConfiguration), a generated compile-time witness (DatadogConfigWitness + drive), and an initial Datadog-to-Saluki translator. It also expands the Datadog config inventory and introduces a NO_SMOKE sentinel for keys that shouldn’t participate in the current smoke-test model.
Changes:
- Add new typed ADP configuration model crates (
agent-data-plane-config,agent-data-plane-config-system) and initial Datadog translator + Saluki-only seeding. - Generate a Datadog config witness trait + driver and reorganize generated output under
datadog_agent_config::generated. - Promote additional config keys into the overlay inventory, update docs, and add
NO_SMOKEhandling in config-testing generation.
Reviewed changes
Copilot reviewed 30 out of 35 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| rustfmt.toml | Ignore generated config dir |
| lib/datadog-agent/config/src/translate_error.rs | Add translator error type |
| lib/datadog-agent/config/src/lib.rs | Re-export generated witness APIs |
| lib/datadog-agent/config/src/generated/witness.rs | Generated witness + driver |
| lib/datadog-agent/config/src/generated/mod.rs | Generated module layout |
| lib/datadog-agent/config/src/generated/datadog_configuration.rs | Update generated typed config |
| lib/datadog-agent/config/src/generated/classifier_data.rs | Adjust imports for relocated module |
| lib/datadog-agent/config/src/classifier/mod.rs | Consume classifier data from generated module |
| lib/datadog-agent/config/schema/schema_overlay.yaml | Promote keys; add NO_SMOKE usage |
| lib/datadog-agent/config/Cargo.toml | Add snafu dependency |
| lib/datadog-agent/config/build/witness_gen.rs | New witness generator |
| lib/datadog-agent/config/build/datadog_config_gen.rs | Write to generated/; materialize sections |
| lib/datadog-agent/config/build/classifier_gen.rs | Write classifier data to generated/ |
| lib/datadog-agent/config/build.rs | Run witness generator after config gen |
| lib/datadog-agent/config-testing/src/config_registry/tag_filterlist.rs | Generated registry adds metric tag filterlist |
| lib/datadog-agent/config-testing/build/registry_gen.rs | Skip NO_SMOKE keys in smoke registry |
| lib/datadog-agent/config-overlay-model/src/smoke_test_support.rs | Add NO_SMOKE sentinel enum variant |
| lib/agent-data-plane-config/src/shared.rs | Add shared typed config model |
| lib/agent-data-plane-config/src/lib.rs | Define SalukiConfiguration root |
| lib/agent-data-plane-config/src/domains/traces.rs | Add traces domain model |
| lib/agent-data-plane-config/src/domains/otlp.rs | Add OTLP domain model |
| lib/agent-data-plane-config/src/domains/multi_region_failover.rs | Add MRF domain model |
| lib/agent-data-plane-config/src/domains/mod.rs | Wire domain configuration struct |
| lib/agent-data-plane-config/src/domains/dogstatsd.rs | Add DogStatsD domain model |
| lib/agent-data-plane-config/src/domains/checks.rs | Add checks domain model |
| lib/agent-data-plane-config/src/control.rs | Add control/orchestration model |
| lib/agent-data-plane-config/Cargo.toml | New model crate manifest |
| lib/agent-data-plane-config-system/src/translators/mod.rs | Translator trait + module wiring |
| lib/agent-data-plane-config-system/src/translators/datadog_translator.rs | Implement Datadog witness translator |
| lib/agent-data-plane-config-system/src/saluki_only.rs | Saluki-only source + seeding |
| lib/agent-data-plane-config-system/src/lib.rs | Public translate entrypoint + test |
| lib/agent-data-plane-config-system/Cargo.toml | New config-system crate manifest |
| docs/agent-data-plane/configuration/dogstatsd.md | Document newly inventoried keys |
| Cargo.toml | Add new workspace members/deps |
| Cargo.lock | Lockfile updates for new crates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8649764c8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
This comment has been minimized.
This comment has been minimized.
webern
force-pushed
the
m/pr2-model
branch
3 times, most recently
from
f2c19d4 to
8712577
Compare
webern
force-pushed
the
m/pr2-model
branch
6 times, most recently
from
55eedc5 to
5ca1e2d
Compare
|
I've spent many hours trying to preserve the separation of commits in this PR and it's just... a massive headache. So I'm going to squash it down to one commit so that future rebases and changes are less painful. Goodbye my beautiful commit history... |
| } | ||
|
|
||
| fn consume_dd_url(&mut self, value: String) { | ||
| self.config.shared.endpoints.dd_url = non_empty(value).filter(|url| url != DEFAULT_PRIMARY_ENDPOINT); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e88715d5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| dogstatsd_expiry_seconds: | ||
| support: full | ||
| pipelines: [dogstatsd] |
There was a problem hiding this comment.
Are these pipeline declarations and the differentiation between "shared" and "domains" config structs still worth the lift? IIRC this was originally to help the smoke tests determine if something was used, but seems like that job is being moved to the witness?
There was a problem hiding this comment.
Aaaactually, these are used primarily, and only, by the mechanism we use to warn on incompatible configs. And I believe they do nothing for support: full keys 🙄 . The thing that will become unused, as components cutover, is the used_by field that names structs that are directly deserializing from the GenericConfiguration map. So as we cut those over to typed config, the overlay will need a sentinel like MIGRATED_TO_CONFIG_SYSTEM (or whatever), during the transitional state. Then used_by and the smoke tests will be deleted, and alternate forms of config system testing will be introduced.
|
Is there a plan to compare the old vs new config structs to make sure type conversions etc are all handled equivalently? |
tobz
left a comment
tobz
left a comment
There was a problem hiding this comment.
Took another pass on this, and I'm still generally OK with this.
Given the size, I think we should bias towards getting it merged since we can incrementally tidy things up if we decide that we really want to make stylistic / ergonomics-related changes before starting the next "phase" of the migration.
Push a few more aspects of Go time.Duration parsing to the go-duration crate.
Foundation for strongly-typed ADP configuration. No binary behavior change; it sets up the base for later PRs that migrate from GenericConfiguration map access to typed SalukiConfiguration access. - adds the translation target: SalukiConfiguration - adds a compile-time auditable translation system: DatadogConfigWitness - translates DatadogConfiguration into SalukiConfiguration - discovers and promotes config keys that were missing from the inventory
|
/merge -m rebase |
|
View all feedbacks in Devflow UI.
The expected merge time in
mergequeue build completed successfully, but the github api returned an error while merging the pr DetailsError: PUT https://api.github.com/repos/DataDog/saluki/pulls/1957/merge: 405 Rebase merges are not allowed on this repository. [] (Request ID: E22A:F91B0:14373E1:46BB173:6A4F9B0F) FullStacktrace: |
|
Note: i tried to separate out a small commit for the go-time-duration change that is more generally applicable, but... the merge queue rejected rebase-merge (apparently only after testing it or something), so I just squash merged it after that. |
## Human Summary
Adds a typed configuration update subscription mechanism, known as
`Live<T>`.
The contract is to get a Live<T> from the type system like this:
```rust
let live = config_system.live(|c| &c.domains.dogstatsd.debug_log)
```
Then you put `live.changes()` in your tokio `select!`
```rust
_ = live.changed() => {
let metrics_stats_enabled = live.current().metrics_stats_enable;
self.metrics_stats_enabled = metrics_stats_enabled;
debug!(metrics_stats_enabled, "Updated DogStatsD metrics stats debug logging gate.");
```
## AI Summary
Adds the typed runtime configuration library and `Live<T>` view without
wiring it into the Agent Data Plane binary.
### Configuration system
The new `agent-data-plane-config-system` crate translates two
configuration sources into the ADP-native `SalukiConfiguration` model:
- Datadog configuration, translated through the generated configuration
witness
- Saluki-only configuration, which seeds fields not represented in the
Datadog schema
`ConfigurationSystem`:
- translates an initial `GenericConfiguration` into a complete typed
configuration
- subscribes to updates when the source map is dynamic
- coalesces bursts of updates before re-translating
- fails startup when the initial source cannot be translated
- keeps runtime updates durable by retaining defaults for invalid
translated values while applying other valid changes
- exposes the current configuration through a shared `ArcSwap`
### Live view of the configuration
`Live<T>` in `agent-data-plane-config` provides a typed view over a
projection of `SalukiConfiguration`:
- `Live::fixed` creates a value that never changes
- `Live::dynamic` tracks a shared configuration snapshot
- `current()` returns the latest projected value
- `Deref` reads the view's latest snapshot
- `changed()` waits for the projected value to change
- `project()` narrows a view to a child node or field
The view uses `ArcSwap` for consistent snapshots and a Tokio `watch`
channel for update notifications. A view only wakes its consumer when
its projected value changes.
This PR is library-only. It does not modify `bin/agent-data-plane`, wire
the configuration system into startup, add the `/config/internal`
endpoint, or add integration-test coverage. Those changes are deferred
to PR-3b, where environment handling and runtime wiring will be
addressed together.
## Change Type
- [x] Non-functional (chore, refactoring, docs)
## How did you test this PR?
- `make check-clippy`
- Unit tests in `agent-data-plane-config-system` cover startup
translation, translation errors, update convergence, projections,
field-level live views, fixed views, and startup values.
No integration test is included in this PR because the Agent Data Plane
wiring is deferred to PR-3b.
## References
- Depends on #1957
Human Summary
This PR serves as the foundation for strongly-typed configuration for ADP. It does not affect the binary, but sets up the base for future PRs that will migrate the binary from map access with
GenericConfigurationto typed access withSalukiConfiguration.This PR sets up:
SalukiConfigurationDatadogConfigWitnessDatadogConfigurationtoSalukiConfigurationThe shape of
SalukiConfigurationis likely to change as we progress.Challenges
One difficult aspect of this project is that the discovery and inventorying of all of the configs we are using messy. The full audit and shape of
SalukiConfigurationcan only be fully realized once it is being pushed down to components and usage sites, so consider this a sketch of its final form.The other big challenge is churn. It will be costly for me to keep it up-to-date as the codebase changes underneath it. I have written skills to help with this, but it is not easy. Once the system is complete and high-churn areas like DogStatsD are swung over to typed config, I don't think it will be hard for team members to use, but it is not intuitive in a transitory state. As such, I recommend we refrain from merging until enough of the PR stack is approved to swing us over to using this.
That being said, it's important to get early feedback before the stack gets too deep.
AI Summary
This PR adds the foundation for translating Datadog Agent configuration into a typed ADP
configuration model.
The change is additive: the existing runtime config path is not cut over in this PR.
What changed:
agent-data-plane-config, a source-agnostic data model centered onSalukiConfiguration { control, shared, domains }. The model derivesPartialEqto supportlater per-subtree change detection.
datadog-agent-config:DatadogConfigWitnessdrive(...), which reports every failed key, not just the firstTranslateError(one failed key) andTranslateErrors(the aggregatedrivereturns)agent-data-plane-config-system, with the initial Datadog translator(
DatadogTranslator::translate, an inherent method that returns the fully-populated model plusany translation errors).
SalukiOnly::seed.in the typed model.
NO_SMOKEconfig-testing sentinel for supported keys that do not yet fit the existingsmoke-test model.
datadog_agent_config::generatedso generated configmodules are grouped together.
Out of scope for this PR:
bin/agent-data-planeGenericConfigurationChange Type
How did you test this PR?
make check-allReferences