From 98fe1950a1845cf4618c4da470c431afcc21a97e Mon Sep 17 00:00:00 2001 From: Evgeniy Negriy Date: Wed, 6 Nov 2024 19:35:59 +0300 Subject: [PATCH] app/vmselect: fixes graphite function transformRemoveEmptySeries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously it incorrectly applied xFilesFactor, if it's value equal to 0. This commit properly handles this case and returns result according to the graphite documentation: `xFilesFactor follows the same semantics as in Whisper storage schemas. Setting it to 0 (the default) means that only a single value in the series needs to be non-null for it to be considered non-empty, setting it to 1 means that all values ​​in the series must be non-null. A setting of 0.5 means that at least half the values ​​in the series must be non-null.` Signed-off-by: f41gh7 Co-authored-by: Evgeniy Negriy (cherry picked from commit d27dfac5c69e83f085930e5072287266154283aa) --- app/vmselect/graphite/eval_test.go | 19 +++++++++++++++++++ app/vmselect/graphite/transform.go | 2 +- docs/changelog/CHANGELOG.md | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/vmselect/graphite/eval_test.go b/app/vmselect/graphite/eval_test.go index 17f37e075..e6110df67 100644 --- a/app/vmselect/graphite/eval_test.go +++ b/app/vmselect/graphite/eval_test.go @@ -2139,6 +2139,25 @@ func TestExecExprSuccess(t *testing.T) { }, }) f(`removeEmptySeries(removeBelowValue(time('a'),150),1)`, []*series{}) + // if xFilesFactor is set, a single value in the series needs to be non-null for it to be + // considered non-empty + f(`removeEmptySeries(removeBelowValue(time('a'),150),0)`, []*series{ + { + Timestamps: []int64{120000, 180000}, + Values: []float64{nan, 180}, + Name: "removeBelowValue(a,150)", + Tags: map[string]string{"name": "a"}, + }, + }) + f(`removeEmptySeries(removeBelowValue(time('a'),150),-1)`, []*series{ + { + Timestamps: []int64{120000, 180000}, + Values: []float64{nan, 180}, + Name: "removeBelowValue(a,150)", + Tags: map[string]string{"name": "a"}, + }, + }) + f(`round(time('a',17),-1)`, []*series{ { Timestamps: []int64{120000, 137000, 154000, 171000, 188000, 205000}, diff --git a/app/vmselect/graphite/transform.go b/app/vmselect/graphite/transform.go index 66ed47d17..e9d47c983 100644 --- a/app/vmselect/graphite/transform.go +++ b/app/vmselect/graphite/transform.go @@ -3151,7 +3151,7 @@ func transformRemoveEmptySeries(ec *evalConfig, fe *graphiteql.FuncExpr) (nextSe xff = xFilesFactor } n := aggrCount(s.Values) - if n/float64(len(s.Values)) < xff { + if n/float64(len(s.Values)) <= xff { return nil, nil } s.expr = fe diff --git a/docs/changelog/CHANGELOG.md b/docs/changelog/CHANGELOG.md index 59afb5443..218b6c1ed 100644 --- a/docs/changelog/CHANGELOG.md +++ b/docs/changelog/CHANGELOG.md @@ -20,6 +20,7 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/). * BUGFIX: [vmctl](https://docs.victoriametrics.com/vmctl/): drop rows that do not belong to the current series during import. The dropped rows should belong to another series whose tags are a superset of the current series. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/7301) and [this pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/7330). Thanks to @dpedu for reporting and cooperating with the test. * BUGFIX: [vmsingle](https://docs.victoriametrics.com/single-server-victoriametrics/), `vmselect` in [VictoriaMetrics cluster](https://docs.victoriametrics.com/cluster-victoriametrics/): keep the order of resulting time series when `limit_offset` is applied. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/7068). +* BUGFIX: [graphite](https://docs.victoriametrics.com/#graphite-render-api-usage): properly handle xFilesFactor=0 for `transformRemoveEmptySeries` function. See [this PR](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/7337) for details. ## [v1.106.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.106.0)