From 4fde27c905ea9edf694a20918cf8988fecd35c12 Mon Sep 17 00:00:00 2001 From: Ivan Babrou Date: Mon, 20 Jan 2025 11:28:01 -0800 Subject: [PATCH] Fix off-by-one for histogram bucket seams See #488 for more details. With `exp2zero` histogram: * Go: ``` $ go run cmd/derp/main.go poc_metric_bucket{le="0"} 1 poc_metric_bucket{le="1"} 2 poc_metric_bucket{le="2"} 3 poc_metric_bucket{le="4"} 5 poc_metric_bucket{le="8"} 9 poc_metric_bucket{le="+Inf"} 9 poc_metric_sum 36 poc_metric_count 9 ``` * `ebpf_exporter`: ``` ebpf_exporter_poc_exp2zero_values_bucket{le="0"} 1 ebpf_exporter_poc_exp2zero_values_bucket{le="1"} 2 ebpf_exporter_poc_exp2zero_values_bucket{le="2"} 3 ebpf_exporter_poc_exp2zero_values_bucket{le="4"} 5 ebpf_exporter_poc_exp2zero_values_bucket{le="8"} 9 ebpf_exporter_poc_exp2zero_values_bucket{le="+Inf"} 9 ebpf_exporter_poc_exp2zero_values_sum 36 ebpf_exporter_poc_exp2zero_values_count 9 ``` With `exp2` histogram: * Go: ``` $ go run cmd/derp/main.go poc_metric_bucket{le="1"} 1 poc_metric_bucket{le="2"} 2 poc_metric_bucket{le="4"} 4 poc_metric_bucket{le="8"} 8 poc_metric_bucket{le="+Inf"} 8 poc_metric_sum 36 poc_metric_count 8 ``` * `ebpf_exporter` ``` ebpf_exporter_poc_exp2_values_bucket{le="1"} 1 ebpf_exporter_poc_exp2_values_bucket{le="2"} 2 ebpf_exporter_poc_exp2_values_bucket{le="4"} 4 ebpf_exporter_poc_exp2_values_bucket{le="8"} 8 ebpf_exporter_poc_exp2_values_bucket{le="+Inf"} 8 ebpf_exporter_poc_exp2_values_sum 36 ebpf_exporter_poc_exp2_values_count 8 ``` --- README.md | 21 ++------------------- examples/bits.bpf.h | 16 ++++++++++++++++ examples/maps.bpf.h | 4 ++-- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 17ff3e55..ae182c81 100644 --- a/README.md +++ b/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. diff --git a/examples/bits.bpf.h b/examples/bits.bpf.h index ed7e7a8f..50e4e72e 100644 --- a/examples/bits.bpf.h +++ b/examples/bits.bpf.h @@ -27,4 +27,20 @@ static __always_inline u64 log2l(u64 v) return log2(v); } +// 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; +} + #endif /* __BITS_BPF_H */ diff --git a/examples/maps.bpf.h b/examples/maps.bpf.h index 51f1a540..6b051165 100644 --- a/examples/maps.bpf.h +++ b/examples/maps.bpf.h @@ -51,7 +51,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 +69,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);