Skip to content

Added debug assertions for negative values in counter and histograms#2757

Merged
jmacd merged 1 commit intoopen-telemetry:mainfrom
marcsnid:marcsnid/debug-assert-negative-counter-histogram
Apr 24, 2026
Merged

Added debug assertions for negative values in counter and histograms#2757
jmacd merged 1 commit intoopen-telemetry:mainfrom
marcsnid:marcsnid/debug-assert-negative-counter-histogram

Conversation

@marcsnid
Copy link
Copy Markdown
Contributor

Change Summary

Add debug_assert! checks to enforce non-negative values in Counter<f64> and Mmsc (Histogram bridge) instruments in otap_df_telemetry. Counters and Histogram-based instruments must only receive non-negative deltas for correctness. Their sums are exported as Prometheus counters, which require monotonicity.

Three guards added:

  • Counter<f64>::add(v) — asserts v >= 0.0
  • AddAssign<f64> for Counter<f64> — asserts rhs >= 0.0
  • Mmsc::record(value) — asserts value >= 0.0

These use debug_assert! (zero cost in release builds) per the issue discussion.

What issue does this PR close?

#2100

How are these changes tested?

  • Replaced the existing test_mmsc_negative_values test (which validated now-invalid behavior) with three #[cfg(debug_assertions)] #[should_panic] tests that verify the assertions fire on negative input:
    • test_mmsc_record_rejects_negative
    • test_counter_f64_add_rejects_negative
    • test_counter_f64_add_assign_rejects_negative
  • Tests in otap-df-telemetry continue to pass.

Are there any user-facing changes?

No. debug_assert! is stripped in release builds. In debug builds, passing a negative value to Counter<f64>::add(), Counter<f64> +=, or Mmsc::record() will now panic with a descriptive message, catching incorrect usage during development.

@marcsnid marcsnid requested a review from a team as a code owner April 24, 2026 22:34
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Apr 24, 2026

impl AddAssign<f64> for Counter<f64> {
fn add_assign(&mut self, rhs: f64) {
debug_assert!(rhs >= 0.0, "Counter += called with negative value: {rhs}");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if this is stripped off by compiler in release build, this is good. I don't know if any better solution than this.

Copy link
Copy Markdown
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Looks good, and welcome!

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.06%. Comparing base (993ba07) to head (ce30547).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2757      +/-   ##
==========================================
- Coverage   88.06%   88.06%   -0.01%     
==========================================
  Files         644      644              
  Lines      246791   246796       +5     
==========================================
- Hits       217340   217332       -8     
- Misses      28927    28940      +13     
  Partials      524      524              
Components Coverage Δ
otap-dataflow 89.64% <100.00%> (-0.01%) ⬇️
query_abstraction 80.61% <ø> (ø)
query_engine 90.75% <ø> (ø)
otel-arrow-go 52.45% <ø> (ø)
quiver 92.25% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lalitb
Copy link
Copy Markdown
Member

lalitb commented Apr 24, 2026

Nice cleanup. One small consistency question: should Counter<f64>::new(...) / From<f64> also reject negative initial values in debug builds? This PR catches negative deltas through add() and +=, but callers can still create an invalid counter directly with Counter::new(-1.0f64) or Counter::::from(-1.0).

@jmacd jmacd added this pull request to the merge queue Apr 24, 2026
@marcsnid
Copy link
Copy Markdown
Contributor Author

Nice cleanup. One small consistency question: should Counter<f64>::new(...) / From<f64> also reject negative initial values in debug builds? This PR catches negative deltas through add() and +=, but callers can still create an invalid counter directly with Counter::new(-1.0f64) or Counter::::from(-1.0).

Good catch! Since this PR is already in the merge queue, I'll address this in a follow-up PR covering Counter::new() and From.

Merged via the queue into open-telemetry:main with commit bc4cec8 Apr 24, 2026
83 of 84 checks passed
@marcsnid marcsnid deleted the marcsnid/debug-assert-negative-counter-histogram branch April 25, 2026 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants