chore(config): migrate APM and trace components to typed config - #1991
Conversation
Build the APM stats encoder, Datadog trace encoder, trace obfuscation transform, and APM stats transform from the resolved SalukiConfiguration model instead of the raw configuration map, and delete the shared `ApmConfig` parsing helper they relied on. Trace settings are read from `domains.traces` (env, sampling, obfuscation, OTLP trace knobs); the shared encoder flush timeout and compression settings are read from `shared`. The obfuscator config is built from the typed `Obfuscation` model via a `From` conversion. The encoder flush timeout (`flush_timeout_secs`) becomes a required Saluki-only value with a shared 2s default hoisted into the config layer. - Progresses #1788
There was a problem hiding this comment.
Pull request overview
Migrates the APM/trace pipeline components away from GenericConfiguration parsing and onto the typed agent_data_plane_config::SalukiConfiguration model, removing the shared ApmConfig helper and centralizing the encoder flush-timeout default in the config layer.
Changes:
- Build trace/obfuscation/APM-stats components from the resolved traces domain model (
domains.traces) plus shared settings (shared.metrics_encoding,shared.endpoints.compression). - Delete
common/datadog/apm.rs(ApmConfig) and replace obfuscation parsing with aFrom<&model::Obfuscation>conversion. - Hoist the shared encoder
flush_timeoutdefault (2s) intoagent-data-plane-configand apply it viasaluki_onlyseeding.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/saluki-components/src/transforms/trace_obfuscation/mod.rs | Builds obfuscation transform config from typed traces domain; replaces config smoke test with targeted unit test. |
| lib/saluki-components/src/transforms/apm_stats/mod.rs | Builds APM stats transform config from typed traces domain and removes dependency on ApmConfig. |
| lib/saluki-components/src/encoders/datadog/traces/mod.rs | Constructs trace encoder config from typed model + shared flush timeout + shared compression; simplifies tests. |
| lib/saluki-components/src/encoders/datadog/stats/mod.rs | Constructs APM stats encoder config from typed model + shared flush timeout; updates tests accordingly. |
| lib/saluki-components/src/common/datadog/obfuscation.rs | Adds From<&domains::traces::Obfuscation> to map typed model into obfuscator config. |
| lib/saluki-components/src/common/datadog/mod.rs | Removes the apm module export after ApmConfig deletion. |
| lib/saluki-components/src/common/datadog/apm.rs | Deletes legacy shared APM config parsing helper. |
| lib/agent-data-plane-config/src/shared.rs | Introduces shared default encoder flush timeout (2s) and implements Default for MetricsEncoding. |
| lib/agent-data-plane-config-system/src/saluki_only.rs | Makes flush_timeout_secs a defaulted Saluki-only setting and always seeds shared.metrics_encoding.flush_timeout. |
| bin/agent-data-plane/src/cli/run.rs | Wires traces pipeline component construction to use the typed configuration model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Binary Size Analysis (Agent Data Plane)Baseline: 7b98c6f · Comparison: 7f63ad4 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f63ad424d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (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, reviewing on behalf of @webern.
I did a clean-room pass focused on default/behavior preservation. The migration is faithful on almost every axis I checked, but there is one real behavior change worth resolving before this rolls up.
The one real issue: obfuscation defaults flip from false to true
This is the same thing the Codex bot flagged, and I agree with it after tracing it through.
The old ApmConfig obfuscation path deserialized ObfuscationConfig with plain #[serde(default)], so every enabled flag defaulted to false when the key was absent. The typed model instead takes the vendored Datadog schema defaults, which are true for credit_cards, elasticsearch, memcached, mongodb, opensearch, redis, and valkey:
datadog-agent/config/src/generated/datadog_configuration.rs— eachApmConfigObfuscation*::enabledisdefault_bool::<true>().datadog_translator.rs(consume_apm_config_obfuscation_*_enabled) writes those intodomains.traces.obfuscation.- The obfuscator gates on these (
obfuscator.rs:if config.credit_cards.enabled,config.es.enabled,config.mongo.enabled,config.open_search.enabled, plus the per-span redis/memcached/valkey paths).
So an empty/default config now obfuscates those span fields where the removed path left them off. That directly contradicts the PR description's claim that "witnessed keys keep their schema defaults (which match the old component defaults)" — for obfuscation they do not match.
One nuance that affects severity: when a fully-resolved config is streamed in from the core Agent (the same situation the serializer_zstd_compressor_level handling in datadog_translator.rs is written around), these keys arrive as concrete values and both the old and new paths see true, so that path is unchanged. The divergence shows up when the keys are absent — standalone / default-config / test runs. Whether that path matters is your call.
Two reasonable resolutions:
- Accept the change (it matches upstream Agent behavior, and the old
falsedefault was arguably the ADP-side bug) and fix the PR description so it no longer claims defaults are unchanged; or - Preserve the previous
falsedefaults if standalone/default-config behavior needs to be bit-for-bit.
Either is fine, but it should be a decision rather than an accident.
Minor: lost env-var precedence coverage for error-tracking-standalone
The deleted apm.rs tests included ets_enabled_via_env_var and ets_env_var_overrides_yaml, which exercised the DD_APM_ERROR_TRACKING_STANDALONE_ENABLED → key aliasing and env-over-YAML precedence — real config-loading behavior, not just deserialization. The retained trace-encoder tests only cover the header/chunk-tag logic given a bool, and I couldn't find replacement coverage for error_tracking_standalone resolution in the config-system crate. Not blocking, but if that aliasing/precedence isn't covered generically elsewhere, it's now untested.
What checked out (no action needed)
target_traces_per_second/errors_per_second(schema default 10/10) andcompute_stats_by_span_kind/peer_tags_aggregation(schema default true/true) match the oldApmConfigdefaults, and are driven unconditionally.env(top-level, empty→"none" fallback) anddefault_env(seeded fromapm_config.default_env, default "none") preserve the old sourcing and defaults.- The
From<&model::Obfuscation>conversion maps every sub-field correctly, field-for-field. flush_timeoutconsolidation is faithful: the manualMetricsEncodingDefaultkeeps 2s, seeding always writes it, and the metrics encoder is untouched (still reads its ownflush_timeout_secs), so no metrics-side regression; the0 → 10msfloor is preserved for traces/stats.- Compression sourcing (
serializer_compressor_kind/_zstd_compressor_level, zstd/3 via the existing translator override) is preserved. - Hostname handling (
set_hostname_if_empty→unwrap_or_default) and the OTLPignore_missing_datadog_fields/ sampling-percentage reads are equivalent. - The config-smoke test deletions are acceptable losses; the new translator/construction tests cover the mapping.
|
Claude Code, commenting on behalf of @webern. Owner resolution: this behavior change is accepted. These values come from the vendored Datadog schema, whose defaults are driven into the typed model by the translator. In the normal fully-resolved configuration path, those schema defaults already clobber the component-local fallback. Aligning the standalone/default-config path with that source of truth is preferable to preserving the old all-false The PR description should describe this as intentional alignment with the Datadog schema rather than claiming that these particular defaults match the old component defaults. |
## 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`
TODO: human writes here 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. - [x] Non-functional (chore, refactoring, docs) - `make build-schema-overlay && make fmt` (no file changes) - `make check-all` - `make test` - `make check-docs` - Progresses #1788 - Merges into `m/pr5-cutover`
TODO: human writes here 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. - [x] Non-functional (chore, refactoring, docs) - `make build-schema-overlay && make fmt` (no file changes) - `make check-all` - `make test` - `make check-docs` - Progresses #1788 - Merges into `m/pr5-cutover`
Human Summary
TODO: human writes here
AI Summary
Build the APM/trace pipeline components from the typed
SalukiConfigurationmodel instead of the raw configuration map, and remove the shared
ApmConfigparsing helper they all leaned on.
Migrated components:
DatadogApmStatsEncoderConfiguration(APM stats encoder)DatadogTraceConfiguration(Datadog trace encoder)TraceObfuscationConfiguration(trace obfuscation transform)ApmStatsTransformConfiguration(APM stats transform)Details:
obfuscation, OTLP trace knobs) are read from
domains.traces. The sharedencoder flush timeout is read from
shared.metrics_encoding, and thecompression settings from
shared.endpoints.compression.ObfuscationConfigis built from the typedObfuscationmodel via a
Fromconversion, so the obfuscation engine is untouched.ApmConfigincommon/datadog/apm.rsis deleted; nothing else consumed it.flush_timeout_secs) becomes a requiredSaluki-only value whose 2s default is hoisted into the config layer and
shared across the metrics, trace, and APM stats encoders.
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
envfallback normalize to the same effective values as before.Change Type
How did you test this PR?
make build-schema-overlay && make fmt(no file changes)make check-allmake testmake check-docsReferences
m/pr5-cutover