From f825a9de80f8be3b9721de4c680b08def52b6990 Mon Sep 17 00:00:00 2001 From: Roman Khavronenko Date: Mon, 7 Oct 2024 14:27:50 +0200 Subject: [PATCH] app/vmselect/promql: fix seriesFetched update logic (#7181) ### Describe Your Changes evalInstantRollup could have overreport the number of fetched series if `offset` checks will result into retry. This change updates fetched series only if these checks were successful. It also adds a comment to another potential place of over-reporting series fetched. It doesn't fix it, because it would require spending extra resources on such a check, while discrepancy in seriesFetched doesn't affect calculations in any way. Probably related to https://github.com/VictoriaMetrics/VictoriaMetrics/issues/7170 ### Checklist The following checks are **mandatory**: - [x] My change adheres [VictoriaMetrics contributing guidelines](https://docs.victoriametrics.com/contributing/). Signed-off-by: hagen1778 (cherry picked from commit ebd393d8b334bdec6a938a5d4ff67135bb1dbfaa) Signed-off-by: hagen1778 --- app/vmselect/promql/eval.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/vmselect/promql/eval.go b/app/vmselect/promql/eval.go index 69825a40a2..a27685e49f 100644 --- a/app/vmselect/promql/eval.go +++ b/app/vmselect/promql/eval.go @@ -1123,7 +1123,6 @@ func evalInstantRollup(qt *querytracer.Tracer, ec *EvalConfig, funcName string, again: offset := int64(0) tssCached := rollupResultCacheV.GetInstantValues(qt, at, expr, window, ec.Step, ec.EnforcedTagFilterss) - ec.QueryStats.addSeriesFetched(len(tssCached)) if len(tssCached) == 0 { // Cache miss. Re-populate the missing data. start := int64(fasttime.UnixTimestamp()*1000) - cacheTimestampOffset.Milliseconds() @@ -1167,6 +1166,7 @@ func evalInstantRollup(qt *querytracer.Tracer, ec *EvalConfig, funcName string, deleteCachedSeries(qt) goto again } + ec.QueryStats.addSeriesFetched(len(tssCached)) return tssCached, offset, nil } @@ -1678,6 +1678,10 @@ func evalRollupFuncWithMetricExpr(qt *querytracer.Tracer, ec *EvalConfig, funcNa ecNew = copyEvalConfig(ec) ecNew.Start = start } + // call to evalWithConfig also updates QueryStats.addSeriesFetched + // without checking whether tss has intersection with tssCached. + // So final number could be bigger than actual number of unique series. + // This discrepancy is acceptable, since seriesFetched stat is used as info only. tss, err := evalWithConfig(ecNew) if err != nil { return nil, err