chore(config): migrate trace sampler to typed configuration - #2191
Conversation
dd-octo-sts
Bot
added
area/components
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The migration preserves trace sampler behavior through the resolved configuration path: generated schema defaults still provide the historical 10 TPS values, while Saluki-only defaults and explicit nested settings are seeded into the traces domain. No diff-only behavioral regression was identified. No additional tests recommended: the existing configuration transport coverage already exercises the migrated Saluki-only fields, and execution was blocked by unavailable Rust dependencies.
🤖 Datadog Autotest · Commit fa6af8d · What is Autotest? · Any feedback? Reach out in #autotest
![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: fa6af8d373
ℹ️ 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".
| let otlp_sampling_rate = normalize_sampling_rate(traces.otlp.probabilistic_sampler_sampling_percentage / 100.0); | ||
| Self { | ||
| probabilistic_sampler_enabled: traces.probabilistic_sampler.enabled, | ||
| sampling_percentage: traces.probabilistic_sampler.sampling_percentage, |
There was a problem hiding this comment.
When apm_config.probabilistic_sampler.enabled is set but sampling_percentage is omitted, this now copies the typed Datadog default of 0_f64 from lib/datadog-agent/config/src/generated/datadog_configuration.rs:1769; the removed raw ApmConfig default was 100%, so line 111 builds a sampling rate of 0.0 and the probabilistic path drops all non-error/non-rare traces. Preserve the previous 100% fallback for the omitted-percentage case so this typed-config cutover remains behaviorally transparent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is correct and a bugfix that goes toward #1802. ADP should honor the core Agent defaults. Fortunately this feature isn't released to customers yet so only our tests are affected.
Binary Size Analysis (Agent Data Plane)Baseline: 2d270c1 · Comparison: 3ca5877 · 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: ❌ Failed (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 ( |
b5f20b6 to
8efc4d4
Compare
760cc24 to
be8b6a3
Compare
da6bd8a to
45fade5
Compare
dd-octo-sts
Bot
added
encoder/datadog-stats
Build the trace sampler from the resolved traces domain slice instead of reading GenericConfiguration through the shared ApmConfig helper. - Take the sampler's probabilistic, error, priority, and rare-sampler inputs from domains.traces, and drop its raw deserialization. - Establish the Saluki-only sampler defaults in the traces model, matching ADP's historical values: default_env "none", error_sampling_enabled on, and rare_sampler tps 5, cooldown 300s, cardinality 200. - Trim ApmConfig to the span-kind, peer-tags, environment, hostname, and obfuscation surface still read by the APM stats and trace obfuscation transforms, removing the now-unused sampler fields and their legacy deserialization tests. The witnessed apm_config.probabilistic_sampling.sampling_percentage takes the Datadog schema default of 0 rather than ADP's former 100. This only affects the effective rate when the APM probabilistic sampler is enabled without an explicit percentage; the sampler is off by default.
There was a problem hiding this comment.
More details
The typed migration preserves the trace sampler's Datadog defaults, Saluki-only overrides, and OTLP sampling normalization across the inspected startup and override paths. Local Rust execution could not run because the sandbox cannot fetch the repository's pinned git dependency, so confidence is based on the three-dot diff and complete source-level path comparison.
🤖 Datadog Autotest · Commit 3ca5877 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ca5877e4b
ℹ️ 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".
| error_sampling_enabled: self.apm_config.error_sampling_enabled(), | ||
| error_tracking_standalone: self.apm_config.error_tracking_standalone_enabled(), | ||
| probabilistic_sampler_enabled: self.apm_config.probabilistic_sampler_enabled(), | ||
| sampling_rate: self.sampling_percentage / 100.0, |
There was a problem hiding this comment.
With the typed config, the Core Agent default for apm_config.probabilistic_sampler.sampling_percentage is 0 while probabilistic_sampler_enabled defaults to false. In that default legacy path, kept non-OTLP traces still pass through apply_sampling_metadata, which copies this sampling_rate into trace.otlp_sampling_rate, and the trace encoder writes it as _dd.otlp_sr; default-sampled traces are therefore emitted with 0.00 instead of the prior/default 1.00 unless users explicitly set a sampling percentage. Fresh evidence is the disabled-sampler metadata path, not the intentional 0% behavior when the probabilistic sampler is enabled; avoid using the disabled sampler's percentage for the emitted rate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't really understand this. Does it seem problematic for an unreleased feature under development?
There was a problem hiding this comment.
Sounds like a separate bugfix but also something that should have a correctness test on it that'd break if you changed it. Maybe worth kicking off a clanker session on it to put a correctness test in place so we don't inadvertently regress it here.
2 participants
Human Summary
This is a relatively straightforward conversion of
TraceSamplerConfigurationto typed config. No issues were encountered. The Codex flag is because we are changing our default to match the Agent (which is what we want).AI Summary
Migrate the trace sampler transform to the typed traces configuration domain.
TraceSamplerConfigurationfromdomains.tracesinstead ofGenericConfigurationandApmConfig.ApmConfig.Change Type
How did you test this PR?
make fmtmake build-schema-overlay(no generated drift)cargo check --workspace --testscargo clippy -p saluki-components -p agent-data-plane-config-system -p agent-data-plane -p agent-data-plane-config --all-targetscargo nextest run -p saluki-components -p agent-data-plane-config-system -p agent-data-plane-config(843 passed, 1 skipped)References