chore(config): migrate DogStatsD metric filters to typed config#1998
Closed
webern wants to merge 29 commits into
Closed
chore(config): migrate DogStatsD metric filters to typed config#1998webern wants to merge 29 commits into
webern wants to merge 29 commits into
Conversation
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
Environment variables reach the config map as flat, underscore-joined top-level keys (`DD_AUTOSCALING_FAILOVER_ENABLED` -> `autoscaling_failover_enabled`), while `DatadogConfiguration` deserializes the nested shape. The whole-struct deserialize never reads those flat keys, so an env var set for a multi-segment key is silently dropped. The legacy per-key lookup bridged this at query time by retrying the dotted key with `.` replaced by `_`; the typed path issues no per-key queries, so that bridge no longer fires. Reinstate it as a one-time pass over the merged value, driven by a generated table of the supported multi-segment keys. For each known dotted path, relocate any matching flat key into the nested slot the deserializer reads. This runs the safe direction (known path -> its single flat form), so it never has to guess where an arbitrary flat key's nesting boundaries are. String lists are split on whitespace to match the Agent's env convention; scalars relocate verbatim. The mode is chosen at deserialization: Fallback fills only slots the Agent left empty (matching the legacy precedence, restoring prior behavior), Override lets an env var replace an Agent-supplied value, and Disabled relocates nothing. The overlay is applied only to the Datadog input; the Saluki-only source deserializes from the untouched value and is out of scope here. (cherry picked from commit 7e6e4095dbfc12224773216c4c6ecf6611103442)
Preparatory fixes the component cutovers depend on; none of these change a component yet. - Load ConfigurationSystem before topology assembly and thread the system handle down to the DogStatsD pipeline builder (unused for now, hence the leading underscore). - Reshape the Saluki-only source struct to mirror the config key hierarchy exactly, so each field maps by plain serde to its real config-key path. - Model the memory limit as a byte size (u64 in the model, ByteSize in the source) so a bare-integer value no longer fails the whole config load.
Environment variables reach the merged map as flat, underscore-joined keys, but SalukiOnly deserializes a nested shape, so a DD_* var for a dotted Saluki-only path (e.g. DD_DATA_PLANE_STANDALONE_MODE) was silently dropped. The Datadog overlay already fixes this for schema keys via a generated table; SalukiOnly has no table by design. Discover SalukiOnly's canonical leaf paths algorithmically from its derived Deserialize using a hand-rolled recording deserializer (no new dependency), treating scalar-like custom types (DurationString, ByteSize, Duration) as leaves. Cache the paths. Before deserializing SalukiOnly, relocate each multi-segment path's flat form into its nested slot, honoring EnvOverlayMode (Disabled/Fallback/Override). Single-segment keys are already flat and untouched.
Cut the aggregate transform over from raw GenericConfiguration access to the typed SalukiConfiguration model. AggregateConfiguration builds from the dogstatsd aggregation slice and the shared histogram encoding; histogram parsing moves into HistogramConfiguration::from_encoding, and the component config struct no longer carries a Default or deserializes Datadog config. The aggregation slice is made authoritative from the Agent schema, dropping the redundant Saluki-only aggregation keys. run.rs reads the typed config and hands the transform its subtree.
…onfig Cut the debug-log destination over to the typed SalukiConfiguration model. DogStatsDDebugLogConfiguration builds from the dogstatsd debug-log slice and holds a Live view, so the destination reacts to metrics-stats changes through the live view instead of a string-key watch. run.rs hands it the typed subtree and mints the live view from the configuration system.
## AI Summary Build the OTTL filter processor from the resolved traces-domain configuration instead of parsing `GenericConfiguration` in the component. Remove the component-owned source serde configuration and use the shared `OttlErrorMode` model type. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` - `make check-all` - `make test` - `make check-docs` - Targeted OTTL filter processor tests ## References Related to configuration cutover PR #1979.
) ## AI Summary Build `DatadogEventsConfiguration` from the typed shared configuration model instead of deserializing the generic configuration map. Remove component-local source defaults and retire the configuration smoke test. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` - `make check-fmt check-docs` - `make test` - `make check-deny check-unused-deps check-licenses check-features generate-api-docs` - `cargo check -p saluki-components -p agent-data-plane` ## References No issue.
Build the Datadog Logs encoder from the shared typed compression configuration and remove its raw configuration deserialization. - [x] Non-functional (chore, refactoring, docs) - `make build-schema-overlay && make fmt` - `make check-all` - `make test` - `make check-docs`
## AI Summary Build autoscaling failover from the translated shared configuration instead of reading `GenericConfiguration` directly. The topology builder now passes the typed model slice into the component, and component tests construct the typed configuration directly. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` - `cargo check -p agent-data-plane` - `cargo test -p saluki-components --lib` (744 passed, 1 ignored) - `make check-docs` - `make check-all` passed formatting, clippy, docs, dependency checks, API docs, and began the feature matrix before timing out during the remaining matrix checks. ## References - Progresses #1788
## AI Summary Build multi-region failover configuration from the typed `SalukiConfiguration` domain model and preserve endpoint normalization during translation. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` - `make check-all` - `make test` - `make check-docs` - Targeted MRF and configuration-system unit tests ## References - Progresses #1788
## AI Summary Build `TraceSamplerConfiguration` from the resolved traces configuration instead of the raw configuration map. Move trace sampler defaults into the typed configuration layers and remove the sampler-specific source parsing from the shared APM helper. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` - `make check-all` - `make test` - `make check-docs` ## References - Progresses #1788
## AI Summary
Migrates `ClusterAgentConfiguration` off the raw `GenericConfiguration`
map onto
the typed configuration model. The struct is now built from the
translated
`shared::ClusterAgent` slice; the `cluster_agent.*` keys are witnessed
(present
in the vendored Datadog schema) and already resolved into the model.
Trimming of the Cluster Agent URL, auth token, and Kubernetes service
name moves
up into the config layer (the witness translator), since trailing
whitespace
would otherwise corrupt the value at the point of use (URL parsing,
bearer
token, service-name-derived env lookup). Component construction just
copies the
already-normalized fields, and its `from_configuration` signature
changes from
`&GenericConfiguration` to `&shared::ClusterAgent`.
The call site in `run.rs` now passes the typed `shared.cluster_agent`
slice
alongside the existing `shared.autoscaling_failover` slice.
## Change Type
- [x] Non-functional (chore, refactoring, docs)
## How did you test this PR?
- Component construction tests rewritten to build from `ClusterAgent`
model
slices directly (enabled gating, URL scheme normalization, Kubernetes
service
resolution, IPv6 host wrapping, URL-vs-service precedence).
- Added translator tests covering the relocated normalization:
surrounding
whitespace is trimmed and empty/whitespace-only values collapse to
`None`, and
an absent `kubernetes_service_name` picks up the Datadog schema default.
- `make build-schema-overlay && make fmt` (no file changes), `make
check-all`,
`make test`, `make check-docs` all pass.
### Follow-up Fixed by AI Review
Claude Code operating on behalf of webern.
Follow-up review of `216f6836f89f333773e21fc04032134a2b2729a3`: the
reported regression is fixed. The translator now preserves the
distinction between the schema default and an explicitly blank
`cluster_agent.kubernetes_service_name`: absent input still becomes
`Some("datadog-cluster-agent")`, configured values are trimmed, and
empty or whitespace-only input becomes `Some("")`. That empty value
reaches `resolve_endpoint`, which suppresses Kubernetes service
discovery even when the default service environment variables are
present. URL and auth-token normalization remain unchanged.
I verified coverage for the schema default, explicit empty input,
whitespace-only input, and the component-level suppression behavior with
default service environment variables available. The fix is narrowly
scoped, and the relevant formatting, Clippy, schema-overlay, and
unit-test CI checks reported success during verification.
The original review concern is resolved. This is good to go from the
config-cutover review perspective.
## References
- Progresses #1788
- Merges into `m/pr5-cutover`
## Human Summary There's some weird stuff going on here with preserving "absence" in an attempt to preserve exact behavioral parity, but I think it's OK. ## AI Summary Cuts the OTLP components over from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, following the PR #1979 pattern: `OtlpConfig` (shared receiver config), the OTLP source, decoder, forwarder, and relay. Each component now builds from typed model slices (`domains.otlp`, `domains.traces.otlp`) instead of deserializing the raw config: - **Source / relay / decoder / forwarder**: `from_configuration` takes borrowed model slices; the call site in `run.rs` reads them from the config system. Component structs carry no serde and no defaults. - **Seeded OTLP knobs** (context sizing, HTTP receiver transport, trace interner size, top-level-by-span-kind) are converted to the required-with-default doctrine: non-optional in `SalukiOnly` with the default defined once as a shared `const`/`fn` in `agent-data-plane-config` and referenced by both the model `Default` and the `SalukiOnly` serde default. The two byte-size interner keys are typed `ByteSize` so they accept both a bare integer and a suffixed string. - **`OtlpTracesTranslator`** now takes the two native booleans it actually reads instead of the whole raw `TracesConfig`. `TracesConfig` is kept only for the not-yet-migrated Datadog trace encoder. - The per-component config smoke tests are retired (coverage now lives in the translator and construction tests). ### Behavior notes Because the OTLP receiver keys are witnessed against the vendored Datadog schema, their defaults now come from that schema rather than Saluki's former hardcoded values. When a key is left unset: - gRPC/HTTP receiver endpoints default to `localhost:4317` / `localhost:4318` (previously `0.0.0.0:...`), matching the Datadog Agent. - `otlp_config.logs.enabled` defaults to `false` (previously `true`), matching the Datadog Agent. - `otlp_config.receiver.protocols.grpc.max_recv_msg_size_mib` of `0` now maps to grpc-go's built-in 4 MiB limit, matching the Agent and the schema documentation (previously `0` produced a 0-byte limit). Deployments that set these keys explicitly (including the OTLP correctness cases, which pin the gRPC endpoint to `0.0.0.0:4317`) are unaffected. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` (no generated drift), `make check-all`, `make check-docs`. - Unit tests for `agent-data-plane-config`, `agent-data-plane-config-system`, `saluki-components`, and `agent-data-plane`, including new `SalukiOnly` transport/default tests (both byte-size input forms) and per-component construction tests. - Statically reviewed the OTLP correctness cases: all send over gRPC on the explicitly configured `0.0.0.0:4317`, so the receiver default changes above do not affect them. ## References - Progresses #1788 - Merges into `m/pr5-cutover`
## Human Summary TODO: human writes here ## AI Summary Build the APM/trace pipeline components from the typed `SalukiConfiguration` model instead of the raw configuration map, and remove the shared `ApmConfig` parsing helper they all leaned on. Migrated components: - `DatadogApmStatsEncoderConfiguration` (APM stats encoder) - `DatadogTraceConfiguration` (Datadog trace encoder) - `TraceObfuscationConfiguration` (trace obfuscation transform) - `ApmStatsTransformConfiguration` (APM stats transform) Details: - Trace settings (env, sampling targets, error tracking, peer tags, obfuscation, OTLP trace knobs) are read from `domains.traces`. The shared encoder flush timeout is read from `shared.metrics_encoding`, and the compression settings from `shared.endpoints.compression`. - The obfuscator's `ObfuscationConfig` is built from the typed `Obfuscation` model via a `From` conversion, so the obfuscation engine is untouched. - `ApmConfig` in `common/datadog/apm.rs` is deleted; nothing else consumed it. - The encoder flush timeout (`flush_timeout_secs`) becomes a required Saluki-only value whose 2s default is hoisted into the config layer and shared across the metrics, trace, and APM stats encoders. - The per-struct config smoke tests are retired in favor of translator and component construction tests; classification metadata is unchanged. Behavior is preserved: witnessed keys keep their schema defaults (which match the old component defaults), and the OTLP probabilistic sampler default and `env` fallback normalize to the same effective values as before. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` (no file changes) - `make check-all` - `make test` - `make check-docs` ## References - Progresses #1788 - Merges into `m/pr5-cutover`
…to typed config (#1990) ## AI Summary 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. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `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 ### Final 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 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. ## References - Progresses #1788 - Merges into `m/pr5-cutover`
…1992) ## AI Summary Build the DogStatsD source and metric mapper from the typed `SalukiConfiguration` model (`domains.dogstatsd`) instead of the raw `GenericConfiguration` map. - **Source** (`DogStatsDConfiguration`): `from_configuration` now takes `&domains::dogstatsd::Domain` plus a caller-derived default capture directory. `run_path` is deliberately unmodeled, so the call site reads it from the raw config and passes `run_path/dsd_capture` in; the component no longer touches `GenericConfiguration`. All source serde (`Deserialize`, renames, `default_*` fns) is stripped; injected runtime state (workload provider, capture/replay control) stays. Origin enrichment builds from the model via `OriginEnrichmentConfiguration::from_model`. - **Mapper** (`DogStatsDMapperConfiguration`): `from_configuration` takes `&domains::dogstatsd::Mapper` and builds the `MetricMapper` from typed `MapperProfile`s. The interner byte budget parses as `ByteSize` in `SalukiOnly` (so `64KiB` and a bare integer both load). - **Defaults**: the Saluki-only DogStatsD listener/context/mapper defaults (`buffer_count`, `buffer_count_max`, `permissive_decoding`, `cached_contexts_limit`, `cached_tagsets_limit`, `allow_context_heap_allocs`, `minimum_sample_rate`, mapper `string_interner_size`) move into hand-written model `Default` impls, so an absent key yields the value the components expect. Witnessed keys keep their schema-driven defaults. - Retire the two component config smoke tests (the structs no longer deserialize); construction and parsing coverage now lives in the translator/`SalukiOnly` tests and the components' own tests. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` (no file changes) - `make check-all` - `make test` - `make check-docs` ### Last of Several AI PR Reviews Claude Code, operating on behalf of @webern. Verified at `eec29828`: all three alternate-input-form regressions are resolved, and I think this is ready. - `dogstatsd_string_interner_size_bytes` — the `SalukiOnly` field is now `Option<ByteSize>` with `.as_u64()` in `seed()`, mirroring the mapper interner, so the suffix-string form (`"2MiB"`) loads again alongside the bare integer. - `dogstatsd_eol_required` — `normalize_datadog_input_forms` splits the space-separated string in the input seam (after the env overlay, before `DatadogConfiguration::deserialize`), matching `deserialize_space_separated_or_seq`. - `dogstatsd_mapper_profiles` — the same helper parses the JSON-string form into the array the translator expects, leaving malformed JSON in place so the normal error still surfaces. Each form has both alternate- and native-shape coverage through `ConfigurationSystem::load` / `seed`, replacing the deleted component-level tests at the right layer. Only `saluki_only.rs` and `system.rs` changed — no generated files or model types touched. The defaults and field mappings I checked earlier still hold. The deterministic static gates are green (`check-schema-overlay`, `check-fmt`, `check-deny`, `check-docs`, `check-unused-deps`, mergegate); unit tests are still running. No further blocking items from me. ## References - Progresses #1788 - Merges into `m/pr5-cutover`
## Human Summary Caught by AI when reviewing #1993, it looks like this was a regression in the environment variable path only, which *should not* have affected customers anyway, but it looks like now we handle JSON in environment variable correctly if we happen to ever read that. ## AI Summary Restores the wider `additional_endpoints` input shapes that were lost when the metrics encoder and forwarder moved off the component serde onto the typed model. The old `AdditionalEndpoints` serde used `PickFirst<(DisplayFromStr, _)>` plus `OneOrMany`, so it accepted: - the whole map as a JSON string — the form an environment variable produces, e.g. `DD_ADDITIONAL_ENDPOINTS='{"https://app.datadoghq.com":["key"]}'`, and - a bare string in place of a one-element key list for a host. The generated `DatadogConfiguration` deserializes `additional_endpoints` as a plain `HashMap<String, Vec<String>>`, and the env overlay only materializes scalar/space-separated-list env values, so neither shape survives. Both now fail deserialization outright — a dual-shipping deployment configured through the environment fails to start. This adds an `additional_endpoints` case to `normalize_datadog_input_forms` (the same pass that already restores the `dogstatsd_eol_required` and `dogstatsd_mapper_profiles` env-var forms): parse the JSON-string form into an object, then wrap any bare-string host value into a one-element array. Invalid JSON is left in place so the downstream deserializer still surfaces the error. ## Change Type - [x] Bug fix ## How did you test this PR? - `cargo nextest run -p agent-data-plane-config-system` (added `additional_endpoints_accepts_json_string_scalar_or_map`, covering the JSON-string, JSON-string-with-scalar, native-scalar, and native-list shapes; confirmed it fails without the fix with `invalid type: string ..., expected a map`). - `make fmt` ## References - Merges into `m/pr5-cutover`
## Human Summary Agents are thrashing hard on #1965 and they have a hard time understanding that we can't fix it right now. I tried to get everything else fixed. ## AI Summary Cuts the Datadog forwarder subsystem over from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model. This is a tightly-coupled cluster, so it moves together in one PR. Static construction (no more source serde, no restated defaults): - `ForwarderConfiguration`, `EndpointConfiguration`, `ProxyConfiguration`, and `RetryConfiguration` build field-by-field from `shared.endpoints` (and `shared.metrics_encoding` for the V3 settings). - `DatadogForwarderConfiguration` builds from the shared model; the Cluster Agent forwarder and the `run.rs` call sites are rewired to pass typed slices. Runtime refresh (now reads a `Live<SalukiConfiguration>` view instead of a live `GenericConfiguration`): - `ResolvedEndpoint` refreshes its API key from a typed source: the shared primary key, the Multi-Region Failover override key, or the dual-shipping endpoint keys by URL/index. - `TransactionForwarder` and `ApiKeyValidator` thread the live view; the retry policy's 403 secrets gate reads it too. `ApiKeyValidator` re-validates when any API-key source changes. Config model: - `secret_backend_command`, `secret_refresh_on_api_key_failure_interval`, and `run_path` are witnessed keys (in the vendored schema), promoted from `excluded:` to `inventory:` and modeled as `shared.secrets` and `shared.run_path`, with `consume_*` bodies in the Datadog translator. The `ForwarderConfiguration` and `ProxyConfiguration` config smoke tests are retired; equivalent coverage now lives in the construction and translator tests. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - `make build-schema-overlay && make fmt` (no changes) - `make check-all` - `make check-docs` - `cargo nextest run` for `saluki-components` (`common::datadog`, `forwarders`), `agent-data-plane-config`, `agent-data-plane-config-system`, and the `agent-data-plane` config/cli modules ## References - Progresses #1788 - Merges into `m/pr5-cutover`
There was a problem hiding this comment.
Pull request overview
Refactors the DogStatsD metric-filter transforms in agent-data-plane to consume the new typed SalukiConfiguration model (and Live<T> reactive views) instead of reading from GenericConfiguration and per-key update watchers, aligning these components with the typed config cutover work.
Changes:
- Migrates the prefix filter, post-aggregate filter, and tag filterlist transforms to use typed config slices +
Live<T>for runtime-reactive updates. - Consolidates filterlist rebuilding logic via
EffectiveFilterlist::from_prefix_filterand removes now-unused config-key constants and component-local serde/default plumbing. - Rewrites dynamic-update tests to drive
Live<T>updates viaArcSwap+watch, matching the typed config system behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| bin/agent-data-plane/src/components/tag_filterlist/mod.rs | Switches tag-filterlist config + runtime updates from GenericConfiguration watchers to Live<Vec<MetricTagFilterEntry>>, updates tests accordingly. |
| bin/agent-data-plane/src/components/dogstatsd_prefix_filter/mod.rs | Migrates prefix filter to typed PrefixFilter + Live<PrefixFilter>, simplifies update handling and removes config smoke test. |
| bin/agent-data-plane/src/components/dogstatsd_post_aggregate_filter/mod.rs | Migrates post-aggregate filter to typed histogram encoding + Live<PrefixFilter> for reactive list updates, updates tests. |
| bin/agent-data-plane/src/components/dogstatsd_filterlist.rs | Removes config-key constants and adds EffectiveFilterlist::from_prefix_filter helper for reuse. |
| bin/agent-data-plane/src/cli/run.rs | Updates pipeline wiring to pass typed config values and Live<T> projections into the three transforms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+236
to
+239
| _ = live.changed() => { | ||
| let prefix_filter = live.current(); | ||
| debug!(?prefix_filter, "Updated metric filterlist configuration."); | ||
| self.apply_update(&prefix_filter); |
Comment on lines
+218
to
+221
| _ = live.changed() => { | ||
| let prefix_filter = live.current(); | ||
| debug!(?prefix_filter, "Updated post-aggregate metric filterlist configuration."); | ||
| self.apply_update(&prefix_filter); |
Binary Size Analysis (Agent Data Plane)Baseline: 7b98c6f · Comparison: 9de8df6 · 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 (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 ( |
) ## Summary Cut the OTTL transform processor over from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, following the pattern already applied to the sibling OTTL filter processor. - `OttlTransformConfiguration::from_configuration` now takes the resolved `domains::traces::OttlTransform` settings and copies them directly, instead of deserializing an `OttlTransformConfig` out of `GenericConfiguration`. The component-local config module (its `ErrorMode` enum and `OttlTransformConfig` struct) is removed; the component reuses the model's shared `OttlErrorMode`. - The call site in `run.rs` reads `saluki.domains.traces.ottl_transform`. Since this was the last consumer of the raw config in `add_baseline_traces_pipeline_to_blueprint`, the now-unused `GenericConfiguration` parameter is dropped from that function. - The seed source `OttlTransformConfig` in `saluki_only.rs` is aligned with the filter's: its `error_mode` deserializes directly into `OttlErrorMode` (rather than a loose `Option<String>` fed through a hand-rolled parse helper) and it now rejects unknown fields, restoring the strict deserialization the original component had. `ottl_transform_config` is absent from the vendored Datadog schema, so it is a Saluki-only key on the seed track; there is no witnessed alias for it. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - Rewrote the component tests to build from typed traces settings; all OTTL transform tests pass. - Added an `ottl_transform_config` round-trip rejection test mirroring the filter's. - `make build-schema-overlay` leaves the tree unchanged; `make fmt`, `make check-all`, `make test`, and `make check-docs` all pass. ## References - Progresses #1788 - Merges into #1987
webern
commented
Jul 6, 2026
webern
left a comment
webern
left a comment
Contributor
Author
There was a problem hiding this comment.
Claude Code operating on behalf of webern. I found two input-compatibility regressions that should be fixed before merge. The ordinary defaults and reachability for the prefix/block lists, histogram suffixes, namespace alias, and tag-filter cache default otherwise look preserved, and the Live<T> update paths look sound. CI is green; Copilot's two comments only concern verbose debug logging and do not cover these compatibility issues.
| } | ||
|
|
||
| #[test] | ||
| fn invalid_action_defaults_to_exclude() { |
Contributor
Author
There was a problem hiding this comment.
This test covered behavior, not merely component-local deserialization. Before this cutover, an unknown action was warned about and coerced to Exclude, so the initial configuration still loaded; null was also accepted as Exclude. The typed translator now records a TranslateError for an unknown action (which makes ConfigurationSystem::load reject startup) and rejects null. Its RawEntry also adds #[serde(default)] to tags, although this field was previously required; an entry such as { metric_name: "my.dist", action: "include" } can now load and strip every ordinary tag from that metric. Please preserve the old accepted-input/coercion contract in the translator and move the regression tests there rather than deleting them.
| .domains | ||
| .dogstatsd | ||
| .aggregation | ||
| .aggregator_tag_filter_cache_capacity, |
Contributor
Author
There was a problem hiding this comment.
Claude: Please preserve the old rejection of negative cache capacities. The previous try_get_typed::<usize> failed on a negative value. The model translator currently uses value.max(0) as usize, so -1 is silently accepted as zero, after which build_context_cache turns it into a one-entry cache. That is both an input-sanitization and behavioral regression. Translation should reject/record an error for negative values (and use a checked conversion), with a regression test.
## Human Summary It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. ## AI Summary Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. ## References - Progresses #1788 - Merges into #1987
Cut the DogStatsD prefix filter, post-aggregate filter, and tag filterlist transforms over from the raw GenericConfiguration map to the typed SalukiConfiguration model. Each component now takes borrowed model slices for its static settings and a Live<T> view for the fields it reacts to at runtime, replacing the stored GenericConfiguration and per-key watch_for_updates calls. The prefix/post-agg filters track domains.dogstatsd.prefix_filter; the tag filterlist tracks domains.dogstatsd.tag_filterlist and reads the dedup-cache capacity from domains.dogstatsd.aggregation. Histogram suffixes come from shared.metrics_encoding.histogram. The shared filterlist helper drops the now-unused config-key constants and per-field setters, gaining EffectiveFilterlist::from_prefix_filter. Component serde, defaults, and the prefix-filter smoke test are removed; those defaults are now supplied by the generated schema witness. Reactive tests drive a Live view through an ArcSwap + watch channel instead of ConfigurationLoader.
Restore two input behaviors that the DogStatsD metric-filter cutover changed:
- metric_tag_filterlist entries again match the pre-cutover deserialization
contract. A missing, null, empty, or "exclude" action means exclude and
"include" means include; any other action string is tolerated (kept, coerced
to exclude, warned) instead of rejecting startup. "tags" is required again, so
an entry like {"metric_name":"m","action":"include"} no longer loads with
an empty tag set that would strip every tag from the metric. The obsolete
TagFilterEntry recovery enum is dropped in favor of a plain Result.
- data_plane.dogstatsd.aggregator_tag_filter_cache_capacity now goes through the
checked integer conversion, so a negative or unrepresentable value records a
translation error instead of being silently clamped to zero (which would build
a degenerate one-entry dedup cache). The valid default of 100,000 and valid
zero behavior are preserved.
Adds translator regression tests covering unknown, null, missing, and
missing-tags action forms and the capacity default/zero/negative cases.
1 task
This comment has been minimized.
This comment has been minimized.
webern
force-pushed
the
m/pr6-adp-cutover
branch
7 times, most recently
from
47a5a4f to
f638633
Compare
Contributor
Author
|
Deferred |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Summary
There's a lot going on in this one. I'm not going to merge it into the aggregating PR, so we can treat this one on it's own (and last) if everything else merges.
AI Summary
Migrates the three DogStatsD metric-filter transforms from the raw
GenericConfigurationmap to the typedSalukiConfigurationmodel:DogStatsDPrefixFilterConfigurationDogStatsDPostAggregateFilterConfigurationTagFilterlistConfigurationAll three are dynamic (runtime-reactive) transforms. Each now takes borrowed
model slices for its static settings and a
Live<T>view for the fields itreacts to, replacing the stored
GenericConfigurationand the per-keywatch_for_updatescalls:domains.dogstatsd.prefix_filter(filterlist/blocklist and their prefix-match flags). The prefix filter reads
the namespace and its exemption list once; the post-aggregate filter reads the
histogram aggregate/percentile suffixes once from
shared.metrics_encoding.histogram.domains.dogstatsd.tag_filterlistand reads thededup-cache capacity from
domains.dogstatsd.aggregation.The shared
dogstatsd_filterlisthelper drops the now-unused config-keyconstants and per-field setters and gains
EffectiveFilterlist::from_prefix_filter.Component-side serde, hand-written defaults (histogram aggregates/percentiles,
the metric-namespace blocklist, the tag-filter cache capacity), and the
prefix-filter smoke test are removed; the generated schema witness now supplies
those defaults. The prefix filter's per-field runtime update counter is
preserved by comparing the effective (active) filter values across updates.
Reactive tests drive a
Liveview through anArcSwap+watchchannel,mirroring the reference cutover, in place of
ConfigurationLoader.Change Type
How did you test this PR?
make check-all(fmt, clippy, docs, deny, licenses, unused-deps, api-docs, features)make test(all unit tests pass)make build-schema-overlay && make fmtleave the tree unchanged (no schema drift)References