From 3b9a000c15792b082d406e002d6fe09a9caa295f Mon Sep 17 00:00:00 2001
From: Andrew Kryczka <andrewkr@fb.com>
Date: Fri, 1 Sep 2017 10:46:26 -0700
Subject: [PATCH] fix inclusive-exclusiveness of histogram ToString

Summary:
I spent too much time thinking about histograms lately and realized boundary values fall into the lower bucket, not the upper bucket. It's because we're using `std::map::lower_bound` here: https://github.com/facebook/rocksdb/blob/867fe92e5e65ce501069aa22c538757acfaade34/monitoring/histogram.cc#L53.  Fixed histogram's `ToString()` to reflect this.
Closes https://github.com/facebook/rocksdb/pull/2817

Differential Revision: D5751159

Pulled By: ajkr

fbshipit-source-id: 67432bb45849eec9b5bcc0d095551dbc0ee81766
---
 monitoring/histogram.cc      |  3 ++-
 monitoring/histogram_test.cc | 13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/monitoring/histogram.cc b/monitoring/histogram.cc
index b3c01a78e..d2b7ff844 100644
--- a/monitoring/histogram.cc
+++ b/monitoring/histogram.cc
@@ -209,7 +209,8 @@ std::string HistogramStat::ToString() const {
     if (bucket_value <= 0.0) continue;
     cumulative_sum += bucket_value;
     snprintf(buf, sizeof(buf),
-             "[ %7" PRIu64 ", %7" PRIu64 " ) %8" PRIu64 " %7.3f%% %7.3f%% ",
+             "%c %7" PRIu64 ", %7" PRIu64 " ] %8" PRIu64 " %7.3f%% %7.3f%% ",
+             (b == 0) ? '[' : '(',
              (b == 0) ? 0 : bucketMapper.BucketLimit(b-1),  // left
               bucketMapper.BucketLimit(b),  // right
               bucket_value,                   // count
diff --git a/monitoring/histogram_test.cc b/monitoring/histogram_test.cc
index b4e3c981c..df58822fc 100644
--- a/monitoring/histogram_test.cc
+++ b/monitoring/histogram_test.cc
@@ -85,6 +85,19 @@ TEST_F(HistogramTest, BasicOperation) {
   BasicOperation(histogramWindowing);
 }
 
+TEST_F(HistogramTest, BoundaryValue) {
+  HistogramImpl histogram;
+  // - both should be in [0, 1] bucket because we place values on bucket
+  //   boundaries in the lower bucket.
+  // - all points are in [0, 1] bucket, so p50 will be 0.5
+  // - the test cannot be written with a single point since histogram won't
+  //   report percentiles lower than the min or greater than the max.
+  histogram.Add(0);
+  histogram.Add(1);
+
+  ASSERT_LE(fabs(histogram.Percentile(50.0) - 0.5), kIota);
+}
+
 TEST_F(HistogramTest, MergeHistogram) {
   HistogramImpl histogram;
   HistogramImpl other;