chore(config): migrate logging translation to typed config#2000
Closed
webern wants to merge 30 commits into
Closed
chore(config): migrate logging translation to typed config#2000webern wants to merge 30 commits into
webern wants to merge 30 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
This PR migrates ADP logging configuration consumption from the raw GenericConfiguration map to the typed SalukiConfiguration/control.logging model, and updates the dynamic log-level watcher to follow a Live<String> view instead of watching raw config keys.
Changes:
- Add
translate_snapshotinagent-data-plane-config-systemto produce a one-shot typed config snapshot for bootstrap-time consumers. - Update logging initialization/reload paths to translate raw config → typed snapshot →
LoggingConfiguration. - Refactor
DynamicLogLevelWorkerto react toLive<String>updates ofcontrol.logging.level, and rewrite related unit tests to build the typed logging model directly.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/agent-data-plane-config-system/src/translators/datadog_translator.rs | Adds translator tests validating logging schema defaults, driven values, and invalid byte-size error behavior. |
| lib/agent-data-plane-config-system/src/system.rs | Introduces translate_snapshot for bootstrap-phase typed config translation with lenient error handling. |
| lib/agent-data-plane-config-system/src/lib.rs | Re-exports translate_snapshot. |
| bin/agent-data-plane/src/main.rs | Switches bootstrap logging setup to use a typed snapshot before building logging config. |
| bin/agent-data-plane/src/cli/run.rs | Reloads logging after authoritative config arrives via typed snapshot translation. |
| bin/agent-data-plane/src/internal/mod.rs | Threads &ConfigurationSystem into internal supervisor creation. |
| bin/agent-data-plane/src/internal/control_plane.rs | Wires DynamicLogLevelWorker to a live view of control.logging.level and updates config-internal wiring. |
| bin/agent-data-plane/src/internal/logging.rs | Refactors logging translation to accept the typed logging model; refactors dynamic log-level worker and tests. |
| bin/agent-data-plane/Cargo.toml | Removes the serde_with dependency (no longer needed after refactor). |
| Cargo.lock | Updates lockfile to reflect dependency removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+81
to
+85
| let log_file = if logging.disable_file_logging { | ||
| String::new() | ||
| } else if logging.file.is_empty() { | ||
| PlatformSettings::get_default_log_file_path() | ||
| .to_string_lossy() |
Comment on lines
+435
to
+438
| /// Flipping the level through the driven view wakes the worker's watch and yields the new first-party filter that | ||
| /// the worker feeds to the logging controller. | ||
| #[tokio::test] | ||
| async fn worker_reacts_to_live_log_level_change() { |
Binary Size Analysis (Agent Data Plane)Baseline: 7b98c6f · Comparison: a3eb0be · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
![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: 6e60f4ea20
ℹ️ 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".
| .to_string_lossy() | ||
| .into_owned(), | ||
| } | ||
| logging.file.clone() |
There was a problem hiding this comment.
When data_plane.log_file is absent, the typed Datadog model already contains the schema default /var/log/datadog/agent-data-plane.log, so this else branch treats that Linux path as an explicit configuration and never calls PlatformSettings::get_default_log_file_path(). On macOS and Windows, where PlatformSettings points at /opt/datadog-agent/logs or the registry-derived log directory, ADP now writes to the wrong default path unless the user overrides data_plane.log_file.
Useful? React with 👍 / 👎.
| // (per-subagent log file key, never sharing a file with the Core Agent). The configuration system does not exist | ||
| // yet at this point, so translate a one-shot typed snapshot of the bootstrap configuration to read the model from. | ||
| let bootstrap_saluki_config = | ||
| agent_data_plane_config_system::translate_snapshot(&bootstrap_config, EnvOverlayMode::Fallback) |
There was a problem hiding this comment.
This bootstrap path now deserializes the whole config through the generated typed Datadog model before logging translation. The generated logging fields are plain bools, so values the previous read_permissive_bool path accepted, such as DD_LOG_TO_SYSLOG=1 or quoted YAML like log_to_console: "false", fail source deserialization and abort bootstrap instead of applying the logging setting.
Useful? React with 👍 / 👎.
|
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.
Clean-room review of the logging cutover. Overall this is a faithful migration: all eleven logging keys are witnessed with real consume_* bodies and land in control.logging, the syslog gating and default-URI fallback are preserved, the log-level parsing path is unchanged (parse_adp_log_level still routes plain levels through the first-party filter), and the test rewrites keep the behavioral coverage rather than just the deserialization. The Live<String> conversion for the dynamic worker is a clean equivalent of the old key watch.
Two things are worth resolving before this goes into the big-bang merge, plus a couple of low-severity notes.
1. Platform-specific default log-file path is lost on non-Linux
bin/agent-data-plane/src/internal/logging.rs still carries the PlatformSettings::get_default_log_file_path() fallback for an empty logging.file, but that branch is now unreachable in the default case.
data_plane.log_file carries a hardcoded schema default of /var/log/datadog/agent-data-plane.log (core_schema.yaml), so drive() populates logging.file with that Linux path even when the user configures nothing. logging.file is therefore never empty unless the user explicitly sets data_plane.log_file: "", and the platform fallback only fires in that one corner.
- On Linux this is a no-op (same path).
- On macOS (
/opt/datadog-agent/logs) and Windows (C:\ProgramData\Datadog\logs, registry-derived), the pre-cutover code fell back to the platform path when the key was unset; it now writes the Linux path instead. That's a default regression.
Note the asymmetry with syslog_uri, whose schema default is empty (''), so its platform fallback still works correctly. The Core Agent's own log_file also defaults to ''. The cleanest fix is probably to give data_plane.log_file an empty default too (via the overlay/vendored schema), so the config layer keeps its "empty means use the platform default" contract and the existing translate fallback governs — rather than baking a single-platform literal into the model.
(Both Copilot and Codex flagged this independently. Copilot's comment references an adp-logging-windows-default-path integration test that doesn't exist in the tree, so ignore that specific citation — the underlying issue is real regardless.)
2. Dropped permissive-boolean coercion
The old component ran the logging booleans (log_format_json, log_to_console, log_to_syslog, syslog_rfc, disable_file_logging, etc.) through PermissiveBool; the typed model deserializes plain bool. Plain true/false still work (figment coerces those from env), but the forms PermissiveBool explicitly accepted — the short forms 1/0/t/T/f/F, and quoted-YAML strings like log_to_console: "false" — no longer deserialize, and at the bootstrap sites they'll fail DatadogConfiguration deserialization and abort startup rather than being coerced.
Logging was one of the few places that deliberately reached for PermissiveBool, so the question is whether that was load-bearing (mirroring the Agent's viper-style leniency for these keys) or belt-and-suspenders that's fine to drop for consistency with the other migrated bool keys. If it should be preserved, it needs hoisting to the source layer (e.g. alongside the existing normalize_datadog_input_forms coercions) rather than living in the component. Worth a conscious decision either way.
Minor notes (not blocking)
- The two bootstrap sites go through the lenient
translate_snapshot, which turns an invalidlog_file_max_sizeinto a warning and leavesfile_max_size = 0(→ByteSize::b(0)). The pre-cutover component returned a hard error that aborted startup. This matches the config system's documented lenient-update contract, so it's likely intentional — just flagging that bootstrap no longer rejects a malformed byte size. consume_log_file_max_rollsclamps negatives to0(value.max(0)) where the oldtry_get_typed::<usize>would have errored. Benign, and consistent with the project's integer handling.
The LoggingConfiguration field-by-field construction, syslog gating, and disable-file short-circuit all match the prior behavior. #1 is the one I'd want addressed before merge; #2 deserves an explicit decision.
## 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 LoggingConfigurationTranslator and DynamicLogLevelWorker over from the raw GenericConfiguration map to the typed SalukiConfiguration model. The translator now builds LoggingConfiguration from control.logging (all witnessed keys, driven from the vendored schema) rather than re-parsing raw config, keeping only the ADP-specific business rules (first-party log-filter expansion, syslog gating with platform-default fallback, per-subagent log-file path). DynamicLogLevelWorker reacts to a Live<String> view of the log level. Logging is set up before the configuration system exists, so add a lenient one-shot translate_snapshot to the config-system crate and use it at the bootstrap and pre-config-system reload sites.
… bools Follow-up to the logging typed-config migration. data_plane.log_file: flag the key saluki_overrides_default so its typed default is empty instead of the Agent schema's Linux literal. An unset key now leaves control.logging.file empty, which LoggingConfigurationTranslator resolves to PlatformSettings::get_default_log_file_path() (correct on macOS and Windows, not just Linux). An explicit path is preserved. Logging permissive booleans: log_format_json, log_format_rfc3339, log_to_console, log_to_syslog, syslog_rfc, and disable_file_logging were read with saluki_common::deser::PermissiveBool before the migration. Coerce their wider input forms back to native booleans in normalize_datadog_input_forms (after apply_env_overlay, so env-var strings are covered too), scoped to just these six keys. Invalid forms are left in place so the bool deserialize still rejects them.
… sentinel Follow-up to the platform-log-file fix. `control.logging.file` used an empty string as a sentinel for "unset, resolve the platform default", which the `consume_data_plane_log_file` witness decoded from its `Option<String>` and the component re-decoded via `is_empty()`. That routed the same "maybe unset" fact through two representations. Make the model type honest: `Logging.file` is now `Option<String>` (`None` = unset). The witness assignment becomes a straight pass-through, and `LoggingConfigurationTranslator` resolves `None` (or a blank path) to `PlatformSettings::get_default_log_file_path()` via `unwrap_or_else`. No behavior change: unset/blank still fall back to the platform default, an explicit path still wins, and `disable_file_logging` still short-circuits.
…way snapshot On the config-stream path, the logging reload built a full SalukiConfiguration via `translate_snapshot` only to read `control.logging`, then discarded it — moments before `ConfigurationSystem::load` deserialized and translated the same map all over again. That was two full translations for one logging sub-tree. Move the reload to just after the configuration system is loaded and read the logging model straight from it. The reload now runs after the `!enabled` early exit (so a disabled ADP no longer reloads logging on its way out) and before topology/env-provider setup. `translate_snapshot` remains for the `main.rs` bootstrap site, where no configuration system exists yet.
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 crazy stuff happening in this one. I am not going to merge it into the aggregating PR that is part of the merge stack. This will me left as a follow up into main if we end up merging the stack.
AI Summary
Cut
LoggingConfigurationTranslatorandDynamicLogLevelWorkerover from the rawGenericConfigurationmap to the typedSalukiConfigurationmodel.All eleven logging keys the translator reads (
log_level,log_format_json,log_format_rfc3339,log_to_console,log_to_syslog,syslog_rfc,syslog_uri,log_file_max_size,log_file_max_rolls,disable_file_logging,data_plane.log_file) arewitnessed against the vendored Datadog schema and were already driven into
control.logging.The translator now builds
LoggingConfigurationfrom that typed model instead of re-parsing rawconfig, keeping only the ADP-specific business rules: first-party log-filter expansion, syslog
gating with a platform-default URI fallback, and the per-subagent log-file path with a
platform-default fallback (the Core Agent's own
log_fileis still never consulted).DynamicLogLevelWorkerno longer stores aGenericConfiguration; it reacts to aLive<String>view ofcontrol.logging.leveland re-applies the parsed filter on change.Logging is initialized before the configuration system exists, so this adds a lenient one-shot
translate_snapshotto the config-system crate (translation errors keep field defaults, mirroringthe runtime update path) and uses it at the two bootstrap-phase sites: the initial logging setup in
mainand the config-stream reload inrun. The control-plane wiring now takes&ConfigurationSystemand mints both the log-level view and the/config/internalhandle from it.Change Type
How did you test this PR?
gating, log-file fallback/disable, and byte-size conversion; added a driven-
Livetest that theworker reacts to a live log-level change.
log_file_max_sizebyte-size parse), and the invalid-byte-size error path.make build-schema-overlay && make fmt(no drift),make check-all,make test,make check-docs.References