Skip to content

Commit 639bdc4

Browse files
authored
chore(config): preserve provenance in typed configuration (#2279)
## Human Summary This PR prepares us to properly close #1965. However we need to migrate the affected components to typed configuration to pick up the fix. #1965 will close when affected components migrate. It is a bit much to do in a single PR so I want to land the mechanism first. In this PR we preserve a binary interpretation of the `source` field in the remote Agent config stream. When that stream declares the `source` as `default` or `schema`, we record `Provenance::Default`, otherwise `Provenance::Explicit`. For algorithms such as overriding `site` with `dd_url`, this information is required because the Agent stream populates the fields with their default values and we need to know which one should take precedence. **Important**: without such guidance in my prompt, the coding agent decided that Agent defaults should **not** override explicit local settings (i.e. in the config file or environment variables that ADP reads during startup). This is a behavioral change and, to be honest, I am not sure what the right behavior is. This seems fine? But also, I could see us making the argument that the config stream is definitive even to the extent of overriding local values. In practice this should have little or no effect since we do not expect operators to configure ADP directly beyond the minimum configuration required to connect to the Agent. **Reviewer's Guide** - `lib/saluki-config/src/dynamic/event.rs` introduces types to carry provenance in the Agent updates. - `lib/agent-data-plane-config/src/provenance.rs` introduces types to carry provenance in typed config. - `bin/agent-data-plane/src/internal/remote_agent.rs` is where we preserve the information in the config stream. - `lib/agent-data-plane-config-system/src/translators/datadog_translator.rs` is where we preserve the information for those fields where we need it in typed config. Most of the code churn is in tests creating config stream events for themselves, which now have to add the provenance metadata. ## AI Summary The Core Agent configuration stream publishes a complete reified configuration, including values materialized from schema defaults, and source metadata for each key. Once that configuration is deserialized into a value-only model, an explicitly supplied value equal to the schema default is indistinguishable from a defaulted value. That distinction is required for source-aware translation and is the key to resolving #1965. This PR carries the two-state provenance (`Default`/`Explicit`) from the stream through the typed configuration system and exposes it to `SalukiConfiguration` consumers with `ConfigValue<T>`. Translators and consumers can now determine whether a setting was supplied without hardcoding or comparing schema-default values. Provenance is preserved across local bootstrap values, snapshots, partial updates, resets, and live changes while retaining the existing serialized configuration shape. As a side effect, default-sourced Agent values no longer shadow explicit local file or environment values in the typed path; explicit Agent values still override them. This is useful for local and test configuration, but is secondary to making source-aware configuration decisions possible. The legacy `GenericConfiguration` compatibility view remains value-only, and affected components still need to migrate before #1965 is fully resolved. ### Follow-up The components that need typed-config plumbing for the #1965 endpoint fix are below. This PR also carries provenance for the forwarder retry-queue settings; the retry-related rows are needed to consume that additional fix, but are not required for the `site`/`dd_url` endpoint fix itself. ``` ┌────────────────────────────────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────┐ │ Component │ Why it must migrate │ ├────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────┤ │ DatadogMetricsConfiguration / │ It currently deserializes `site` and `dd_url` from the raw map and decides which endpoint wins │ │ dd_metrics_encode │ in `configured_primary_endpoint` and `any_series_endpoint_matches`. It must use │ │ │ `Endpoints.dd_url.is_explicit()` and the typed site value rather than treating the default URL │ │ │ as absent. This also covers the MRF and autoscaling-failover metrics encoder instances. │ ├────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────┤ │ DatadogForwarderConfiguration / │ Its `EndpointConfiguration` currently independently deserializes `site` and `dd_url`. If only. │ │ dd_out (and `mrf_dd_out`) │ the metrics encoder migrates, the encoder and forwarder can disagree about the primary │ │ │ endpoint. │ ├────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────┤ │ ForwarderConfiguration │ This is the common adapter used by both forwarders. It needs to overlay the typed endpoint and │ │ │ retry values onto the remaining raw/deserialized forwarder settings. │ ├────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────┤ │ EndpointConfiguration │ This is where primary endpoint resolution currently happens. It needs to represent a defaulted │ │ │ `dd_url` as “not an override” and an explicit `dd_url`—even `https://app.datadoghq.com`—as an │ │ │ override. │ ├────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────┤ │ RetryConfiguration │ `queue_max_size_bytes()` currently uses `Option` presence. It needs to prefer │ │ │ `retry_queue_payloads_max_size` only when that value is explicit, then fall back to │ │ │ `retry_queue_max_size` only when that value is explicit. │ ├────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────┤ │ ClusterAgentForwarderConfiguration / │ Its destination URL is explicitly supplied programmatically, so it does not need global │ │ cluster_agent_out │ `site`/`dd_url`. However, it uses the shared `ForwarderConfiguration` and calls │ │ │ `queue_max_size_bytes()`, so it must receive the provenance-aware retry settings. │ └────────────────────────────────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────┘ ``` These are shared configuration adapters as well as components. The topology constructors in `bin/agent-data-plane/src/cli/run.rs` must pass the typed shared endpoint configuration through for the primary, MRF, and autoscaling-failover paths. The MRF and Cluster Agent endpoint overrides remain explicit programmatic overrides. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `cargo check --workspace --tests` - `cargo test -p agent-data-plane-config -p agent-data-plane-config-system -p saluki-config -p agent-data-plane` - `git diff --check` #### Human Note Not tested at the integration level yet because we need to migrate to typed config. But I recommend that we test at least one example of this behavior in the integration suite when we migrate to this mechanism. ## References - Progresses #1965 - Related to #2169 Co-authored-by: matt.briggs <matt.briggs@datadoghq.com>
1 parent 71a9d5c commit 639bdc4

23 files changed

Lines changed: 1558 additions & 634 deletions

File tree

.claude/skills/config-system/SKILL.md

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,10 @@ into `SalukiConfiguration`.
5656
| Figment provider wrapping that reader | `lib/datadog-agent/config/src/env_provider.rs` |
5757
| Same provider, plus Saluki-only keys | `lib/agent-data-plane-config-system/src/env_provider.rs` |
5858

59+
In `SalukiConfiguration`, you may use `ConfigValue<T>` when we need to know the difference between a
60+
value that was explicitly set by the user, or where the value is a default. The Agent API provides a
61+
`source` field which we are simplifying into `Provinence`, which is either `Default` or `Explicit`.
62+
5963
Paths and type names can move. Notify the user when this skill needs an update.
6064

6165
### Architectural dependency boundaries
@@ -109,10 +113,11 @@ generated tables instead:
109113
- the by-key path via `EnvironmentProvider`, a Figment provider wrapping those same readers.
110114

111115
**Every source therefore delivers the Agent's canonical shape.** A struct deserialized from
112-
`GenericConfiguration` **MUST** read that shape. Do not add a `#[serde(rename)]` or `#[serde(alias)]`
113-
that maps a nested key onto a flattened spelling, and do not reintroduce a key-alias or
114-
environment-remapping table. Those existed once, only ran on file load, and so never applied to the
115-
Datadog Agent's configuration stream — which is the authority in the Core Agent deployment.
116+
`GenericConfiguration` **MUST** read that shape. Do not add a `#[serde(rename)]` or
117+
`#[serde(alias)]` that maps a nested key onto a flattened spelling, and do not reintroduce a
118+
key-alias or environment-remapping table. Those existed once, only ran on file load, and so never
119+
applied to the Datadog Agent's configuration stream — which is the authority in the Core Agent
120+
deployment.
116121

117122
Reserve `#[serde(flatten)]` for a struct that genuinely groups several *top-level* Agent keys (for
118123
example, the forwarder's `forwarder_*` retry settings). Name a Rust field after its canonical
@@ -132,16 +137,17 @@ Keep the authoritative Datadog path and delete the duplicate Saluki-only path.
132137

133138
Exactly one layer owns each default:
134139

135-
| Source class | Model type | Default owner | Translation behavior |
136-
|--------------|-------------|------------------------------------------------|-----------------------------------|
137-
| Saluki-only | `Option<T>` | No default | `seed` preserves `None` |
138-
| Saluki-only | `T` | `agent-data-plane-config/src/defaults.rs` | `seed` assigns the resolved value |
139-
| Witnessed | `T` | Generated Datadog schema default | `drive` always writes it |
140-
| Witnessed | `Option<T>` | No default | `drive` preserves `None` |
140+
| Source class | Model type | Default owner | Translation behavior |
141+
|--------------|-------------|-------------------------------------------|-----------------------------------|
142+
| Saluki-only | `Option<T>` | No default | `seed` preserves `None` |
143+
| Saluki-only | `T` | `agent-data-plane-config/src/defaults.rs` | `seed` assigns the resolved value |
144+
| Witnessed | `T` | Generated Datadog schema default | `drive` always writes it |
145+
| Witnessed | `Option<T>` | No default | `drive` preserves `None` |
141146

142147
Define each Saluki-only default once in `lib/agent-data-plane-config/src/defaults.rs`; source and
143148
model defaults must reference that definition rather than restating its value. If the component
144-
requires a value, model `T`; use `Option<T>` only when absence is meaningful, not to defer a default.
149+
requires a value, model `T`; use `Option<T>` only when absence is meaningful, not to defer a
150+
default.
145151

146152
Push source parsing, defaults, and input validation to the configuration boundary. Components keep
147153
only validation that is truly business logic.

bin/agent-data-plane/src/components/dogstatsd_post_aggregate_filter/mod.rs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,10 @@ mod tests {
325325
use std::time::Duration;
326326

327327
use metrics::set_default_local_recorder;
328-
use saluki_config::{dynamic::ConfigUpdate, ConfigurationLoader};
328+
use saluki_config::{
329+
dynamic::{ConfigSetting, ConfigUpdate},
330+
ConfigurationLoader,
331+
};
329332
use saluki_context::Context;
330333
use saluki_core::{
331334
data_model::event::{metric::Metric, Event},
@@ -560,10 +563,7 @@ mod tests {
560563
async fn runtime_updates_rebuild_the_effective_matcher() {
561564
let (config, sender) = ConfigurationLoader::for_tests(Some(serde_json::json!({})), None, true).await;
562565
let sender = sender.expect("sender should exist");
563-
sender
564-
.send(ConfigUpdate::Snapshot(serde_json::json!({})))
565-
.await
566-
.unwrap();
566+
sender.send(ConfigUpdate::snapshot([])).await.unwrap();
567567
config.ready().await;
568568

569569
let mut filter = noop_filter(vec!["request.duration.max"], false, vec![], false);
@@ -574,10 +574,10 @@ mod tests {
574574

575575
let mut filterlist_watcher = config.watch_for_updates("metric_filterlist");
576576
sender
577-
.send(ConfigUpdate::Partial {
578-
key: "metric_filterlist".to_string(),
579-
value: serde_json::json!(["request.duration.avg"]),
580-
})
577+
.send(ConfigUpdate::Partial(ConfigSetting::explicit(
578+
"metric_filterlist",
579+
serde_json::json!(["request.duration.avg"]),
580+
)))
581581
.await
582582
.unwrap();
583583

bin/agent-data-plane/src/components/dogstatsd_prefix_filter/mod.rs

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,10 @@ impl Transform for DogStatsDPrefixFilter {
344344
#[cfg(test)]
345345
mod tests {
346346
use metrics::set_default_local_recorder;
347-
use saluki_config::{dynamic::ConfigUpdate, ConfigurationLoader};
347+
use saluki_config::{
348+
dynamic::{ConfigSetting, ConfigUpdate},
349+
ConfigurationLoader,
350+
};
348351
use saluki_metrics::{test::TestRecorder, MetricsBuilder};
349352

350353
use super::*;
@@ -499,10 +502,7 @@ mod tests {
499502
async fn blocklist_and_filterlist_dynamic_updates_are_applied() {
500503
let (cfg, sender) = ConfigurationLoader::for_tests(Some(serde_json::json!({})), None, true).await;
501504
let sender = sender.expect("sender should exist");
502-
sender
503-
.send(ConfigUpdate::Snapshot(serde_json::json!({})))
504-
.await
505-
.unwrap();
505+
sender.send(ConfigUpdate::snapshot([])).await.unwrap();
506506

507507
cfg.ready().await;
508508

@@ -527,10 +527,10 @@ mod tests {
527527
let mut blocklist_watcher = cfg.watch_for_updates("statsd_metric_blocklist");
528528

529529
sender
530-
.send(ConfigUpdate::Partial {
531-
key: "statsd_metric_blocklist".to_string(),
532-
value: serde_json::json!(["foo".to_string()]),
533-
})
530+
.send(ConfigUpdate::Partial(ConfigSetting::explicit(
531+
"statsd_metric_blocklist",
532+
serde_json::json!(["foo".to_string()]),
533+
)))
534534
.await
535535
.unwrap();
536536

@@ -557,10 +557,10 @@ mod tests {
557557

558558
let mut metric_filterlist_watcher = cfg.watch_for_updates("metric_filterlist");
559559
sender
560-
.send(ConfigUpdate::Partial {
561-
key: "metric_filterlist".to_string(),
562-
value: serde_json::json!(["baz".to_string()]),
563-
})
560+
.send(ConfigUpdate::Partial(ConfigSetting::explicit(
561+
"metric_filterlist",
562+
serde_json::json!(["baz".to_string()]),
563+
)))
564564
.await
565565
.unwrap();
566566

@@ -594,10 +594,7 @@ mod tests {
594594
.await;
595595
let sender = sender.expect("sender should exist");
596596

597-
sender
598-
.send(ConfigUpdate::Snapshot(serde_json::json!({})))
599-
.await
600-
.unwrap();
597+
sender.send(ConfigUpdate::snapshot([])).await.unwrap();
601598
cfg.ready().await;
602599

603600
let mut filter = FilterBuilder::new()
@@ -616,10 +613,10 @@ mod tests {
616613

617614
let mut match_prefix_watcher = cfg.watch_for_updates("metric_filterlist_match_prefix");
618615
sender
619-
.send(ConfigUpdate::Partial {
620-
key: "metric_filterlist_match_prefix".to_string(),
621-
value: serde_json::json!(true),
622-
})
616+
.send(ConfigUpdate::Partial(ConfigSetting::explicit(
617+
"metric_filterlist_match_prefix",
618+
serde_json::json!(true),
619+
)))
623620
.await
624621
.unwrap();
625622

bin/agent-data-plane/src/components/tag_filterlist/mod.rs

Lines changed: 24 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,10 @@ pub fn filter_metric_tags(
345345
mod tests {
346346
use std::sync::Arc;
347347

348-
use saluki_config::{dynamic::ConfigUpdate, ConfigurationLoader};
348+
use saluki_config::{
349+
dynamic::{ConfigSetting, ConfigUpdate},
350+
ConfigurationLoader,
351+
};
349352
use saluki_context::{
350353
tags::{Tag, TagSet},
351354
Context, TagSetMutViewState,
@@ -915,21 +918,18 @@ mod tests {
915918
async fn dynamic_update_partial_replaces_filter() {
916919
let (cfg, sender) = ConfigurationLoader::for_tests(Some(serde_json::json!({})), None, true).await;
917920
let sender = sender.expect("sender should exist");
918-
sender
919-
.send(ConfigUpdate::Snapshot(serde_json::json!({})))
920-
.await
921-
.unwrap();
921+
sender.send(ConfigUpdate::snapshot([])).await.unwrap();
922922
cfg.ready().await;
923923

924924
let mut watcher = cfg.watch_for_updates("metric_tag_filterlist");
925925

926926
sender
927-
.send(ConfigUpdate::Partial {
928-
key: "metric_tag_filterlist".to_string(),
929-
value: serde_json::json!([
927+
.send(ConfigUpdate::Partial(ConfigSetting::explicit(
928+
"metric_tag_filterlist",
929+
serde_json::json!([
930930
{ "metric_name": "my.dist", "action": "exclude", "tags": ["host"] }
931931
]),
932-
})
932+
)))
933933
.await
934934
.unwrap();
935935

@@ -952,21 +952,18 @@ mod tests {
952952
async fn dynamic_update_to_empty_clears_filter() {
953953
let (cfg, sender) = ConfigurationLoader::for_tests(Some(serde_json::json!({})), None, true).await;
954954
let sender = sender.expect("sender should exist");
955-
sender
956-
.send(ConfigUpdate::Snapshot(serde_json::json!({})))
957-
.await
958-
.unwrap();
955+
sender.send(ConfigUpdate::snapshot([])).await.unwrap();
959956
cfg.ready().await;
960957

961958
let mut watcher = cfg.watch_for_updates("metric_tag_filterlist");
962959

963960
sender
964-
.send(ConfigUpdate::Partial {
965-
key: "metric_tag_filterlist".to_string(),
966-
value: serde_json::json!([
961+
.send(ConfigUpdate::Partial(ConfigSetting::explicit(
962+
"metric_tag_filterlist",
963+
serde_json::json!([
967964
{ "metric_name": "my.dist", "action": "exclude", "tags": ["env"] }
968965
]),
969-
})
966+
)))
970967
.await
971968
.unwrap();
972969

@@ -978,10 +975,10 @@ mod tests {
978975
.expect("timed out waiting for initial metric_tag_filterlist update");
979976

980977
sender
981-
.send(ConfigUpdate::Partial {
982-
key: "metric_tag_filterlist".to_string(),
983-
value: serde_json::json!([]),
984-
})
978+
.send(ConfigUpdate::Partial(ConfigSetting::explicit(
979+
"metric_tag_filterlist",
980+
serde_json::json!([]),
981+
)))
985982
.await
986983
.unwrap();
987984

@@ -1004,20 +1001,18 @@ mod tests {
10041001
async fn dynamic_update_snapshot_applies_filter() {
10051002
let (cfg, sender) = ConfigurationLoader::for_tests(Some(serde_json::json!({})), None, true).await;
10061003
let sender = sender.expect("sender should exist");
1007-
sender
1008-
.send(ConfigUpdate::Snapshot(serde_json::json!({})))
1009-
.await
1010-
.unwrap();
1004+
sender.send(ConfigUpdate::snapshot([])).await.unwrap();
10111005
cfg.ready().await;
10121006

10131007
let mut watcher = cfg.watch_for_updates("metric_tag_filterlist");
10141008

10151009
sender
1016-
.send(ConfigUpdate::Snapshot(serde_json::json!({
1017-
"metric_tag_filterlist": [
1010+
.send(ConfigUpdate::snapshot([ConfigSetting::explicit(
1011+
"metric_tag_filterlist",
1012+
serde_json::json!([
10181013
{ "metric_name": "my.dist", "action": "include", "tags": ["service"] }
1019-
]
1020-
})))
1014+
]),
1015+
)]))
10211016
.await
10221017
.unwrap();
10231018

0 commit comments

Comments
 (0)