Fix histogram overflow errors (#272)

* Recalculate mean on insertion, improve histogram data precision and value capacity

* Format code

Author: Corey-Zumar

src/libclipper/include/clipper/metrics.hpp

@@ -249,9 +249,18 @@ class ReservoirSampler {
   ReservoirSampler(ReservoirSampler &&other) = delete;
   ReservoirSampler &operator=(ReservoirSampler &&other) = delete;
 
-  void sample(const int64_t value);
+  /**
+   * Inserts a value into the reservoir, potentially
+   * removing an old value if the reservoir is at maximum
+   * capacity
+   *
+   * @param value The new value to be inserted
+   * @return The value that was removed, or zero if no value was removed
+   */
+  int64_t sample(const int64_t value);
   void clear();
   const std::vector<int64_t> snapshot() const;
+  size_t current_size() const;
 
  private:
   size_t sample_size_;
@@ -264,17 +273,17 @@ class HistogramStats {
   /** Constructs a HistogramStats object with all values zero **/
   explicit HistogramStats(){};
   explicit HistogramStats(size_t data_size, int64_t min, int64_t max,
-                          double mean, double std_dev, double p50, double p95,
-                          double p99);
+                          long double mean, long double std_dev,
+                          long double p50, long double p95, long double p99);
 
   size_t data_size_ = 0;
   int64_t min_ = 0;
   int64_t max_ = 0;
-  double mean_ = 0;
-  double std_dev_ = 0;
-  double p50_ = 0;
-  double p95_ = 0;
-  double p99_ = 0;
+  long double mean_ = 0;
+  long double std_dev_ = 0;
+  long double p50_ = 0;
+  long double p95_ = 0;
+  long double p99_ = 0;
 };
 
 class Histogram : public Metric {
@@ -290,10 +299,10 @@ class Histogram : public Metric {
 
   void insert(const int64_t value);
   const HistogramStats compute_stats();
-  static double percentile(std::vector<int64_t> snapshot, double rank);
+  static long double percentile(std::vector<int64_t> snapshot, double rank);
   // This method obtains a snapshot from the histogram's reservoir sampler
   // and then calculates the percentile
-  double percentile(double rank);
+  long double percentile(double rank);
 
   // Metric implementation
   MetricType type() const override;
@@ -303,8 +312,20 @@ class Histogram : public Metric {
   void clear() override;
 
  private:
+  /**
+   * @param old_reservoir_size The old size of the reservoir
+   * @param new_reservoir_size The new size of the reservoir
+   * @param new_value The value that was just inserted into the reservoir
+   * @param old_value The value that was just removed from the reservoir (use
+   * zero if nothing was removed)
+   */
+  void update_mean(const size_t old_reservoir_size,
+                   const size_t new_reservoir_size, const int64_t new_value,
+                   const int64_t old_value);
+
   std::string name_;
   std::string unit_;
+  long double mean_ = 0;
   ReservoirSampler sampler_;
   std::mutex sampler_lock_;
 };

src/libclipper/src/metrics.cpp

@@ -401,7 +401,8 @@ void Meter::clear() {
 ReservoirSampler::ReservoirSampler(size_t sample_size)
     : sample_size_(sample_size) {}
 
-void ReservoirSampler::sample(const int64_t value) {
+int64_t ReservoirSampler::sample(const int64_t value) {
+  int64_t replaced_value = 0;
   if (n_ < sample_size_) {
     reservoir_.push_back(value);
   } else {
@@ -410,12 +411,16 @@ void ReservoirSampler::sample(const int64_t value) {
     }
     size_t j = rand() % (n_ + 1);
     if (j < sample_size_) {
+      replaced_value = reservoir_[j];
       reservoir_[j] = value;
     }
   }
   n_++;
+  return replaced_value;
 }
 
+size_t ReservoirSampler::current_size() const { return reservoir_.size(); }
+
 const std::vector<int64_t> ReservoirSampler::snapshot() const {
   return reservoir_;
 }
@@ -426,8 +431,9 @@ void ReservoirSampler::clear() {
 }
 
 HistogramStats::HistogramStats(size_t data_size, int64_t min, int64_t max,
-                               double mean, double std_dev, double p50,
-                               double p95, double p99)
+                               long double mean, long double std_dev,
+                               long double p50, long double p95,
+                               long double p99)
     : data_size_(data_size),
       min_(min),
       max_(max),
@@ -443,10 +449,21 @@ Histogram::Histogram(const std::string name, const std::string unit,
 
 void Histogram::insert(const int64_t value) {
   std::lock_guard<std::mutex> guard(sampler_lock_);
-  sampler_.sample(value);
+  size_t old_reservoir_size = sampler_.current_size();
+  int64_t replaced_value = sampler_.sample(value);
+  size_t new_reservoir_size = sampler_.current_size();
+  update_mean(old_reservoir_size, new_reservoir_size, value, replaced_value);
+}
+
+void Histogram::update_mean(const size_t old_reservoir_size,
+                            const size_t new_reservoir_size,
+                            const int64_t new_value, const int64_t old_value) {
+  int64_t old_sum = static_cast<int64_t>(mean_ * old_reservoir_size);
+  mean_ = static_cast<long double>(old_sum - old_value + new_value) /
+          static_cast<long double>(new_reservoir_size);
 }
 
-double Histogram::percentile(std::vector<int64_t> snapshot, double rank) {
+long double Histogram::percentile(std::vector<int64_t> snapshot, double rank) {
   if (rank < 0 || rank > 1) {
     throw std::invalid_argument("Percentile rank must be in [0,1]!");
   }
@@ -468,7 +485,7 @@ double Histogram::percentile(std::vector<int64_t> snapshot, double rank) {
     x = sample_size;
   }
   size_t index = std::floor(x) - 1;
-  double v = snapshot[index];
+  long double v = snapshot[index];
   double remainder = x - std::floor(x);
   if (remainder == 0) {
     return v;
@@ -477,7 +494,7 @@ double Histogram::percentile(std::vector<int64_t> snapshot, double rank) {
   }
 }
 
-double Histogram::percentile(double rank) {
+long double Histogram::percentile(double rank) {
   std::lock_guard<std::mutex> guard(sampler_lock_);
   std::vector<int64_t> snapshot = sampler_.snapshot();
   if (snapshot.size() == 0) {
@@ -499,22 +516,19 @@ const HistogramStats Histogram::compute_stats() {
   std::sort(snapshot.begin(), snapshot.end());
   int64_t min = snapshot.front();
   int64_t max = snapshot.back();
-  double p50 = percentile(snapshot, .5);
-  double p95 = percentile(snapshot, .95);
-  double p99 = percentile(snapshot, .99);
-  double mean = static_cast<double>(
-                    std::accumulate(snapshot.begin(), snapshot.end(), 0)) /
-                static_cast<double>(snapshot_size);
-  double var = 0;
+  long double p50 = percentile(snapshot, .5);
+  long double p95 = percentile(snapshot, .95);
+  long double p99 = percentile(snapshot, .99);
+  long double var = 0;
   if (snapshot_size > 1) {
     for (auto elem : snapshot) {
-      double incr = std::pow((static_cast<double>(elem) - mean), 2);
+      long double incr = std::pow(elem - mean_, static_cast<long double>(2));
       var += incr;
     }
     var = var / static_cast<double>(snapshot_size);
   }
-  double std_dev = std::sqrt(var);
-  return HistogramStats(snapshot_size, min, max, mean, std_dev, p50, p95, p99);
+  long double std_dev = std::sqrt(var);
+  return HistogramStats(snapshot_size, min, max, mean_, std_dev, p50, p95, p99);
 }
 
 MetricType Histogram::type() const { return MetricType::Histogram; }

src/libclipper/test/metrics_test.cpp

@@ -1,4 +1,5 @@
 #include <cmath>
+#include <limits>
 #include <string>
 
 #include <gtest/gtest.h>
@@ -117,7 +118,7 @@ TEST(MetricsTests, HistogramPercentileFunctionCorrectness) {
   ASSERT_EQ(Histogram::percentile(vec4, p5), Histogram::percentile(vec5, p5));
 }
 
-TEST(MetricsTests, HistogramStatsCorrectness) {
+TEST(MetricsTests, HistogramStatsAreCorrectWithNumericallySmallElements) {
   int64_t arr[] = {16, 53, 104, 113, 185, 202};
   size_t sample_size = 6;
   Histogram histogram("Test Histogram", "milliseconds", sample_size);
@@ -134,6 +135,23 @@ TEST(MetricsTests, HistogramStatsCorrectness) {
   ASSERT_EQ(stats.p99_, 202);
 }
 
+TEST(MetricsTests, HistogramMinMaxMeanAreCorrectWithNumericallyLargeElements) {
+  size_t sample_size = 6;
+  Histogram histogram("Test Histogram", "milliseconds", sample_size);
+  long max_long = std::numeric_limits<long>::max();
+  int64_t sum = 0;
+  for (int i = 0; i < sample_size; i++) {
+    histogram.insert(max_long - i);
+    sum += max_long - i;
+  }
+  long double expected_mean =
+      static_cast<long double>(sum) / static_cast<long double>(sample_size);
+  HistogramStats stats = histogram.compute_stats();
+  ASSERT_DOUBLE_EQ(stats.mean_, expected_mean);
+  ASSERT_DOUBLE_EQ(stats.max_, max_long);
+  ASSERT_DOUBLE_EQ(stats.min_, max_long - sample_size + 1);
+}
+
 TEST(MetricsTests, MetricsRegistryReportingFormatCorrectness) {
   MetricsRegistry &registry = MetricsRegistry::get_metrics();
   const std::string hist1_name("Hist 1");
@@ -177,23 +195,24 @@ TEST(MetricsTests, MetricsRegistryReportingFormatCorrectness) {
   ASSERT_EQ(hist1_tree.get<size_t>("size"), hist1_stats.data_size_);
   ASSERT_EQ(hist1_tree.get<int64_t>("min"), hist1_stats.min_);
   ASSERT_EQ(hist1_tree.get<int64_t>("max"), hist1_stats.max_);
-  ASSERT_EQ(hist1_tree.get<double>("mean"), hist1_stats.mean_);
-  ASSERT_EQ(hist1_tree.get<double>("std_dev"), hist1_stats.std_dev_);
-  ASSERT_EQ(hist1_tree.get<double>("p50"), hist1_stats.p50_);
-  ASSERT_EQ(hist1_tree.get<double>("p95"), hist1_stats.p95_);
-  ASSERT_EQ(hist1_tree.get<double>("p99"), hist1_stats.p99_);
+  ASSERT_DOUBLE_EQ(hist1_tree.get<long double>("mean"), hist1_stats.mean_);
+  ASSERT_EQ(hist1_tree.get<long double>("std_dev"), hist1_stats.std_dev_);
+  ASSERT_EQ(hist1_tree.get<long double>("p50"), hist1_stats.p50_);
+  ASSERT_EQ(hist1_tree.get<long double>("p95"), hist1_stats.p95_);
+  ASSERT_EQ(hist1_tree.get<long double>("p99"), hist1_stats.p99_);
 
   boost::property_tree::ptree hist2_tree =
       hists_tree.back().second.get_child(hist2_name);
   HistogramStats hist2_stats = hist2->compute_stats();
   ASSERT_EQ(hist2_tree.get<size_t>("size"), hist2_stats.data_size_);
   ASSERT_EQ(hist2_tree.get<int64_t>("min"), hist2_stats.min_);
   ASSERT_EQ(hist2_tree.get<int64_t>("max"), hist2_stats.max_);
-  ASSERT_EQ(hist2_tree.get<double>("mean"), hist2_stats.mean_);
-  ASSERT_EQ(hist2_tree.get<double>("std_dev"), hist2_stats.std_dev_);
-  ASSERT_EQ(hist2_tree.get<double>("p50"), hist2_stats.p50_);
-  ASSERT_EQ(hist2_tree.get<double>("p95"), hist2_stats.p95_);
-  ASSERT_EQ(hist2_tree.get<double>("p99"), hist2_stats.p99_);
+  ASSERT_DOUBLE_EQ(hist2_tree.get<long double>("mean"), hist2_stats.mean_);
+  ASSERT_DOUBLE_EQ(hist2_tree.get<long double>("std_dev"),
+                   hist2_stats.std_dev_);
+  ASSERT_DOUBLE_EQ(hist2_tree.get<long double>("p50"), hist2_stats.p50_);
+  ASSERT_DOUBLE_EQ(hist2_tree.get<long double>("p95"), hist2_stats.p95_);
+  ASSERT_DOUBLE_EQ(hist2_tree.get<long double>("p99"), hist2_stats.p99_);
 
   boost::property_tree::ptree counters_tree =
       report_tree.get_child(get_metrics_category_name(MetricType::Counter));