feat(payload): adopt Probability for TimestampConfig::probability#1877
feat(payload): adopt Probability for TimestampConfig::probability#1877goxberry wants to merge 1 commit into
Conversation
|
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. |
|
goxberry
force-pushed
the
goxberry/probability-timestamp-config
branch
from
9f7f336 to
106f2ca
Compare
![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: 106f2cadb5
ℹ️ 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".
| Self { | ||
| range: ConfRange::Constant(1), | ||
| probability: 0.0, | ||
| probability: Probability::try_new(0.0).expect("0.0 is in [0.0, 1.0]"), |
There was a problem hiding this comment.
For this repo, the provided /workspace/lading/AGENTS.md explicitly says No .unwrap() or .expect() and MUST NOT panic; this new production Default implementation adds an .expect() panic path. Even though 0.0 is currently a valid Probability, keeping construction panic-free preserves the documented error-handling invariant and avoids future refactors of Probability::try_new turning a default config path into a panic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good flag. @goxberry words are failing me now, but I wonder if there's a nice tidy way to express "is 0%" and "is 100%" as infallible constructors.
goxberry
force-pushed
the
goxberry/probability-timestamp-config
branch
from
106f2ca to
91a7f3b
Compare
Change the public `TimestampConfig::probability` field from `f32` to the
`Probability` alias of `BoundedProbability<{ f32::to_bits(0.0) }>`. The
`try_from` impl enforces the finite + `[0.0, 1.0]` invariant at deserialize
time, so the matching check in `TimestampConfig::valid()` is removed.
The new type threads through `MemberGenerator::new` and
`MetricGenerator::new`. At the comparison sites in `metric::timestamp`,
`.get()` extracts the inner `f32` so RNG sequences and bit-exact output
are preserved.
goxberry
force-pushed
the
goxberry/probability-timestamp-config
branch
from
91a7f3b to
2905d45
Compare
| R: Rng + ?Sized, | ||
| { | ||
| if self.timestamp_probability == 0.0 { | ||
| if self.timestamp_probability.get() == 0.0 { |
There was a problem hiding this comment.
No change needed, vaguely wondering if we should have a is_zero(&Self) -> bool
| Self { | ||
| range: ConfRange::Constant(1), | ||
| probability: 0.0, | ||
| probability: Probability::try_new(0.0).expect("0.0 is in [0.0, 1.0]"), |
There was a problem hiding this comment.
This is a good flag. @goxberry words are failing me now, but I wonder if there's a nice tidy way to express "is 0%" and "is 100%" as infallible constructors.
2 participants
What does this PR do?
Switches
TimestampConfig::probabilityfrom baref32toProbability(
BoundedProbability<{ f32::to_bits(0.0) }>from #1876). Thetry_fromimpl enforces finite +
[0.0, 1.0]at deserialize time, so the matchingcheck in
TimestampConfig::valid()is removed. The new type threadsthrough
MemberGenerator::newandMetricGenerator::new; at thecomparison sites in
MetricGenerator::timestamp,.get()extracts theinner
f32so RNG draws and bit-exact output are preserved.Motivation
Phase 1, PR 1 of the stacked rollout planned in #1876 — wire
BoundedProbabilityaliases intolading_payloadconfigs one field ata time, smallest blast radius first.
TimestampConfigis self-containedand has its own
valid(), so it's the cleanest field to start with.Moving the validation into the type means invalid configs are rejected
at parse time with a structured error instead of after
Config::newreturns a string; the in-code field can no longer hold a value outside
[0.0, 1.0].Related issues
Stacked on #1876 (Phase 0). Follow-ups to land in this stack:
ValueConf::float_probability,Config::sampling_probability,Config::multivalue_pack_probability,Config::unique_tag_ratio(viaAtLeastOneHundredth).Additional Notes
Wire format unchanged.
Probabilityserializes as a baref32via
serde(into = "f32", try_from = "f32"), so YAML/JSON configswritten for the previous field type continue to parse.
No sampler swap. The hot-path comparison stays
OpenClosed01.sample(rng) < self.timestamp_probability.get(); the.get()is aconst fnreturning a copied scalar and inlines away.Test fixture changes. Test bodies that constructed
TimestampConfigliterally now go through aprob(f32) -> Probabilityhelper. The
timestamp_probability_must_be_validtest was rewrittento
timestamp_probability_rejects_out_of_range_on_deserializeandpins the new deserialize-time rejection path (using YAML, matching
lading's config format).
Bench delta.
cargo bench -p lading_payload --bench dogstatsdagainst the pre-change baseline:
dogstatsd_setup: −3.8%dogstatsd_throughput/1 MiB: −12.6% (improved)dogstatsd_throughput/10 MiB: −18.7% (improved)dogstatsd_throughput/100 MiB: −4.7% (improved)dogstatsd_throughput/1 GiB: no changeAll deltas are favorable or neutral; the per-sample variance at small
buffer sizes is consistent with system noise rather than a real
speedup.
--bench block(Json payload, unrelated) shows noise-levelvariance.