From 3770d6b74bcbe96cfa258cb8db411067e209720b Mon Sep 17 00:00:00 2001
From: Peter Dillinger <peterd@fb.com>
Date: Thu, 1 Sep 2022 17:46:30 -0700
Subject: [PATCH] Fix possible NaN StandardDeviation in Histogram (#10586)

Summary:
Appears possible after 5de98f2 introduced possible lost
updates. Could be related to 2af132c also. Simply ensure no sqrt of
negative.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/10586

Test Plan: test added

Reviewed By: ajkr

Differential Revision: D39068391

Pulled By: pdillinger

fbshipit-source-id: 230b214a41e6c9ae91a1ef3e8b2a17b46bbb17c2
---
 monitoring/histogram.cc      |  6 ++++--
 monitoring/histogram.h       |  2 ++
 monitoring/histogram_test.cc | 16 +++++++++++++++-
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/monitoring/histogram.cc b/monitoring/histogram.cc
index 331796e5b..4d00219c9 100644
--- a/monitoring/histogram.cc
+++ b/monitoring/histogram.cc
@@ -168,10 +168,12 @@ double HistogramStat::StandardDeviation() const {
       static_cast<double>(num());  // Use double to avoid integer overflow
   double cur_sum = static_cast<double>(sum());
   double cur_sum_squares = static_cast<double>(sum_squares());
-  if (cur_num == 0) return 0;
+  if (cur_num == 0.0) {
+    return 0.0;
+  }
   double variance =
       (cur_sum_squares * cur_num - cur_sum * cur_sum) / (cur_num * cur_num);
-  return std::sqrt(variance);
+  return std::sqrt(std::max(variance, 0.0));
 }
 
 std::string HistogramStat::ToString() const {
diff --git a/monitoring/histogram.h b/monitoring/histogram.h
index 6d72b0651..41af01442 100644
--- a/monitoring/histogram.h
+++ b/monitoring/histogram.h
@@ -140,6 +140,8 @@ class HistogramImpl : public Histogram {
 
   virtual ~HistogramImpl() {}
 
+  inline HistogramStat& TEST_GetStats() { return stats_; }
+
  private:
   HistogramStat stats_;
   std::mutex mutex_;
diff --git a/monitoring/histogram_test.cc b/monitoring/histogram_test.cc
index 7b63dc93a..1ed561d44 100644
--- a/monitoring/histogram_test.cc
+++ b/monitoring/histogram_test.cc
@@ -147,7 +147,8 @@ TEST_F(HistogramTest, HistogramWindowingExpire) {
   ASSERT_EQ(histogramWindowing.num(), 100);
   ASSERT_EQ(histogramWindowing.min(), 1);
   ASSERT_EQ(histogramWindowing.max(), 1);
-  ASSERT_EQ(histogramWindowing.Average(), 1);
+  ASSERT_EQ(histogramWindowing.Average(), 1.0);
+  ASSERT_EQ(histogramWindowing.StandardDeviation(), 0.0);
 
   PopulateHistogram(histogramWindowing, 2, 2, 100);
   clock->SleepForMicroseconds(micros_per_window);
@@ -155,6 +156,7 @@ TEST_F(HistogramTest, HistogramWindowingExpire) {
   ASSERT_EQ(histogramWindowing.min(), 1);
   ASSERT_EQ(histogramWindowing.max(), 2);
   ASSERT_EQ(histogramWindowing.Average(), 1.5);
+  ASSERT_GT(histogramWindowing.StandardDeviation(), 0.0);
 
   PopulateHistogram(histogramWindowing, 3, 3, 100);
   clock->SleepForMicroseconds(micros_per_window);
@@ -162,6 +164,7 @@ TEST_F(HistogramTest, HistogramWindowingExpire) {
   ASSERT_EQ(histogramWindowing.min(), 1);
   ASSERT_EQ(histogramWindowing.max(), 3);
   ASSERT_EQ(histogramWindowing.Average(), 2.0);
+  ASSERT_GT(histogramWindowing.StandardDeviation(), 0.0);
 
   // dropping oldest window with value 1, remaining 2 ~ 4
   PopulateHistogram(histogramWindowing, 4, 4, 100);
@@ -170,6 +173,7 @@ TEST_F(HistogramTest, HistogramWindowingExpire) {
   ASSERT_EQ(histogramWindowing.min(), 2);
   ASSERT_EQ(histogramWindowing.max(), 4);
   ASSERT_EQ(histogramWindowing.Average(), 3.0);
+  ASSERT_GT(histogramWindowing.StandardDeviation(), 0.0);
 
   // dropping oldest window with value 2, remaining 3 ~ 5
   PopulateHistogram(histogramWindowing, 5, 5, 100);
@@ -178,6 +182,7 @@ TEST_F(HistogramTest, HistogramWindowingExpire) {
   ASSERT_EQ(histogramWindowing.min(), 3);
   ASSERT_EQ(histogramWindowing.max(), 5);
   ASSERT_EQ(histogramWindowing.Average(), 4.0);
+  ASSERT_GT(histogramWindowing.StandardDeviation(), 0.0);
 }
 
 TEST_F(HistogramTest, HistogramWindowingMerge) {
@@ -231,6 +236,15 @@ TEST_F(HistogramTest, LargeStandardDeviation) {
   ASSERT_LT(fabs(histogram.StandardDeviation() - 288675), 1);
 }
 
+TEST_F(HistogramTest, LostUpdateStandardDeviation) {
+  HistogramImpl histogram;
+  PopulateHistogram(histogram, 100, 100, 100);
+  // Simulate a possible lost update (since they are not atomic)
+  histogram.TEST_GetStats().sum_squares_ -= 10000;
+  // Ideally zero, but should never be negative or NaN
+  ASSERT_GE(histogram.StandardDeviation(), 0.0);
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {