Fix off-by-one for histogram bucket seams

README.md

@@ -457,7 +457,7 @@ for i = bucket_min; i < bucket_max; i++ {
 
 Here `map` is the map from the kernel and `result` is what goes to prometheus.
 
-We take cumulative `count`, because this is what prometheus expects.
+Use `increment_exp2_histogram` in ebpf to observe values.
 
 ##### `exp2zero` histograms
 
@@ -466,12 +466,7 @@ These are the same as `exp2` histograms, except:
 * The first key is for the value `0`
 * All other keys are `1` larger than they should be
 
-This is useful if your actual observed value can be zero, as regular `exp2`
-histograms cannot express this due the the fact that `log2(0)` is invalid,
-and in fact BPF treats `log2(0)` as `0`, and `exp2(0)` is 1, not 0.
-
-See [`tcp-syn-backlog-exp2zero.bpf.c`](examples/tcp-syn-backlog-exp2zero.bpf.c)
-for an example of a config that makes use of this.
+Use `increment_exp2zero_histogram` in ebpf to observe values.
 
 ##### `linear` histograms
 
@@ -509,18 +504,6 @@ information and allowing richer metrics.
 For `fixed` histograms, if `buckets_keys[len(bucket_keys) - 1 ] + 1` contains
 a non-zero value, it will be used as the `sum` key.
 
-##### Advice on values outside of `[bucket_min, bucket_max]`
-
-For both `exp2` and `linear` histograms it is important that kernel does
-not count events into buckets outside of `[bucket_min, bucket_max]` range.
-If you encounter a value above your range, truncate it to be in it. You're
-losing `+Inf` bucket, but usually it's not that big of a deal.
-
-Each kernel map key must count values under that key's value to match
-the behavior of prometheus. For example, `exp2` histogram key `3` should
-count values for `(exp2(2), exp2(3)]` interval: `(4, 8]`. To put it simply:
-use `log2l` or integer division and you'll be good.
-
 ### Labels
 
 Labels transform kernel map keys into prometheus labels.

examples/maps.bpf.h

@@ -1,5 +1,22 @@
 #include "bits.bpf.h"
 
+// Produce values that are usable for prometheus. See:
+// * https://github.com/cloudflare/ebpf_exporter/issues/488
+static __always_inline u64 log2l_histogram(u64 v)
+{
+    u64 rounded = log2l(v);
+
+    if (rounded == 0) {
+        return 0;
+    }
+
+    if (2 << (rounded - 1) == v) {
+        return rounded;
+    } else {
+        return rounded + 1;
+    }
+}
+
 #define lookup_or_zero_init_key(map, key, into)                                                                        \
     u64 zero = 0;                                                                                                      \
                                                                                                                        \
@@ -51,7 +68,7 @@ static inline int increment_map_nosync(void *map, void *key, u64 increment)
     }
 
 #define _increment_ex2_histogram(map, key, increment, max_bucket, increment_fn)                                        \
-    key.bucket = log2l(increment);                                                                                     \
+    key.bucket = log2l_histogram(increment);                                                                           \
                                                                                                                        \
     if (key.bucket > max_bucket) {                                                                                     \
         key.bucket = max_bucket;                                                                                       \
@@ -69,7 +86,7 @@ static inline int increment_map_nosync(void *map, void *key, u64 increment)
     if (increment == 0) {                                                                                              \
         key.bucket = 0;                                                                                                \
     } else {                                                                                                           \
-        key.bucket = log2l(increment) + 1;                                                                             \
+        key.bucket = log2l_histogram(increment) + 1;                                                                   \
     }                                                                                                                  \
                                                                                                                        \
     _increment_histogram(map, key, increment, max_bucket, increment_fn);