From 7fb314bb59197648f81a6bd1558608a7e1f10163 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Tue, 20 Dec 2022 00:12:54 -0800 Subject: [PATCH] app/vmselect/promql: do not extend too short lookbehind window for rate() function if it is set explicitly Previously too short lookbehind window d for rate(m[d]) could be automatically extended if it didn't cover at least two raw samples. This was needed in order to guarantee non-empty results from rate(m[d]) on short time ranges. Now the lookbehind window isn't extended if it is set explicitly, since it is expected that the user knows what he is doing. The lookbehind window continues to be extended when needed if it isn't set explicitly. For example, in the case of rate(m). Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3483 --- app/vmselect/promql/exec_test.go | 12 ++++++------ app/vmselect/promql/rollup.go | 22 +++++++++++++++------- docs/CHANGELOG.md | 4 ++++ 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index b669ef8ecf..cd1b6bfeab 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -945,7 +945,7 @@ func TestExecSuccess(t *testing.T) { ))` r := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{nan, nan, nan, 1, nan, nan}, + Values: []float64{nan, nan, 1, 1, nan, nan}, Timestamps: timestampsExpected, } resultExpected := []netstorage.Result{r} @@ -6702,7 +6702,7 @@ func TestExecSuccess(t *testing.T) { q := `rate((2000-time())[100s])` r := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{5.5, 4.5, 3.5, 2.5, 1.5, 0.5}, + Values: []float64{5, 4, 3, 2, 1, 0}, Timestamps: timestampsExpected, } resultExpected := []netstorage.Result{r} @@ -6713,7 +6713,7 @@ func TestExecSuccess(t *testing.T) { q := `rate((2000-time())[100s:])` r := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{5.5, 4.5, 3.5, 2.5, 1.5, 0.5}, + Values: []float64{5, 4, 3, 2, 1, 0}, Timestamps: timestampsExpected, } resultExpected := []netstorage.Result{r} @@ -6724,7 +6724,7 @@ func TestExecSuccess(t *testing.T) { q := `rate((2000-time())[100s:100s])` r := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{0, 0, 6.5, 4.5, 2.5, 0.5}, + Values: []float64{0, 0, 6, 4, 2, 0}, Timestamps: timestampsExpected, } resultExpected := []netstorage.Result{r} @@ -6735,7 +6735,7 @@ func TestExecSuccess(t *testing.T) { q := `rate((2000-time())[100s:100s] offset 100s)` r := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{0, 0, 3.5, 5.5, 3.5, 1.5}, + Values: []float64{0, 0, 7, 5, 3, 1}, Timestamps: timestampsExpected, } resultExpected := []netstorage.Result{r} @@ -6746,7 +6746,7 @@ func TestExecSuccess(t *testing.T) { q := `rate((2000-time())[100s:100s] offset 100s)[:] offset 100s` r := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{0, 0, 0, 3.5, 5.5, 3.5}, + Values: []float64{0, 0, 0, 7, 5, 3}, Timestamps: timestampsExpected, } resultExpected := []netstorage.Result{r} diff --git a/app/vmselect/promql/rollup.go b/app/vmselect/promql/rollup.go index 06603aac27..8acb8aa090 100644 --- a/app/vmselect/promql/rollup.go +++ b/app/vmselect/promql/rollup.go @@ -145,11 +145,11 @@ var rollupAggrFuncs = map[string]rollupFunc{ "zscore_over_time": rollupZScoreOverTime, } -// VictoriaMetrics can increase lookbehind window in square brackets for these functions -// if the given window doesn't contain enough samples for calculations. +// VictoriaMetrics can extends lookbehind window for these functions +// in order to make sure it contains enough points for returning non-empty results. // -// This is needed in order to return the expected non-empty graphs when zooming in the graph in Grafana, -// which is built with `func_name(metric[$__interval])` query. +// This is needed for returning the expected non-empty graphs when zooming in the graph in Grafana, +// which is built with `func_name(metric)` query. var rollupFuncsCanAdjustWindow = map[string]bool{ "default_rollup": true, "deriv": true, @@ -584,15 +584,23 @@ func (rc *rollupConfig) doInternal(dstValues []float64, tsm *timeseriesMap, valu window := rc.Window if window <= 0 { window = rc.Step + if rc.MayAdjustWindow && window < maxPrevInterval { + // Adjust lookbehind window only if it isn't set explicilty, e.g. rate(foo). + // In the case of missing lookbehind window it should be adjusted in order to return non-empty graph + // when the window doesn't cover at least two raw samples (this is what most users expect). + // + // If the user explicitly sets the lookbehind window to some fixed value, e.g. rate(foo[1s]), + // then it is expected he knows what he is doing. Do not adjust the lookbehind window then. + // + // See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3483 + window = maxPrevInterval + } if rc.isDefaultRollup && rc.LookbackDelta > 0 && window > rc.LookbackDelta { // Implicit window exceeds -search.maxStalenessInterval, so limit it to -search.maxStalenessInterval // according to https://github.com/VictoriaMetrics/VictoriaMetrics/issues/784 window = rc.LookbackDelta } } - if rc.MayAdjustWindow && window < maxPrevInterval { - window = maxPrevInterval - } rfa := getRollupFuncArg() rfa.idx = 0 rfa.window = window diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index e24272bd96..c120625af9 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -15,6 +15,10 @@ The following tip changes can be tested by building VictoriaMetrics components f ## tip +**Update note 1:** This and newer releases of VictoriaMetrics may return gaps for `rate(m[d])` queries on short time ranges if `[d]` lookbehind window is set expliticly. For example, `rate(http_requests_total[$__interval])`. This reduces confusion level when the user expects the needed results from the query with explicitly set lookbehind window. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3483). The previous gap filling behaviour can be restored by removing explicit lookbehind window `[d]` from the query, e.g. by substituting the `rate(m[d])` with `rate(m)`. See [these docs](https://docs.victoriametrics.com/MetricsQL.html#implicit-query-conversions) for details. + +* BUGFIX: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): never extend explicitly set lookbehind window for [rate()](https://docs.victoriametrics.com/MetricsQL.html#rate) function. This reduces the level of confusion when the user expects the needed results after explicitly seting the lookbehind window `[d]` in the query `rate(m[d])`. Previously VictoriaMetrics could silently extend the lookbehind window, so it covers at least two raw samples. Now this behavior works only if the lookbehind window in square brackets isn't set explicitly, e.g. in the case of `rate(m)`. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3483) for details. + ## [v1.85.2](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.85.2)