Improve Summary and Histogram API. Add constructors from rvalue references, observe from templated input it

Author: sjanel

core/include/prometheus/histogram.h

@@ -44,7 +44,9 @@ class PROMETHEUS_CPP_CORE_EXPORT Histogram {
   /// exponential etc..
   ///
   /// The bucket boundaries cannot be changed once the histogram is created.
-  Histogram(const BucketBoundaries& buckets);
+  explicit Histogram(const BucketBoundaries& buckets);
+
+  explicit Histogram(BucketBoundaries&& buckets);
 
   /// \brief Observe the given amount.
   ///
@@ -60,15 +62,36 @@ class PROMETHEUS_CPP_CORE_EXPORT Histogram {
   /// this function must have already sorted the values into buckets).
   /// Also increments the total sum of all observations by the given value.
   void ObserveMultiple(const std::vector<double>& bucket_increments,
-                       const double sum_of_values);
+                       double sum_of_values);
+
+  /// Same as above with custom iterator type.
+  template <class InputIt>
+  void ObserveMultiple(InputIt from, InputIt end, double sum_of_values) {
+    std::lock_guard<std::mutex> lock(mutex_);
+    sum_.Increment(sum_of_values);
+
+    for (std::size_t i{0}; i < bucket_counts_.size(); ++i, ++from) {
+      if (from == end) {
+        throw std::length_error(
+            "The size of bucket_increments should be equal to "
+            "the number of buckets in the histogram.");
+      }
+      bucket_counts_[i].Increment(*from);
+    }
+    if (from != end) {
+      throw std::length_error(
+          "The size of bucket_increments should be equal to "
+          "the number of buckets in the histogram.");
+    }
+  }
 
   /// \brief Get the current value of the counter.
   ///
   /// Collect is called by the Registry when collecting metrics.
   ClientMetric Collect() const;
 
  private:
-  const BucketBoundaries bucket_boundaries_;
+  BucketBoundaries bucket_boundaries_;
   mutable std::mutex mutex_;
   std::vector<Counter> bucket_counts_;
   Gauge sum_;

core/include/prometheus/summary.h

@@ -71,9 +71,13 @@ class PROMETHEUS_CPP_CORE_EXPORT Summary {
   /// and how smooth the time window is moved. With only one age bucket it
   /// effectively results in a complete reset of the summary each time max_age
   /// has passed. The default value is 5.
-  Summary(const Quantiles& quantiles,
-          std::chrono::milliseconds max_age = std::chrono::seconds{60},
-          int age_buckets = 5);
+  explicit Summary(const Quantiles& quantiles,
+                   std::chrono::milliseconds max_age = std::chrono::seconds{60},
+                   int age_buckets = 5);
+
+  explicit Summary(Quantiles&& quantiles,
+                   std::chrono::milliseconds max_age = std::chrono::seconds{60},
+                   int age_buckets = 5);
 
   /// \brief Observe the given amount.
   void Observe(double value);
@@ -84,10 +88,10 @@ class PROMETHEUS_CPP_CORE_EXPORT Summary {
   ClientMetric Collect() const;
 
  private:
-  const Quantiles quantiles_;
+  Quantiles quantiles_;
   mutable std::mutex mutex_;
-  std::uint64_t count_;
-  double sum_;
+  std::uint64_t count_ = 0;
+  double sum_ = 0;
   detail::TimeWindowQuantiles quantile_values_;
 };
 

core/src/histogram.cc

@@ -12,26 +12,30 @@
 namespace prometheus {
 
 Histogram::Histogram(const BucketBoundaries& buckets)
-    : bucket_boundaries_{buckets}, bucket_counts_{buckets.size() + 1}, sum_{} {
+    : bucket_boundaries_{buckets}, bucket_counts_{buckets.size() + 1} {
   assert(std::is_sorted(std::begin(bucket_boundaries_),
                         std::end(bucket_boundaries_)));
 }
 
-void Histogram::Observe(const double value) {
-  // TODO: determine bucket list size at which binary search would be faster
-  const auto bucket_index = static_cast<std::size_t>(std::distance(
-      bucket_boundaries_.begin(),
-      std::find_if(
-          std::begin(bucket_boundaries_), std::end(bucket_boundaries_),
-          [value](const double boundary) { return boundary >= value; })));
+Histogram::Histogram(BucketBoundaries&& buckets)
+    : bucket_boundaries_(std::move(buckets)),
+      bucket_counts_{bucket_boundaries_.size() + 1} {
+  assert(std::is_sorted(bucket_boundaries_.begin(), bucket_boundaries_.end()));
+}
+
+void Histogram::Observe(double value) {
+  const auto bucket_index = static_cast<std::size_t>(
+      std::distance(bucket_boundaries_.begin(),
+                    std::lower_bound(bucket_boundaries_.begin(),
+                                     bucket_boundaries_.end(), value)));
 
   std::lock_guard<std::mutex> lock(mutex_);
   sum_.Increment(value);
   bucket_counts_[bucket_index].Increment();
 }
 
 void Histogram::ObserveMultiple(const std::vector<double>& bucket_increments,
-                                const double sum_of_values) {
+                                double sum_of_values) {
   if (bucket_increments.size() != bucket_counts_.size()) {
     throw std::length_error(
         "The size of bucket_increments was not equal to"
@@ -49,13 +53,13 @@ void Histogram::ObserveMultiple(const std::vector<double>& bucket_increments,
 ClientMetric Histogram::Collect() const {
   std::lock_guard<std::mutex> lock(mutex_);
 
-  auto metric = ClientMetric{};
+  ClientMetric metric{};
 
   auto cumulative_count = 0ULL;
   metric.histogram.bucket.reserve(bucket_counts_.size());
   for (std::size_t i{0}; i < bucket_counts_.size(); ++i) {
     cumulative_count += bucket_counts_[i].Value();
-    auto bucket = ClientMetric::Bucket{};
+    ClientMetric::Bucket bucket{};
     bucket.cumulative_count = cumulative_count;
     bucket.upper_bound = (i == bucket_boundaries_.size()
                               ? std::numeric_limits<double>::infinity()

core/src/summary.cc

@@ -7,11 +7,14 @@ namespace prometheus {
 Summary::Summary(const Quantiles& quantiles,
                  const std::chrono::milliseconds max_age, const int age_buckets)
     : quantiles_{quantiles},
-      count_{0},
-      sum_{0},
       quantile_values_{quantiles_, max_age, age_buckets} {}
 
-void Summary::Observe(const double value) {
+Summary::Summary(Quantiles&& quantiles, const std::chrono::milliseconds max_age,
+                 const int age_buckets)
+    : quantiles_(std::move(quantiles)),
+      quantile_values_{quantiles_, max_age, age_buckets} {}
+
+void Summary::Observe(double value) {
   std::lock_guard<std::mutex> lock(mutex_);
 
   count_ += 1;
@@ -20,13 +23,13 @@ void Summary::Observe(const double value) {
 }
 
 ClientMetric Summary::Collect() const {
-  auto metric = ClientMetric{};
+  ClientMetric metric{};
 
   std::lock_guard<std::mutex> lock(mutex_);
 
   metric.summary.quantile.reserve(quantiles_.size());
   for (const auto& quantile : quantiles_) {
-    auto metricQuantile = ClientMetric::Quantile{};
+    ClientMetric::Quantile metricQuantile{};
     metricQuantile.quantile = quantile.quantile;
     metricQuantile.value = quantile_values_.get(quantile.quantile);
     metric.summary.quantile.push_back(std::move(metricQuantile));

core/tests/histogram_test.cc

@@ -2,6 +2,7 @@
 
 #include <gtest/gtest.h>
 
+#include <forward_list>
 #include <limits>
 #include <memory>
 #include <stdexcept>
@@ -81,7 +82,7 @@ TEST(HistogramTest, cumulative_bucket_count) {
   EXPECT_EQ(h.bucket.at(2).cumulative_count, 7U);
 }
 
-TEST(HistogramTest, observe_multiple_test_bucket_counts) {
+TEST(HistogramTest, observe_multiple_test_bucket_counts_1) {
   Histogram histogram{{1, 2}};
   histogram.ObserveMultiple({5, 9, 3}, 20);
   histogram.ObserveMultiple({0, 20, 6}, 34);
@@ -93,6 +94,20 @@ TEST(HistogramTest, observe_multiple_test_bucket_counts) {
   EXPECT_EQ(h.bucket.at(2).cumulative_count, 43U);
 }
 
+TEST(HistogramTest, observe_multiple_test_bucket_counts_2) {
+  Histogram histogram{{1, 2}};
+  const std::forward_list<double> values1 = {5, 9, 3};
+  const std::forward_list<double> values2 = {0, 20, 6};
+  histogram.ObserveMultiple(values1.begin(), values1.end(), 20);
+  histogram.ObserveMultiple(values2.begin(), values2.end(), 34);
+  auto metric = histogram.Collect();
+  auto h = metric.histogram;
+  ASSERT_EQ(h.bucket.size(), 3U);
+  EXPECT_EQ(h.bucket.at(0).cumulative_count, 5U);
+  EXPECT_EQ(h.bucket.at(1).cumulative_count, 34U);
+  EXPECT_EQ(h.bucket.at(2).cumulative_count, 43U);
+}
+
 TEST(HistogramTest, observe_multiple_test_total_sum) {
   Histogram histogram{{1, 2}};
   histogram.ObserveMultiple({5, 9, 3}, 20);
@@ -103,13 +118,23 @@ TEST(HistogramTest, observe_multiple_test_total_sum) {
   EXPECT_EQ(h.sample_sum, 54);
 }
 
-TEST(HistogramTest, observe_multiple_test_length_error) {
+TEST(HistogramTest, observe_multiple_test_length_error1) {
   Histogram histogram{{1, 2}};
   // 2 bucket boundaries means there are 3 buckets, so giving just 2 bucket
   // increments should result in a length_error.
   ASSERT_THROW(histogram.ObserveMultiple({5, 9}, 20), std::length_error);
 }
 
+TEST(HistogramTest, observe_multiple_test_length_error2) {
+  Histogram histogram{{1, 2}};
+  const std::forward_list<double> values1 = {5, 9};
+  ASSERT_THROW(histogram.ObserveMultiple(values1.begin(), values1.end(), 20),
+               std::length_error);
+  const std::forward_list<double> values2 = {5, 9, 5, 6};
+  ASSERT_THROW(histogram.ObserveMultiple(values2.begin(), values2.end(), 20),
+               std::length_error);
+}
+
 TEST(HistogramTest, sum_can_go_down) {
   Histogram histogram{{1}};
   auto metric1 = histogram.Collect();