Skip to content
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.

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

Add record_many to histogram #509

Closed
0xdeafbeef opened this issue Aug 20, 2024 · 2 comments · Fixed by #531
Closed

Add record_many to histogram #509

0xdeafbeef opened this issue Aug 20, 2024 · 2 comments · Fixed by #531
Labels
C-core Component: core functionality such as traits, etc. E-simple Effort: simple. T-ergonomics Type: ergonomics. T-request Type: request.

Comments

@0xdeafbeef
Copy link

I want to reexport tokio metrics, they already provide bucketed data.
To reexport it, you should write such code:

 let hist = metrics::histogram!("tokio_poll_count_time");
  for (idx, value) in histogram.iter().enumerate() {
      let bucket_value = LOG_BUCKETS[idx];
      for _ in 0..*value {
          hist.record(bucket_value);
      }
   }

Is it possible to add

fn record_many(&self, value: f64, count: u64);

to HistogramFn to not burn cpu cycles? Or maybe there is a better way?

@0xdeafbeef
Copy link
Author

0xdeafbeef commented Aug 20, 2024

Maybe even for backward compatibility

pub trait HistogramFn {
    /// Records a value into the histogram.
    fn record(&self, value: f64);

    fn record_many(&self, value: f64, count: u64) {
        for _ in 0..count {
            self.record(value);
        }
    }
}

I can open a pr if you agree with such an interface.

@tobz
Copy link
Member

tobz commented Aug 20, 2024

Overall, I'm fine with the idea of adding HistogramFn::record_many with the proposed method signature. I would definitely want the second version so that we can add it as a non-breaking change, and I would gladly accept a PR for that implementation. 👍🏻

@tobz tobz added C-core Component: core functionality such as traits, etc. E-simple Effort: simple. T-ergonomics Type: ergonomics. T-request Type: request. labels Aug 20, 2024
@tobz tobz closed this as completed in #531 Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-core Component: core functionality such as traits, etc. E-simple Effort: simple. T-ergonomics Type: ergonomics. T-request Type: request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants