From db8b1cda4841f45af22d149c6bfc575e79289f75 Mon Sep 17 00:00:00 2001
From: Florian Forster <octo@collectd.org>
Date: Wed, 12 Oct 2016 09:15:49 +0200
Subject: [PATCH] src/utils_latency.[ch]: get_rate(): Make lower bound
 exclusive.

This has a bunch of benefits:
* You can easily iterate over a range of latencies without counting
  latencies twice. This was previously tricky because both borders were
  considered to be inclusive.
* When lower equals upper, the returned value is now zero.
  Previously, it was a value very close to zero, but not zero. The exact
  value depended on the bucket width, an information not easily
  available to the caller.
---
 src/utils_latency.c      | 35 ++++++++++++++++-------------------
 src/utils_latency.h      |  5 +++--
 src/utils_latency_test.c | 17 ++++++++++++++++-
 3 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/src/utils_latency.c b/src/utils_latency.c
index 24234f83..f5a51e25 100644
--- a/src/utils_latency.c
+++ b/src/utils_latency.c
@@ -293,50 +293,47 @@ cdtime_t latency_counter_get_percentile (latency_counter_t *lc, /* {{{ */
 double latency_counter_get_rate (const latency_counter_t *lc, /* {{{ */
         cdtime_t lower, cdtime_t upper, const cdtime_t now)
 {
-  cdtime_t lower_bin;
-  cdtime_t upper_bin;
-  double sum = 0;
-
   if ((lc == NULL) || (lc->num == 0))
-    return (0);
+    return (NAN);
 
   if (upper && (upper < lower))
+    return (NAN);
+  if (lower == upper)
     return (0);
 
   /* Buckets have an exclusive lower bound and an inclusive upper bound. That
    * means that the first bucket, index 0, represents (0-bin_width]. That means
-   * that lower==bin_width needs to result in lower_bin=0, hence the -1. */
+   * that latency==bin_width needs to result in bin=0, that's why we need to
+   * subtract one before dividing by bin_width. */
+  cdtime_t lower_bin = 0;
   if (lower)
-    lower_bin = (lower - 1) / lc->bin_width;
-  else
-    lower_bin = 0;
+    /* lower is *exclusive* => determine bucket for lower+1 */
+    lower_bin = ((lower+1) - 1) / lc->bin_width;
 
+  /* lower is greater than the longest latency observed => rate is zero. */
+  if (lower_bin >= HISTOGRAM_NUM_BINS)
+    return (0);
+
+  cdtime_t upper_bin = HISTOGRAM_NUM_BINS - 1;
   if (upper)
     upper_bin = (upper - 1) / lc->bin_width;
-  else
-    upper_bin = HISTOGRAM_NUM_BINS - 1;
-
-  if (lower_bin >= HISTOGRAM_NUM_BINS)
-    lower_bin = HISTOGRAM_NUM_BINS - 1;
 
   if (upper_bin >= HISTOGRAM_NUM_BINS) {
     upper_bin = HISTOGRAM_NUM_BINS - 1;
     upper = 0;
   }
 
-  sum = 0;
+  double sum = 0;
   for (size_t i = lower_bin; i <= upper_bin; i++)
-  {
     sum += lc->histogram[i];
-  }
 
   if (lower) {
     /* Approximate ratio of requests in lower_bin, that fall between
      * lower_bin_boundary and lower. This ratio is then subtracted from sum to
      * increase accuracy. */
     cdtime_t lower_bin_boundary = lower_bin * lc->bin_width;
-    assert (lower > lower_bin_boundary);
-    double lower_ratio = (double)(lower - lower_bin_boundary - 1) / ((double) lc->bin_width);
+    assert (lower >= lower_bin_boundary);
+    double lower_ratio = (double)(lower - lower_bin_boundary) / ((double) lc->bin_width);
     sum -= lower_ratio * lc->histogram[lower_bin];
   }
 
diff --git a/src/utils_latency.h b/src/utils_latency.h
index f531d77b..8fdf0249 100644
--- a/src/utils_latency.h
+++ b/src/utils_latency.h
@@ -55,9 +55,10 @@ cdtime_t latency_counter_get_percentile (latency_counter_t *lc,
  *
  * DESCRIPTION
  *   Calculates rate of latency values fall within requested interval.
- *   Interval specified as [lower,upper], i.e. boundaries are inclusive.
+ *   Interval specified as (lower,upper], i.e. the lower boundary is exclusive,
+ *   the upper boundary is inclusive.
  *   When lower is zero, then the interval is (0, upper].
- *   When upper is zero, then the interval is [lower, infinity).
+ *   When upper is zero, then the interval is (lower, infinity).
  */
 double latency_counter_get_rate (const latency_counter_t *lc,
     cdtime_t lower, cdtime_t upper, const cdtime_t now);
diff --git a/src/utils_latency_test.c b/src/utils_latency_test.c
index fe871710..3505ca3e 100644
--- a/src/utils_latency_test.c
+++ b/src/utils_latency_test.c
@@ -192,11 +192,26 @@ DEF_TEST (get_rate) {
       0,
       1.00,
     },
-    { // overflow test
+    { // overflow test: upper >> longest latency
       DOUBLE_TO_CDTIME_T(1.000),
       DOUBLE_TO_CDTIME_T(999999),
       124.00,
     },
+    { // overflow test: lower > longest latency
+      DOUBLE_TO_CDTIME_T(130),
+      0,
+      0.00,
+    },
+    { // lower > upper => error
+      DOUBLE_TO_CDTIME_T(10),
+      DOUBLE_TO_CDTIME_T(9),
+      NAN,
+    },
+    { // lower == upper => zero
+      DOUBLE_TO_CDTIME_T(9),
+      DOUBLE_TO_CDTIME_T(9),
+      0.00,
+    },
   };
 
   for (size_t i = 0; i < STATIC_ARRAY_SIZE(cases); i++) {
-- 
2.11.0