Skip to content

Commit

Permalink
fix inclusive-exclusiveness of histogram ToString
Browse files Browse the repository at this point in the history
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 facebook#2817

Differential Revision: D5751159

Pulled By: ajkr

fbshipit-source-id: 67432bb45849eec9b5bcc0d095551dbc0ee81766
  • Loading branch information
ajkr authored and facebook-github-bot committed Sep 1, 2017
1 parent 0ec90a7 commit 3b9a000
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 1 deletion.
3 changes: 2 additions & 1 deletion monitoring/histogram.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions monitoring/histogram_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 3b9a000

Please sign in to comment.