From 5c7514134bb4c59f740f30c7fb47e0771d11be6f Mon Sep 17 00:00:00 2001 From: Zakhar Bessarab Date: Fri, 28 Jun 2024 12:43:21 +0400 Subject: [PATCH] app/vmselect/promql: propagate lower bucket values when fixing a histogram In most cases histograms are exposed in sorted manner with lower buckets being first. This means that during scraping buckets with lower bounds have higher chance of being updated earlier than upper ones. Previously, values were propagated from upper to lower bounds, which means that in most cases that would produce results higher than expected once all buckets will become updated. Propagating from upper bound effectively limits highest value of histogram to the value of previous scrape. Once the data will become consistent in the subsequent evaluation this causes spikes in the result. Changing propagation to be from lower to higher buckets reduces value spikes in most cases due to nature of the original inconsistency. See: https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4580 Signed-off-by: Zakhar Bessarab --- app/vmselect/promql/exec_test.go | 2 +- app/vmselect/promql/transform.go | 10 +++++----- app/vmselect/promql/transform_test.go | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index 38ad75185b..593b4d3b94 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -4471,7 +4471,7 @@ func TestExecSuccess(t *testing.T) { ),0.01)` r := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{18.57, 18.57, 18.57, 18.57, 18.57, 18.57}, + Values: []float64{30, 30, 30, 30, 30, 30}, Timestamps: timestampsExpected, } r.MetricName.Tags = []storage.Tag{{ diff --git a/app/vmselect/promql/transform.go b/app/vmselect/promql/transform.go index d88556300c..8dd83850c1 100644 --- a/app/vmselect/promql/transform.go +++ b/app/vmselect/promql/transform.go @@ -1061,12 +1061,12 @@ func fixBrokenBuckets(i int, xss []leTimeseries) { break } } - // Substitute lower bucket values with upper values if the lower values are NaN - // or are bigger than the upper bucket values. - vNext := xss[len(xss)-1].ts.Values[i] - for j := len(xss) - 2; j >= 0; j-- { + // Substitute upper bucket values lower bucket values if the upper values are NaN + // or are bigger than the lower bucket values. + vNext := xss[0].ts.Values[0] + for j := 1; j < len(xss); j++ { v := xss[j].ts.Values[i] - if math.IsNaN(v) || v > vNext { + if math.IsNaN(v) || vNext > v { xss[j].ts.Values[i] = vNext } else { vNext = v diff --git a/app/vmselect/promql/transform_test.go b/app/vmselect/promql/transform_test.go index 6a4b29d01b..ecd5cbacb5 100644 --- a/app/vmselect/promql/transform_test.go +++ b/app/vmselect/promql/transform_test.go @@ -32,11 +32,11 @@ func TestFixBrokenBuckets(t *testing.T) { f(nil, []float64{}) f([]float64{1}, []float64{1}) f([]float64{1, 2}, []float64{1, 2}) - f([]float64{2, 1}, []float64{1, 1}) + f([]float64{2, 1}, []float64{2, 2}) f([]float64{1, 2, 3, nan, nan}, []float64{1, 2, 3, 3, 3}) - f([]float64{5, 1, 2, 3, nan}, []float64{1, 1, 2, 3, 3}) - f([]float64{1, 5, 2, nan, 6, 3}, []float64{1, 2, 2, 3, 3, 3}) - f([]float64{5, 10, 4, 3}, []float64{3, 3, 3, 3}) + f([]float64{5, 1, 2, 3, nan}, []float64{5, 5, 5, 5, 5}) + f([]float64{1, 5, 2, nan, 6, 3}, []float64{1, 5, 5, 5, 6, 6}) + f([]float64{5, 10, 4, 3}, []float64{5, 10, 10, 10}) } func TestVmrangeBucketsToLE(t *testing.T) {