feat(otlp): histogram count limits now configurable - #1913
Conversation
![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: 83fe5ee4e0
ℹ️ 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".
| /// Define the contexts available when generating metrics | ||
| pub contexts: Contexts, | ||
| /// Defines generation limits for histogram count fields. | ||
| pub histogram_count_limits: HistogramCountLimits, |
There was a problem hiding this comment.
Adding this non-defaulted field makes existing Config literal construction non-exhaustive. I checked the repo validation path: ci/clippy runs cargo clippy --all-targets --all-features, which compiles benches, and cargo check --locked --all-features --benches -p lading-payload now fails at lading_payload/benches/opentelemetry_metric.rs:18 and :48 with missing histogram_count_limits, so the validation suite cannot pass until those literals use ..Default::default() or set the new field.
Useful? React with 👍 / 👎.
| .min(self.histogram_count_limits.zero); | ||
| let positive_count = | ||
| point.positive.as_mut().map_or(0, |positive| { | ||
| randomize_exp_bucket_counts(positive, rng) |
There was a problem hiding this comment.
When histogram_count_limits.bucket is configured below 10, delta exponential histograms still call randomize_exp_bucket_counts, whose buckets are sampled from 0..=10 without applying the configured bucket cap. The explicit-histogram delta path and cumulative exponential path do cap buckets, so generated positive and negative exponential bucket counts can exceed the new maximum for this metric kind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I would like to say it's very unlikely that anyone would manually want configure the bucket limit to below 10 when the default is u32::MAX, but nonetheless, a safeguard has been implemented in 28ae4d7
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ae4d723b
ℹ️ 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".
| point.count = checked_count_sum( | ||
| point.bucket_counts.iter().copied(), | ||
| self.histogram_count_limits.total, | ||
| ); |
There was a problem hiding this comment.
When histogram_count_limits.total is configured below the generated bucket sum, this caps point.count while leaving bucket_counts unchanged (for example total: 1 still leaves every explicit bucket sampled from 1..=10). The emitted OTLP histogram then reports a total count that is smaller than sum(bucket_counts), breaking the histogram invariant already asserted later in histograms_populate_min_max; the exponential paths have the same issue for zero_count + positive + negative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It would be preferable to prevent users from setting absurdly low limits or options that break the invariants we've established dc83369
2 participants
What does this PR do?
A brief description of the change being made with this pull request.
Added support for configurable histogram count limits
Motivation
What inspired you to submit this pull request?
A failing CI job when updating the lading version in Agent Data Plane exposed a small bug caused by the Core Agent. Although counts are required to be positive, they are stored as
i64. Previously, lading could produce count sizes up tou64, which would cause overflows, and subsequently, negative numbers. To mitigate this, we now allow configuration of histogram count fields to ensure the invariant thatcount >= 0holds true.Related issues
A list of issues either fixed, containing architectural discussions, otherwise relevant
for this Pull Request.
Additional Notes
Anything else we should know when reviewing?