chore(config): migrate metrics/service-checks/checks-IPC/MRF gateway to typed config#1990
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the migration from GenericConfiguration to the typed SalukiConfiguration model by updating several pipeline components (metrics encoder, service checks encoder, checks IPC source, and the MRF gateway) to read from typed shared/domain config and use the Live<T> mechanism for dynamic updates.
Changes:
- Migrates Datadog Metrics + Service Checks encoders to build from
shared.metrics_encodingandshared.endpoints(including new model→runtimeFrommappings for V3 settings). - Migrates Checks IPC source to read
domains.checks.ipc_endpointand parse aListenAddressat the boundary. - Refactors the MRF metrics gateway to watch
Live<multi_region_failover::Domain>for runtime updates rather than watching raw config keys.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/saluki-components/src/transforms/mrf_gateway/mod.rs | Switches MRF gateway from raw-key watchers to Live<Domain> updates and updates tests accordingly. |
| lib/saluki-components/src/sources/checks_ipc/mod.rs | Reads the checks IPC listen endpoint from typed checks domain config and parses it into ListenAddress. |
| lib/saluki-components/src/encoders/datadog/service_checks/mod.rs | Builds service checks encoder config from typed shared MetricsEncoding + Compression and replaces smoke test with a construction test. |
| lib/saluki-components/src/encoders/datadog/metrics/mod.rs | Builds metrics encoder config from typed shared MetricsEncoding + Endpoints, updates flush-timeout handling, and replaces smoke/deser tests with model-mapping tests. |
| lib/saluki-components/src/common/datadog/protocol.rs | Adds From impls to convert typed-model V3 settings into runtime encoder protocol config. |
| lib/saluki-components/src/common/datadog/endpoints.rs | Removes now-unused is_empty helpers related to additional endpoints. |
| lib/agent-data-plane-config/src/shared.rs | Moves encoder defaults (flush timeout, max metrics per payload) into MetricsEncoding::default. |
| lib/agent-data-plane-config/src/domains/checks.rs | Moves checks IPC endpoint default into checks::Domain::default. |
| lib/agent-data-plane-config-system/src/saluki_only.rs | Extends saluki-only config system tests to assert new defaults are present. |
| bin/agent-data-plane/src/cli/run.rs | Updates topology assembly to pass typed config objects (and Live<Domain> where needed) into migrated components. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Binary Size Analysis (Agent Data Plane)Baseline: 7b98c6f · Comparison: 1647d4b · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (3)Experiments configured
Bounds Checks: ✅ Passed (3)
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 ( |
webern
left a comment
There was a problem hiding this comment.
Claude Code operating on behalf of webern. I found one input-sanitization regression in the V3 series endpoint-mode migration; see the inline comment. The other cutovers preserve the old defaults and reachability, and the MRF live-domain update behavior looks consistent with the previous watchers.
| fn from(mode: &agent_data_plane_config::shared::V3SeriesMode) -> Self { | ||
| Self { | ||
| enabled: mode.mode.clone(), | ||
| endpoints: mode.endpoint_modes.clone(), |
There was a problem hiding this comment.
This copies the model's endpoint-mode map without preserving the old deserialization boundary. Before this cutover, UseV3ApiSeriesConfig accepted only string or boolean values (V3SeriesModeValue); a numeric/null/object mode made as_typed::<DatadogMetricsConfiguration>() fail. The model translator currently stringifies every other JSON value (unwrap_or_else(|mode| mode.to_string())), so e.g. use_v3_api.series.endpoints: {<endpoint>: 1} now starts successfully and silently becomes the invalid string "1", which the runtime treats as false. That makes malformed input more likely to enter the component and changes startup failure into silently disabled V3. Please retain the old String-or-Bool validation (or record a translation error for other value types) before this conversion.
This comment has been minimized.
This comment has been minimized.
…to typed config Build the Datadog Metrics and Service Checks encoders, the Checks IPC source, and the MRF metrics gateway transform from the typed SalukiConfiguration model instead of the raw GenericConfiguration map. - Metrics and service-checks encoders read shared.metrics_encoding and shared.endpoints; V3 API / series-mode model types convert into the encoder's runtime types via From impls in protocol.rs. - Checks IPC source reads domains.checks.ipc_endpoint. - MRF gateway becomes a dynamic component driven by Live<multi_region_failover>. - Hoist the encoder flush-timeout (2s) and max-metrics-per-payload (10,000) defaults into MetricsEncoding::default, and the checks IPC endpoint default (tcp://0.0.0.0:5105) into checks::Domain::default, matching the aggregate slice pattern; the component structs no longer carry defaults. - Retire the per-struct config smoke tests; coverage now lives in translator and construction tests. Progresses #1788
The metrics-encoder cutover let use_v3_api.series.endpoints accept any JSON value and stringified it, weakening validation the old V3SeriesModeValue deserializer enforced. Restore the string/boolean-only domain in consume_use_v3_api_series_endpoints, recording a TranslateError for other kinds while preserving valid entries.
| let mut health = context.take_health_handle(); | ||
| let mut failover_metrics_watcher = self | ||
| .configuration | ||
| .watch_for_updates("multi_region_failover.failover_metrics"); | ||
| let mut metric_allowlist_watcher = self | ||
| .configuration | ||
| .watch_for_updates("multi_region_failover.metric_allowlist"); | ||
| let mut live = self.live.clone(); | ||
|
|
||
| health.mark_ready(); | ||
| debug!(mode = ?self.mode, "MRF metrics gateway transform started."); |
webern
left a comment
There was a problem hiding this comment.
Claude Code operating on behalf of webern — fresh clean-room pass on the current HEAD (fixup included).
The V3 series endpoint-mode sanitization regression from the earlier pass is now fixed and well-covered: strings pass through, bools normalize to "true"/"false", and every other JSON kind records a translation error (rejecting at startup) while valid entries still survive a runtime update. The rest of the cutover looks transparent — checks-IPC default tcp://0.0.0.0:5105 matches the old any_tcp(5105) and still fails startup on a bad address; the metrics/service-checks payload, compression, and flush defaults are preserved (including the 0 → 10ms flush floor and the zstd agent-default→3 swap); the has_additional_endpoints V3 gate matches the old !is_empty(); and the MRF gateway's Live<Domain> cutover is equivalent to the old two-key watchers (build-time mode and the live view both project the same multi_region_failover domain, and Live::changed() only wakes on a real change to that projection).
One non-blocking note on test coverage. This PR retires two per-struct tests that guarded absent-key defaults:
use_v2_api_series_default::defaults_to_true_when_absent(guaranteeduse_v2_api_seriesdefaults totrue)- the
max_series_points_per_payloaddefault assertion (guaranteed10_000when absent)
The replacements only assert that explicit values map through the model — nothing now asserts the effective default when the key is absent. Both are witnessed keys, so the real default arrives from the schema via the witness driver, and the model Default sets use_v2_series_api: false as a placeholder. That's correct today (schema default is true and drive() runs unconditionally), but if the witness wiring ever drifts the default silently flips V2→V1 series routing with no test catching it. The seeded defaults added here (max_metrics_per_payload, checks ipc_endpoint) are guarded by absent_keys_leave_model_defaults, but that test only runs seed(), so it can't cover the witnessed ones.
Suggest adding a translate()-level assertion on an empty config that use_v2_series_api == true (and ideally max_series_points_per_payload == 10_000) to restore the lost guard. Non-blocking — the current behavior is correct.
The forwarder cutover moved additional_endpoints onto the typed model, and #1990 did the same for the metrics encoder, so AdditionalEndpoints's serde impl has no production caller. Its deserialization tests were the last consumer of serde_yaml in this crate; drop the now-unused dependency.
The forwarder cutover moved additional_endpoints onto the typed model, and #1990 did the same for the metrics encoder, so AdditionalEndpoints's serde impl has no production caller. Its deserialization tests were the last consumer of serde_yaml in this crate; drop the now-unused dependency.
…to typed config (#1990) Migrates four more pipeline components off the raw `GenericConfiguration` map and onto the typed `SalukiConfiguration` model. - **Datadog Metrics encoder** builds from `shared.metrics_encoding` and `shared.endpoints`. The model's `V3ApiEncoding` / `V3SeriesMode` types convert into the encoder's runtime `V3ApiConfig` / `UseV3ApiSeriesConfig` via `From` impls in `protocol.rs`; the additional-endpoints presence check reads the model map directly (the now-unused `AdditionalEndpoints::is_empty` is removed). - **Datadog Service Checks encoder** builds from `shared.metrics_encoding` and `shared.endpoints.compression`, mirroring the Events encoder. - **Checks IPC source** reads `domains.checks.ipc_endpoint` and parses it into a `ListenAddress` at the boundary. - **MRF metrics gateway** becomes a dynamic component: it takes `Live<multi_region_failover::Domain>` and re-derives its routing mode from the live domain instead of watching raw keys. Defaults move up to the config layer, following the aggregate-slice pattern (real default in the model `Default`, Saluki-only source stays `Option` with a conditional seed): - `MetricsEncoding::default` now sets the encoder flush timeout (2s) and max-metrics-per-payload (10,000). - `checks::Domain::default` now sets the IPC endpoint (`tcp://0.0.0.0:5105`). The component structs no longer carry source serde or their own defaults, and the per-struct config smoke tests are retired; coverage now lives in the translator tests and the components' construction tests. - [x] Non-functional (chore, refactoring, docs) - `make build-schema-overlay && make fmt` (no changes) - `make check-all` - `make check-docs` - `cargo nextest run --lib --bins` for the affected crates - New construction/dynamic-update tests for each migrated component Claude Code operating on behalf of webern — fresh clean-room pass on the current HEAD (fixup included). The V3 series endpoint-mode sanitization regression from the earlier pass is now fixed and well-covered: strings pass through, bools normalize to `"true"`/`"false"`, and every other JSON kind records a translation error (rejecting at startup) while valid entries still survive a runtime update. The rest of the cutover looks transparent — checks-IPC default `tcp://0.0.0.0:5105` matches the old `any_tcp(5105)` and still fails startup on a bad address; the metrics/service-checks payload, compression, and flush defaults are preserved (including the `0 → 10ms` flush floor and the zstd agent-default→3 swap); the `has_additional_endpoints` V3 gate matches the old `!is_empty()`; and the MRF gateway's `Live<Domain>` cutover is equivalent to the old two-key watchers (build-time mode and the live view both project the same `multi_region_failover` domain, and `Live::changed()` only wakes on a real change to that projection). One non-blocking note on test coverage. This PR retires two per-struct tests that guarded **absent-key** defaults: - `use_v2_api_series_default::defaults_to_true_when_absent` (guaranteed `use_v2_api_series` defaults to `true`) - the `max_series_points_per_payload` default assertion (guaranteed `10_000` when absent) The replacements only assert that explicit values map through the model — nothing now asserts the effective default when the key is absent. Both are witnessed keys, so the real default arrives from the schema via the witness driver, and the model `Default` sets `use_v2_series_api: false` as a placeholder. That's correct today (schema default is `true` and `drive()` runs unconditionally), but if the witness wiring ever drifts the default silently flips V2→V1 series routing with no test catching it. The seeded defaults added here (`max_metrics_per_payload`, checks `ipc_endpoint`) are guarded by `absent_keys_leave_model_defaults`, but that test only runs `seed()`, so it can't cover the witnessed ones. Suggest adding a `translate()`-level assertion on an empty config that `use_v2_series_api == true` (and ideally `max_series_points_per_payload == 10_000`) to restore the lost guard. Non-blocking — the current behavior is correct. - Progresses #1788 - Merges into `m/pr5-cutover`
AI Summary
Migrates four more pipeline components off the raw
GenericConfigurationmapand onto the typed
SalukiConfigurationmodel.shared.metrics_encodingandshared.endpoints. The model'sV3ApiEncoding/V3SeriesModetypes convertinto the encoder's runtime
V3ApiConfig/UseV3ApiSeriesConfigviaFromimpls in
protocol.rs; the additional-endpoints presence check reads the modelmap directly (the now-unused
AdditionalEndpoints::is_emptyis removed).shared.metrics_encodingandshared.endpoints.compression, mirroring the Events encoder.domains.checks.ipc_endpointand parses it into aListenAddressat the boundary.Live<multi_region_failover::Domain>and re-derives its routing mode from thelive domain instead of watching raw keys.
Defaults move up to the config layer, following the aggregate-slice pattern
(real default in the model
Default, Saluki-only source staysOptionwith aconditional seed):
MetricsEncoding::defaultnow sets the encoder flush timeout (2s) andmax-metrics-per-payload (10,000).
checks::Domain::defaultnow sets the IPC endpoint (tcp://0.0.0.0:5105).The component structs no longer carry source serde or their own defaults, and the
per-struct config smoke tests are retired; coverage now lives in the translator
tests and the components' construction tests.
Change Type
How did you test this PR?
make build-schema-overlay && make fmt(no changes)make check-allmake check-docscargo nextest run --lib --binsfor the affected cratesFinal AI Review Output
Claude Code operating on behalf of webern — fresh clean-room pass on the current HEAD (fixup included).
The V3 series endpoint-mode sanitization regression from the earlier pass is now fixed and well-covered: strings pass through, bools normalize to
"true"/"false", and every other JSON kind records a translation error (rejecting at startup) while valid entries still survive a runtime update. The rest of the cutover looks transparent — checks-IPC defaulttcp://0.0.0.0:5105matches the oldany_tcp(5105)and still fails startup on a bad address; the metrics/service-checks payload, compression, and flush defaults are preserved (including the0 → 10msflush floor and the zstd agent-default→3 swap); thehas_additional_endpointsV3 gate matches the old!is_empty(); and the MRF gateway'sLive<Domain>cutover is equivalent to the old two-key watchers (build-time mode and the live view both project the samemulti_region_failoverdomain, andLive::changed()only wakes on a real change to that projection).One non-blocking note on test coverage. This PR retires two per-struct tests that guarded absent-key defaults:
use_v2_api_series_default::defaults_to_true_when_absent(guaranteeduse_v2_api_seriesdefaults totrue)max_series_points_per_payloaddefault assertion (guaranteed10_000when absent)The replacements only assert that explicit values map through the model — nothing now asserts the effective default when the key is absent. Both are witnessed keys, so the real default arrives from the schema via the witness driver, and the model
Defaultsetsuse_v2_series_api: falseas a placeholder. That's correct today (schema default istrueanddrive()runs unconditionally), but if the witness wiring ever drifts the default silently flips V2→V1 series routing with no test catching it. The seeded defaults added here (max_metrics_per_payload, checksipc_endpoint) are guarded byabsent_keys_leave_model_defaults, but that test only runsseed(), so it can't cover the witnessed ones.Suggest adding a
translate()-level assertion on an empty config thatuse_v2_series_api == true(and ideallymax_series_points_per_payload == 10_000) to restore the lost guard. Non-blocking — the current behavior is correct.References
m/pr5-cutover