-
Notifications
You must be signed in to change notification settings - Fork 5.3k
stats: add support for histograms in prometheus export #5601
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
Changes from all commits
b94f79c
5dc8fde
9b6ef02
b4f8ffd
af25859
3f8832b
7663049
631363a
bc52d50
2618f71
3585c2e
4bb2f02
85f9176
3015150
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -14,6 +14,15 @@ HistogramStatisticsImpl::HistogramStatisticsImpl(const histogram_t* histogram_pt | |||
| : computed_quantiles_(supportedQuantiles().size(), 0.0) { | ||||
| hist_approx_quantile(histogram_ptr, supportedQuantiles().data(), supportedQuantiles().size(), | ||||
| computed_quantiles_.data()); | ||||
|
|
||||
| sample_count_ = hist_sample_count(histogram_ptr); | ||||
| sample_sum_ = hist_approx_sum(histogram_ptr); | ||||
|
|
||||
| const std::vector<double>& supported_buckets = supportedBuckets(); | ||||
| computed_buckets_.reserve(supported_buckets.size()); | ||||
| for (const auto bucket : supported_buckets) { | ||||
| computed_buckets_.emplace_back(hist_approx_count_below(histogram_ptr, bucket)); | ||||
| } | ||||
| } | ||||
|
|
||||
| const std::vector<double>& HistogramStatisticsImpl::supportedQuantiles() const { | ||||
|
|
@@ -22,17 +31,33 @@ const std::vector<double>& HistogramStatisticsImpl::supportedQuantiles() const { | |||
| return supported_quantiles; | ||||
| } | ||||
|
|
||||
| std::string HistogramStatisticsImpl::summary() const { | ||||
| const std::vector<double>& HistogramStatisticsImpl::supportedBuckets() const { | ||||
| static const std::vector<double> supported_buckets = { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a better default, but I'm still uncomfortable with hard coding it. I think in the short term, we should allow configuring this via the bootstrap config. I'd prefer to have that in this PR. @htuch any opinion on if the bootstrap config is the correct place for something like this? I think medium or long term, we should allow configuring the buckets either per-histogram, or for each type of histogram (time, bytes, etc).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in agreement, after changing these values and experimenting with it in a set up with a few hundred clusters, the number of metrics was very large, especially when you don't need the same level of range. In other scenarios, you might want the range. I'll make this part of the bootstrap config in this PR as soon as I get a test in for the output. Ultimately we will want different buckets based on type of histogram but not sure if we tag that as present.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, maybe in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One nice thing about Prometheus histograms is the buckets are cumulative. This allows you to use |
||||
| 0.5, 1, 5, 10, 25, 50, 100, 250, 500, 1000, | ||||
| 2500, 5000, 10000, 30000, 60000, 300000, 600000, 1800000, 3600000}; | ||||
| return supported_buckets; | ||||
| } | ||||
|
|
||||
| std::string HistogramStatisticsImpl::quantileSummary() const { | ||||
| std::vector<std::string> summary; | ||||
| const std::vector<double>& supported_quantiles_ref = supportedQuantiles(); | ||||
| summary.reserve(supported_quantiles_ref.size()); | ||||
| for (size_t i = 0; i < supported_quantiles_ref.size(); ++i) { | ||||
| summary.push_back( | ||||
| fmt::format("P{}: {}", 100 * supported_quantiles_ref[i], computed_quantiles_[i])); | ||||
| const std::vector<double>& supported_quantiles = supportedQuantiles(); | ||||
| summary.reserve(supported_quantiles.size()); | ||||
| for (size_t i = 0; i < supported_quantiles.size(); ++i) { | ||||
| summary.push_back(fmt::format("P{}: {}", 100 * supported_quantiles[i], computed_quantiles_[i])); | ||||
| } | ||||
| return absl::StrJoin(summary, ", "); | ||||
| } | ||||
|
|
||||
| std::string HistogramStatisticsImpl::bucketSummary() const { | ||||
| std::vector<std::string> bucket_summary; | ||||
| const std::vector<double>& supported_buckets = supportedBuckets(); | ||||
| bucket_summary.reserve(supported_buckets.size()); | ||||
| for (size_t i = 0; i < supported_buckets.size(); ++i) { | ||||
| bucket_summary.push_back(fmt::format("B{}: {}", supported_buckets[i], computed_buckets_[i])); | ||||
| } | ||||
| return absl::StrJoin(bucket_summary, ", "); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Clears the old computed values and refreshes it with values computed from passed histogram. | ||||
| */ | ||||
|
|
@@ -41,6 +66,17 @@ void HistogramStatisticsImpl::refresh(const histogram_t* new_histogram_ptr) { | |||
| ASSERT(supportedQuantiles().size() == computed_quantiles_.size()); | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add similar |
||||
| hist_approx_quantile(new_histogram_ptr, supportedQuantiles().data(), supportedQuantiles().size(), | ||||
| computed_quantiles_.data()); | ||||
|
|
||||
| sample_count_ = hist_sample_count(new_histogram_ptr); | ||||
| sample_sum_ = hist_approx_sum(new_histogram_ptr); | ||||
|
|
||||
| ASSERT(supportedBuckets().size() == computed_buckets_.size()); | ||||
| computed_buckets_.clear(); | ||||
| const std::vector<double>& supported_buckets = supportedBuckets(); | ||||
| computed_buckets_.reserve(supported_buckets.size()); | ||||
| for (const auto bucket : supported_buckets) { | ||||
| computed_buckets_.emplace_back(hist_approx_count_below(new_histogram_ptr, bucket)); | ||||
| } | ||||
| } | ||||
|
|
||||
| } // namespace Stats | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the approximate sum mean here? If this is not exact, does it need to be documented in the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an implementation detail of
libcircllhistand the way it stores and sums counts of samples across bins. I'll pop a comment on thesampleCountfunction to make it a bit more clearer that it may not be an exact 100% accurate value