From 16fb128bbcd0594e30bbe3bfe1ed81edea27c338 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Sat, 11 Jan 2020 00:24:31 +0200 Subject: [PATCH] app/vmselect/promql: do not take into account the previous point before time window in square brackets for `min_over_time`, `max_over_time`, `rollup_first` and `rollup_last` functions This makes the behaviour for these functions similar to Prometheus when processing broken time series with irregular data points like `gitlab_runner_jobs`. See https://gitlab.com/gitlab-org/gitlab-exporter/issues/50 for details. --- app/vmselect/promql/exec_test.go | 18 ++++----- app/vmselect/promql/rollup.go | 63 +++++++++++++----------------- app/vmselect/promql/rollup_test.go | 16 ++++---- 3 files changed, 45 insertions(+), 52 deletions(-) diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index 4c613fe870..f7f40eecb1 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -4519,7 +4519,7 @@ func TestExecSuccess(t *testing.T) { }} r2 := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{0.9, 0.9, 1, 0.9, 1, 0.9}, + Values: []float64{0.8, 0.9, 1, 0.9, 1, 0.9}, Timestamps: timestampsExpected, } r2.MetricName.Tags = []storage.Tag{{ @@ -4543,7 +4543,7 @@ func TestExecSuccess(t *testing.T) { q := `avg(aggr_over_time(("min_over_time", "max_over_time"), time()[:10s]))` r := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{900, 1100, 1300, 1500, 1700, 1900}, + Values: []float64{905, 1105, 1305, 1505, 1705, 1905}, Timestamps: timestampsExpected, } resultExpected := []netstorage.Result{r} @@ -4554,7 +4554,7 @@ func TestExecSuccess(t *testing.T) { q := `sort(avg(aggr_over_time(("min_over_time", "max_over_time"), time()[:10s])) by (rollup))` r1 := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{800, 1000, 1200, 1400, 1600, 1800}, + Values: []float64{810, 1010, 1210, 1410, 1610, 1810}, Timestamps: timestampsExpected, } r1.MetricName.Tags = []storage.Tag{{ @@ -4587,25 +4587,25 @@ func TestExecSuccess(t *testing.T) { }} r2 := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{0.32, 0.82, 0.13, 0.28, 0.86, 0.57}, + Values: []float64{0.85, 0.15, 0.43, 0.76, 0.47, 0.21}, Timestamps: timestampsExpected, } r2.MetricName.Tags = []storage.Tag{{ Key: []byte("rollup"), - Value: []byte("close"), + Value: []byte("open"), }} r3 := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{0.9, 0.32, 0.82, 0.13, 0.28, 0.86}, + Values: []float64{0.32, 0.82, 0.13, 0.28, 0.86, 0.57}, Timestamps: timestampsExpected, } r3.MetricName.Tags = []storage.Tag{{ Key: []byte("rollup"), - Value: []byte("open"), + Value: []byte("close"), }} r4 := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{0.9, 0.94, 0.97, 0.93, 0.98, 0.92}, + Values: []float64{0.85, 0.94, 0.97, 0.93, 0.98, 0.92}, Timestamps: timestampsExpected, } r4.MetricName.Tags = []storage.Tag{{ @@ -4653,7 +4653,7 @@ func TestExecSuccess(t *testing.T) { q := `sort(rollup(time()[:50s]))` r1 := netstorage.Result{ MetricName: metricNameExpected, - Values: []float64{800, 1000, 1200, 1400, 1600, 1800}, + Values: []float64{850, 1050, 1250, 1450, 1650, 1850}, Timestamps: timestampsExpected, } r1.MetricName.Tags = []storage.Tag{{ diff --git a/app/vmselect/promql/rollup.go b/app/vmselect/promql/rollup.go index 385f702895..3c466be867 100644 --- a/app/vmselect/promql/rollup.go +++ b/app/vmselect/promql/rollup.go @@ -114,6 +114,7 @@ var rollupFuncsMayAdjustWindow = map[string]bool{ "irate": true, "rate": true, "lifetime": true, + "lag": true, "scrape_interval": true, } @@ -884,14 +885,14 @@ func rollupAvg(rfa *rollupFuncArg) float64 { func rollupMin(rfa *rollupFuncArg) float64 { // There is no need in handling NaNs here, since they must be cleaned up // before calling rollup funcs. - minValue := rfa.prevValue values := rfa.values - if math.IsNaN(minValue) { - if len(values) == 0 { - return nan - } - minValue = values[0] + if len(values) == 0 { + // Do not take into account rfa.prevValue, since it may lead + // to inconsistent results comparing to Prometheus on broken time series + // with irregular data points. + return nan } + minValue := values[0] for _, v := range values { if v < minValue { minValue = v @@ -903,14 +904,14 @@ func rollupMin(rfa *rollupFuncArg) float64 { func rollupMax(rfa *rollupFuncArg) float64 { // There is no need in handling NaNs here, since they must be cleaned up // before calling rollup funcs. - maxValue := rfa.prevValue values := rfa.values - if math.IsNaN(maxValue) { - if len(values) == 0 { - return nan - } - maxValue = values[0] + if len(values) == 0 { + // Do not take into account rfa.prevValue, since it may lead + // to inconsistent results comparing to Prometheus on broken time series + // with irregular data points. + return nan } + maxValue := values[0] for _, v := range values { if v > maxValue { maxValue = v @@ -922,17 +923,13 @@ func rollupMax(rfa *rollupFuncArg) float64 { func rollupTmin(rfa *rollupFuncArg) float64 { // There is no need in handling NaNs here, since they must be cleaned up // before calling rollup funcs. - minValue := rfa.prevValue - minTimestamp := rfa.prevTimestamp values := rfa.values timestamps := rfa.timestamps - if math.IsNaN(minValue) { - if len(values) == 0 { - return nan - } - minValue = values[0] - minTimestamp = timestamps[0] + if len(values) == 0 { + return nan } + minValue := values[0] + minTimestamp := timestamps[0] for i, v := range values { if v < minValue { minValue = v @@ -945,17 +942,13 @@ func rollupTmin(rfa *rollupFuncArg) float64 { func rollupTmax(rfa *rollupFuncArg) float64 { // There is no need in handling NaNs here, since they must be cleaned up // before calling rollup funcs. - maxValue := rfa.prevValue - maxTimestamp := rfa.prevTimestamp values := rfa.values timestamps := rfa.timestamps - if math.IsNaN(maxValue) { - if len(values) == 0 { - return nan - } - maxValue = values[0] - maxTimestamp = timestamps[0] + if len(values) == 0 { + return nan } + maxValue := values[0] + maxTimestamp := timestamps[0] for i, v := range values { if v > maxValue { maxValue = v @@ -1335,16 +1328,13 @@ func rollupResets(rfa *rollupFuncArg) float64 { } func rollupFirst(rfa *rollupFuncArg) float64 { - // See https://prometheus.io/docs/prometheus/latest/querying/basics/#staleness - v := rfa.prevValue - if !math.IsNaN(v) { - return v - } - // There is no need in handling NaNs here, since they must be cleaned up // before calling rollup funcs. values := rfa.values if len(values) == 0 { + // Do not take into account rfa.prevValue, since it may lead + // to inconsistent results comparing to Prometheus on broken time series + // with irregular data points. return nan } return values[0] @@ -1357,7 +1347,10 @@ func rollupLast(rfa *rollupFuncArg) float64 { // before calling rollup funcs. values := rfa.values if len(values) == 0 { - return rfa.prevValue + // Do not take into account rfa.prevValue, since it may lead + // to inconsistent results comparing to Prometheus on broken time series + // with irregular data points. + return nan } return values[len(values)-1] } diff --git a/app/vmselect/promql/rollup_test.go b/app/vmselect/promql/rollup_test.go index f9fa085350..41e013aa82 100644 --- a/app/vmselect/promql/rollup_test.go +++ b/app/vmselect/promql/rollup_test.go @@ -459,7 +459,7 @@ func TestRollupNoWindowPartialPoints(t *testing.T) { } rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step) values := rc.Do(nil, testValues, testTimestamps) - valuesExpected := []float64{nan, 123, 123, 123, 34, 34} + valuesExpected := []float64{nan, 123, nan, 34, nan, 44} timestampsExpected := []int64{0, 5, 10, 15, 20, 25} testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected) }) @@ -473,7 +473,7 @@ func TestRollupNoWindowPartialPoints(t *testing.T) { } rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step) values := rc.Do(nil, testValues, testTimestamps) - valuesExpected := []float64{12, 44, 34, nan} + valuesExpected := []float64{44, 32, 34, nan} timestampsExpected := []int64{100, 120, 140, 160} testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected) }) @@ -487,7 +487,7 @@ func TestRollupNoWindowPartialPoints(t *testing.T) { } rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step) values := rc.Do(nil, testValues, testTimestamps) - valuesExpected := []float64{nan, nan, 123, 54, 44} + valuesExpected := []float64{nan, nan, 123, 34, 32} timestampsExpected := []int64{-50, 0, 50, 100, 150} testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected) }) @@ -549,7 +549,7 @@ func TestRollupFuncsLookbackDelta(t *testing.T) { } rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step) values := rc.Do(nil, testValues, testTimestamps) - valuesExpected := []float64{99, 12, 44, nan, 32, 34, nan} + valuesExpected := []float64{99, nan, 44, nan, 32, 34, nan} timestampsExpected := []int64{80, 90, 100, 110, 120, 130, 140} testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected) }) @@ -563,7 +563,7 @@ func TestRollupFuncsLookbackDelta(t *testing.T) { } rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step) values := rc.Do(nil, testValues, testTimestamps) - valuesExpected := []float64{99, 12, 44, 44, 32, 34, nan} + valuesExpected := []float64{99, nan, 44, nan, 32, 34, nan} timestampsExpected := []int64{80, 90, 100, 110, 120, 130, 140} testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected) }) @@ -577,7 +577,7 @@ func TestRollupFuncsLookbackDelta(t *testing.T) { } rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step) values := rc.Do(nil, testValues, testTimestamps) - valuesExpected := []float64{34, 12, 12, 44, 44, 34, nan} + valuesExpected := []float64{99, nan, 44, nan, 32, 34, nan} timestampsExpected := []int64{80, 90, 100, 110, 120, 130, 140} testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected) }) @@ -594,7 +594,7 @@ func TestRollupFuncsNoWindow(t *testing.T) { } rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step) values := rc.Do(nil, testValues, testTimestamps) - valuesExpected := []float64{nan, 123, 21, 12, 34} + valuesExpected := []float64{nan, 123, 54, 44, 34} timestampsExpected := []int64{0, 40, 80, 120, 160} testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected) }) @@ -622,7 +622,7 @@ func TestRollupFuncsNoWindow(t *testing.T) { } rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step) values := rc.Do(nil, testValues, testTimestamps) - valuesExpected := []float64{nan, 21, 12, 12, 34} + valuesExpected := []float64{nan, 21, 12, 32, 34} timestampsExpected := []int64{0, 40, 80, 120, 160} testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected) })