Skip to content

feat(payload): adopt Probability for TimestampConfig::probability #1877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
goxberry wants to merge 1 commit into
base: goxberry/improve-probability-type-ergonomics
Choose a base branch
from
Open

feat(payload): adopt Probability for TimestampConfig::probability #1877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 25 additions & 28 deletions lading_payload/src/dogstatsd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use tracing::{debug, warn};
use crate::{
Serialize,
common::{
config::ConfRange,
config::{ConfRange, Probability},
strings,
strings::{random_strings_with_length, random_strings_with_length_range},
},
Expand Down Expand Up @@ -226,15 +226,15 @@ pub struct TimestampConfig {
///
/// The `DogStatsD` protocol only supports this field for count and gauge metrics.
pub range: ConfRange<u32>,
/// Probability between 0 and 1 that a generated count or gauge metric includes `|T`.
pub probability: f32,
/// Probability that a generated count or gauge metric includes `|T`.
pub probability: Probability,
}

impl Default for TimestampConfig {
fn default() -> Self {
Self {
range: ConfRange::Constant(1),
probability: 0.0,
probability: Probability::try_new(0.0).expect("0.0 is in [0.0, 1.0]"),

@chatgpt-codex-connector chatgpt-codex-connector Bot May 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid introducing an expect in Default

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 👍 / 👎.

@blt blt May 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
Expand All @@ -245,13 +245,7 @@ impl TimestampConfig {
if !range_valid {
return Result::Err(format!("Timestamp range is invalid: {reason}"));
}
if !self.probability.is_finite() || self.probability < 0.0 || self.probability > 1.0 {
return Result::Err(format!(
"Timestamp probability {} must be finite and in range [0.0, 1.0]",
self.probability
));
}
if self.probability > 0.0 && self.range.start() == 0 {
if self.probability.get() > 0.0 && self.range.start() == 0 {
return Result::Err(
"Timestamp range start value cannot be 0 when timestamps are enabled".to_string(),
);
Expand Down Expand Up @@ -450,7 +444,7 @@ impl MemberGenerator {
external_data: Vec<String>,
cardinality: Vec<String>,
timestamp_range: ConfRange<u32>,
timestamp_probability: f32,
timestamp_probability: Probability,
mut rng: &mut R,
) -> Result<Self, crate::Error>
where
Expand Down Expand Up @@ -833,12 +827,16 @@ impl DogStatsD {

#[cfg(test)]
mod test {
use super::{ConfRange, Config, KindWeights, MetricWeights, TimestampConfig};
use super::{ConfRange, Config, KindWeights, MetricWeights, Probability, TimestampConfig};
use proptest::prelude::*;
use rand::{SeedableRng, rngs::SmallRng};

use crate::{DogStatsD, Serialize};

fn prob(value: f32) -> Probability {
Probability::try_new(value).expect("value must be in [0.0, 1.0]")
}

// Generate a batch of raw DogStatsD bytes using the given config and seed.
fn generate_bytes(config: &Config, seed: u64) -> Vec<u8> {
let mut rng = SmallRng::seed_from_u64(seed);
Expand Down Expand Up @@ -942,7 +940,7 @@ mod test {
metric_weights: MetricWeights::new(100, 100, 0, 0, 0, 0),
timestamp: Box::new(TimestampConfig {
range: ConfRange::Constant(1_656_581_400),
probability: 1.0,
probability: prob(1.0),
}),
..Default::default()
};
Expand All @@ -956,7 +954,7 @@ mod test {
metric_weights: MetricWeights::new(100, 100, 0, 0, 0, 0),
timestamp: Box::new(TimestampConfig {
range: ConfRange::Constant(1_656_581_400),
probability: 0.0,
probability: prob(0.0),
}),
..Default::default()
};
Expand All @@ -974,7 +972,7 @@ mod test {
metric_weights: MetricWeights::new(100, 100, 0, 0, 0, 0),
timestamp: Box::new(TimestampConfig {
range: ConfRange::Inclusive { min: 10, max: 20 },
probability: 1.0,
probability: prob(1.0),
}),
..Default::default()
};
Expand All @@ -1001,7 +999,7 @@ mod test {
metric_weights: MetricWeights::new(0, 0, 100, 0, 0, 0),
timestamp: Box::new(TimestampConfig {
range: ConfRange::Constant(1_656_581_400),
probability: 1.0,
probability: prob(1.0),
}),
..Default::default()
};
Expand All @@ -1013,23 +1011,22 @@ mod test {
}

#[test]
fn timestamp_probability_must_be_valid() {
let config = Config {
timestamp: Box::new(TimestampConfig {
probability: 2.0,
..Default::default()
}),
..Default::default()
};
assert!(config.valid().is_err());
fn timestamp_probability_rejects_out_of_range_on_deserialize() {
let yaml = "range: !constant 1\nprobability: 2.0\n";
let err = serde_yaml::from_str::<TimestampConfig>(yaml)
.expect_err("probability 2.0 must be rejected at deserialize time");
assert!(
err.to_string().contains("exceeds 1.0"),
"unexpected error: {err}"
);
}

#[test]
fn timestamp_range_must_be_positive_when_enabled() {
let config = Config {
timestamp: Box::new(TimestampConfig {
range: ConfRange::Constant(0),
probability: 1.0,
probability: prob(1.0),
}),
..Default::default()
};
Expand Down Expand Up @@ -1064,7 +1061,7 @@ mod test {
metric_weights: MetricWeights::new(100, 100, 0, 0, 0, 0),
timestamp: Box::new(TimestampConfig {
range: ConfRange::Inclusive { min: 10, max: 20 },
probability: 0.5,
probability: prob(0.5),
}),
..Default::default()
};
Expand Down
14 changes: 9 additions & 5 deletions lading_payload/src/dogstatsd/metric.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ use rand::{
seq::IteratorRandom,
};

use crate::{Error, Generator, common::strings, dogstatsd::metric::template::Template};
use crate::{
Error, Generator,
common::{config::Probability, strings},
dogstatsd::metric::template::Template,
};
use tracing::debug;

use self::strings::{Pool, choose_or_not_ref};
Expand All @@ -26,7 +30,7 @@ pub(crate) struct MetricGenerator {
pub(crate) external_data: Vec<String>,
pub(crate) cardinality: Vec<String>,
pub(crate) timestamp_range: ConfRange<u32>,
pub(crate) timestamp_probability: f32,
pub(crate) timestamp_probability: Probability,
pub(crate) templates: Vec<template::Template>,
pub(crate) multivalue_count: ConfRange<u16>,
pub(crate) multivalue_pack_probability: f32,
Expand Down Expand Up @@ -54,7 +58,7 @@ impl MetricGenerator {
external_data: Vec<String>,
cardinality: Vec<String>,
timestamp_range: ConfRange<u32>,
timestamp_probability: f32,
timestamp_probability: Probability,
tags_generator: &mut common::tags::Generator,
pools: &StringPools,
value_conf: ValueConf,
Expand Down Expand Up @@ -109,12 +113,12 @@ impl MetricGenerator {
where
R: Rng + ?Sized,
{
if self.timestamp_probability == 0.0 {
if self.timestamp_probability.get() == 0.0 {

@blt blt May 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change needed, vaguely wondering if we should have a is_zero(&Self) -> bool

@goxberry goxberry May 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is needed because I removed the Eq implementation in #1876. I believe there are a bunch of places in the PR stack where .get() could be removed in comparisons if I restore the PartialOrd and Ord trait implementations in #1876.

return None;
}

let prob: f32 = StandardUniform.sample(rng);
if prob < self.timestamp_probability {
if prob < self.timestamp_probability.get() {
Some(self.timestamp_range.sample(rng))
} else {
None
Expand Down
Loading