chore(config): migrate TraceSamplerConfiguration to typed config#1984
Conversation
dd-octo-sts
Bot
added
area/components
There was a problem hiding this comment.
Pull request overview
This PR migrates the trace sampler transform to build TraceSamplerConfiguration from the resolved typed traces domain config (via ConfigurationSystem) instead of reading sampler-specific fields directly from the raw GenericConfiguration map. It also relocates sampler-related defaults into the typed configuration model/system and removes sampler parsing/defaulting from the shared APM helper.
Changes:
- Refactor
TraceSamplerConfigurationto be derived fromagent_data_plane_config::domains::traces::Domainand plumb it throughbin/agent-data-planetopology creation. - Remove probabilistic/rare-sampler-related fields and defaults from
lib/saluki-componentsAPM config helper. - Add shared default helpers + model defaults for traces domain/rare sampler and update Saluki-only seeding to always populate required values; extend tests accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/saluki-components/src/transforms/trace_sampler/mod.rs | Switches sampler config source to resolved typed traces config; updates builder wiring; adds a mapping test. |
| lib/saluki-components/src/common/datadog/apm.rs | Removes sampler-specific config parsing/defaults from the shared APM helper. |
| lib/agent-data-plane-config/src/domains/traces.rs | Introduces shared defaults and explicit Default impls for traces domain + rare sampler. |
| lib/agent-data-plane-config-system/src/saluki_only.rs | Makes certain Saluki-only APM knobs required-with-defaults and always seeds them into the traces domain; updates tests/docs. |
| bin/agent-data-plane/src/cli/run.rs | Passes ConfigurationSystem into baseline traces pipeline setup and builds trace sampler config from resolved traces domain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: ed7b23936c
ℹ️ 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".
Binary Size Analysis (Agent Data Plane)Baseline: 7b98c6f · Comparison: b6ea818 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (3)Experiments configured
Bounds Checks: ✅ Passed (3)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
This comment has been minimized.
This comment has been minimized.
webern
left a comment
webern
left a comment
There was a problem hiding this comment.
Claude Code operating on behalf of webern. I found two blocking behavioral regressions in the cutover: the probabilistic sampler's absent-value default changes, and two previously supported APM environment variables no longer reach the sampler. The latest rebase's generated environment overlay does not resolve the latter because it derives flat names from config paths rather than the schema's actual environment-variable names. Details inline.
webern
force-pushed
the
m/pr5-cutover
branch
2 times, most recently
from
c0cedef to
53f9087
Compare
Build the trace sampler from the resolved traces domain and move its Saluki-only defaults into the configuration layers.
webern
left a comment
webern
left a comment
There was a problem hiding this comment.
Claude Code operating on behalf of webern.
Re-reviewed at the current head (b6ea8182) after the rebase onto the reworked config
system. The rebase resolves one of my two earlier blockers; the other still stands.
Resolved: APM environment variables now reach the sampler
My previous review flagged that DD_APM_ENABLE_RARE_SAMPLER and
DD_APM_ERROR_TRACKING_STANDALONE_ENABLED no longer reached these fields, because the
generated environment overlay derived flat key names from the config path
(apm_config_enable_rare_sampler) rather than the schema's actual env-var names
(apm_enable_rare_sampler).
The rebased overlay generator now derives the flat forms from the schema's env bindings, so
the generated table carries the correct names:
flats: &["apm_enable_rare_sampler"], path: ["apm_config", "enable_rare_sampler"]
flats: &["apm_error_tracking_standalone_enabled"], path: ["apm_config", "error_tracking_standalone", "enabled"]
flats: &["apm_target_tps"], path: ["apm_config", "target_traces_per_second"]
flats: &["apm_error_tps"], path: ["apm_config", "errors_per_second"]
apply_env_overlay relocates these into the nested slots the typed deserializer reads, and
there is now test coverage for the relocation. This one is fixed — no further action needed.
Still blocking: probabilistic sampler rate now defaults to 0.0 instead of 1.0
The APM sampling rate is read without normalization, while its OTLP sibling is normalized:
sampling_rate: config.probabilistic_sampler.sampling_percentage / 100.0,
otlp_sampling_rate: normalize_sampling_rate(config.otlp.probabilistic_sampler_sampling_percentage / 100.0),normalize_sampling_rate clamps a rate of <= 0.0 or >= 1.0 back to 1.0. The old
ApmConfig defaulted probabilistic_sampler.sampling_percentage to 100, so the
un-normalized sampling_rate happened to land on 1.0. The typed model instead takes the
Datadog schema default, which is 0, so sampling_rate now defaults to 0.0 and is used
verbatim.
That value is not confined to the probabilistic path. apply_sampling_metadata writes it onto
every kept non-OTLP trace:
trace.otlp_sampling_rate = Some(if is_otlp { self.otlp_sampling_rate } else { self.sampling_rate });and the traces encoder emits it verbatim as the _dd.otlp_sr chunk tag. So on a default
deployment (probabilistic sampler disabled, no explicit percentage), every kept non-OTLP trace
now reports _dd.otlp_sr:0.00 where it previously reported 1.00. And if the probabilistic
sampler is enabled without an explicit percentage, sampling_rate of 0.0 drops all traces
rather than keeping them.
The fix that matches the OTLP path (and the encoder's own fallback, which clamps <= 0.0 /
>= 1.0 to 1.0) is to normalize this rate too:
sampling_rate: normalize_sampling_rate(config.probabilistic_sampler.sampling_percentage / 100.0),That maps the 0.0 default back to 1.0 and preserves the prior behavior. Worth a confirming
test that an unset percentage yields a 1.0 sampling rate.
|
Human writing: I believe the pressing issues and blockers have been mitigated, so I'm merging this into the aggregation branch. |
## 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
Build
TraceSamplerConfigurationfrom 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
How did you test this PR?
make build-schema-overlay && make fmtmake check-allmake testmake check-docsReferences