From a6eacfdb11ddf44359fb52be8042d44356c90681 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Wed, 21 Feb 2024 18:47:49 +0200 Subject: [PATCH] app/vmselect/promql: move needSilenceIntervalForRollupFunc from eval.go to rollup.go This should improve maintainability of the code related to rollup functions, since it is located in rollup.go While at it, properly return empty results from holt_winters(), rate_over_sum(), sum2_over_time(), geomean_over_time() and distinct_over_time() when there are no real samples on the selected lookbehind window. Previously the previous sample value was mistakenly returned from these functions. --- app/vmselect/promql/eval.go | 52 +--------------------------------- app/vmselect/promql/rollup.go | 53 +++++++++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 63 deletions(-) diff --git a/app/vmselect/promql/eval.go b/app/vmselect/promql/eval.go index dbb8446f6..603cb6991 100644 --- a/app/vmselect/promql/eval.go +++ b/app/vmselect/promql/eval.go @@ -1687,7 +1687,7 @@ func evalRollupFuncNoCache(qt *querytracer.Tracer, ec *EvalConfig, funcName stri tfss := searchutils.ToTagFilterss(me.LabelFilterss) tfss = searchutils.JoinTagFilterss(tfss, ec.EnforcedTagFilterss) minTimestamp := ec.Start - if needSilenceIntervalForRollupFunc(funcName) { + if needSilenceIntervalForRollupFunc[funcName] { minTimestamp -= maxSilenceInterval() } if window > ec.Step { @@ -1788,56 +1788,6 @@ func maxSilenceInterval() int64 { return d } -func needSilenceIntervalForRollupFunc(funcName string) bool { - // All the rollup functions, which do not rely on the previous sample - // before the lookbehind window (aka prevValue and realPrevValue), do not need silence interval. - switch strings.ToLower(funcName) { - case "default_rollup": - // The default_rollup implicitly relies on the previous samples in order to fill gaps. - // See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5388 - return true - case - "absent_over_time", - "avg_over_time", - "count_eq_over_time", - "count_gt_over_time", - "count_le_over_time", - "count_ne_over_time", - "count_over_time", - "first_over_time", - "histogram_over_time", - "hoeffding_bound_lower", - "hoeffding_bound_upper", - "last_over_time", - "mad_over_time", - "max_over_time", - "median_over_time", - "min_over_time", - "predict_linear", - "present_over_time", - "quantile_over_time", - "quantiles_over_time", - "range_over_time", - "share_gt_over_time", - "share_le_over_time", - "share_eq_over_time", - "stale_samples_over_time", - "stddev_over_time", - "stdvar_over_time", - "sum_over_time", - "tfirst_over_time", - "timestamp", - "timestamp_with_name", - "tlast_over_time", - "tmax_over_time", - "tmin_over_time", - "zscore_over_time": - return false - default: - return true - } -} - func evalRollupWithIncrementalAggregate(qt *querytracer.Tracer, funcName string, keepMetricNames bool, iafc *incrementalAggrFuncContext, rss *netstorage.Results, rcs []*rollupConfig, preFunc func(values []float64, timestamps []int64), sharedTimestamps []int64) ([]*timeseries, error) { diff --git a/app/vmselect/promql/rollup.go b/app/vmselect/promql/rollup.go index e58a8763e..52b0a69e5 100644 --- a/app/vmselect/promql/rollup.go +++ b/app/vmselect/promql/rollup.go @@ -103,6 +103,42 @@ var rollupFuncs = map[string]newRollupFunc{ "zscore_over_time": newRollupFuncOneArg(rollupZScoreOverTime), } +// Functions, which need the previous sample before the lookbehind window for proper calculations. +// +// All the rollup functions, which do not rely on the previous sample +// before the lookbehind window (aka prevValue and realPrevValue), do not need silence interval. +var needSilenceIntervalForRollupFunc = map[string]bool{ + "ascent_over_time": true, + "changes": true, + "decreases_over_time": true, + // The default_rollup implicitly relies on the previous samples in order to fill gaps. + // See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5388 + "default_rollup": true, + "delta": true, + "deriv_fast": true, + "descent_over_time": true, + "idelta": true, + "ideriv": true, + "increase": true, + "increase_pure": true, + "increases_over_time": true, + "integrate": true, + "irate": true, + "lag": true, + "lifetime": true, + "rate": true, + "resets": true, + "rollup": true, + "rollup_candlestick": true, + "rollup_delta": true, + "rollup_deriv": true, + "rollup_increase": true, + "rollup_rate": true, + "rollup_scrape_interval": true, + "scrape_interval": true, + "tlast_change_over_time": true, +} + // rollupAggrFuncs are functions that can be passed to `aggr_over_time()` var rollupAggrFuncs = map[string]rollupFunc{ "absent_over_time": rollupAbsent, @@ -957,7 +993,7 @@ func newRollupHoltWinters(args []interface{}) (rollupFunc, error) { // before calling rollup funcs. values := rfa.values if len(values) == 0 { - return rfa.prevValue + return nan } sf := sfs[rfa.idx] if sf < 0 || sf > 1 { @@ -1586,11 +1622,7 @@ func rollupRateOverSum(rfa *rollupFuncArg) float64 { // before calling rollup funcs. timestamps := rfa.timestamps if len(timestamps) == 0 { - if math.IsNaN(rfa.prevValue) { - return nan - } - // Assume that the value didn't change since rfa.prevValue. - return 0 + return nan } sum := float64(0) for _, v := range rfa.values { @@ -1610,7 +1642,7 @@ func rollupSum2(rfa *rollupFuncArg) float64 { // before calling rollup funcs. values := rfa.values if len(values) == 0 { - return rfa.prevValue * rfa.prevValue + return nan } var sum2 float64 for _, v := range values { @@ -1624,7 +1656,7 @@ func rollupGeomean(rfa *rollupFuncArg) float64 { // before calling rollup funcs. values := rfa.values if len(values) == 0 { - return rfa.prevValue + return nan } p := 1.0 for _, v := range values { @@ -2268,10 +2300,7 @@ func rollupDistinct(rfa *rollupFuncArg) float64 { // before calling rollup funcs. values := rfa.values if len(values) == 0 { - if math.IsNaN(rfa.prevValue) { - return nan - } - return 0 + return nan } m := make(map[float64]struct{}) for _, v := range values {