From bb7406e9c0411946a38ac35df1e6fd91997a65cd Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Wed, 3 Jul 2024 00:37:37 +0200 Subject: [PATCH] app/vmselect/promql: follow-up for dd0d2c77c8a3c0c22b1e5e14960f9f3048e85513 and 6149adbe10cb42a2ff651164a82e3c015a035db1 Use metricsql.IsLikelyInvalid() function for determining whether the given query is likely invalid, e.g. there is high change the query is incorrectly written, so it will return unexpected results. The query is invalid most of the time if it passes something other than series selector into rollup function. For example: - rate(sum(foo)) - rate(foo + bar) - rate(foo > bar) Improtant note: the query is considered valid if it misses the lookbehind window in square brackes inside rollup function, e.g. rate(foo), since this is very convenient MetricsQL extention to PromQL, and this query returns the expected results most of the time. Other unsafe query types can be added in the future into metricsql.IsLikelyInvalid(). TODO: probably, the -search.disableImplicitConversion command-line flag must be set by default in the future releases of VictoriaMetrics. Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4338 Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6180 Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6450 --- app/vmselect/promql/exec.go | 71 +++----------------------------- app/vmselect/promql/exec_test.go | 65 ++++++++++++++++------------- docs/MetricsQL.md | 8 ++-- 3 files changed, 47 insertions(+), 97 deletions(-) diff --git a/app/vmselect/promql/exec.go b/app/vmselect/promql/exec.go index 322d7cc760..2c75038aef 100644 --- a/app/vmselect/promql/exec.go +++ b/app/vmselect/promql/exec.go @@ -72,12 +72,13 @@ func Exec(qt *querytracer.Tracer, ec *EvalConfig, q string, isFirstPointOnly boo } if *disableImplicitConversion || *logImplicitConversion { - noConversion := noImplicitConversionRequired(e, false) - if !noConversion && *disableImplicitConversion { + isInvalid := metricsql.IsLikelyInvalid(e) + if isInvalid && *disableImplicitConversion { // we don't add query=%q to err message as it will be added by the caller - return nil, fmt.Errorf("query requires implicit conversion and is rejected according to `-search.disableImplicitConversion=true` setting. See https://docs.victoriametrics.com/metricsql/#implicit-query-conversions for details") + return nil, fmt.Errorf("query requires implicit conversion and is rejected according to -search.disableImplicitConversion command-line flag. " + + "See https://docs.victoriametrics.com/metricsql/#implicit-query-conversions for details") } - if !noConversion && *logImplicitConversion { + if isInvalid && *logImplicitConversion { logger.Warnf("query=%q requires implicit conversion, see https://docs.victoriametrics.com/metricsql/#implicit-query-conversions for details", e.AppendString(nil)) } } @@ -423,65 +424,3 @@ func (pc *parseCache) Put(q string, pcv *parseCacheValue) { pc.m[q] = pcv pc.mu.Unlock() } - -// noImplicitConversionRequired checks if expr requires implicit conversion -func noImplicitConversionRequired(e metricsql.Expr, isSubExpr bool) bool { - switch exp := e.(type) { - case *metricsql.FuncExpr: - if isSubExpr { - return false - } - fe := e.(*metricsql.FuncExpr) - isRollupFn := getRollupFunc(fe.Name) != nil - for _, arg := range exp.Args { - _, isRollupExpr := arg.(*metricsql.RollupExpr) - if (isRollupExpr && !isRollupFn) || (!isRollupExpr && isRollupFn) { - return false - } - if isRollupFn { - isSubExpr = true - } - if !noImplicitConversionRequired(arg, isSubExpr) { - return false - } - } - case *metricsql.RollupExpr: - if _, ok := exp.Expr.(*metricsql.MetricExpr); ok { - return exp.Step == nil - } - // exp.Step is optional in subqueries - if exp.Window == nil { - return false - } - return noImplicitConversionRequired(exp.Expr, false) - case *metricsql.AggrFuncExpr: - if isSubExpr { - return false - } - for _, arg := range exp.Args { - if re, ok := arg.(*metricsql.RollupExpr); ok { - if re.Window != nil { - return false - } - } - if !noImplicitConversionRequired(arg, false) { - return false - } - } - case *metricsql.BinaryOpExpr: - if isSubExpr { - return false - } - if !noImplicitConversionRequired(exp.Left, false) { - return false - } - if !noImplicitConversionRequired(exp.Right, false) { - return false - } - case *metricsql.MetricExpr: - return true - default: - return true - } - return true -} diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index eb1ed8a58d..38ad75185b 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -9453,15 +9453,16 @@ func testAddLabels(t *testing.T, mn *storage.MetricName, labels ...string) { } } -func TestNoImplicitConversionRequiredTrue(t *testing.T) { +func TestMetricsqlIsLikelyInvalid_False(t *testing.T) { f := func(q string) { t.Helper() + e, err := metricsql.Parse(q) if err != nil { t.Fatal(err) } - if !noImplicitConversionRequired(e, false) { - t.Fatalf("query should require no implicit conversion: %s", e.AppendString(nil)) + if metricsql.IsLikelyInvalid(e) { + t.Fatalf("unexpected result for metricsql.IsLikelyInvalid(%q); got true; want false", q) } } @@ -9514,21 +9515,9 @@ WITH ( cpuIdle = rate(cpuSeconds{mode='idle'}[5m]) ) max_over_time(cpuIdle[1h:])`) -} -func TestNoImplicitConversionRequiredFalse(t *testing.T) { - f := func(q string) { - t.Helper() - e, err := metricsql.Parse(q) - if err != nil { - t.Fatal(err) - } - if noImplicitConversionRequired(e, false) { - t.Fatalf("query should have require implicit conversion: %s", e.AppendString(nil)) - } - } + // These queries are mostly harmless, e.g. they return mostly correct results. f("rate(http_total)[5m:1m]") - f("up[:5m]") f("sum(up[:5m])") f("absent(foo[5m])") @@ -9536,6 +9525,37 @@ func TestNoImplicitConversionRequiredFalse(t *testing.T) { f("avg(foo[5m])") f("sort(foo[5m])") + // These are valid subqueries with MetricsQL extention, which allows omitting lookbehind window for rollup functions + f("rate(rate(http_total)[5m:1m])") + f("rate(sum(rate(http_total))[5m:1m])") + f("rate(sum(rate(http_total))[5m:1m])") + f("avg_over_time((rate(http_total)-rate(http_total))[5m:1m])") + + // These are valid MetricsQL queries, which return correct result most of the time + f("count_over_time(http_total)") + + // The following queries are from https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3974 + // + // They are mostly correct. It is better to teach metricsql parser converting them to proper ones + // instead of denying them. + f("sum(http_total) offset 1m") + f(`round(sum(sum_over_time(http_total[1m])) by (instance)) offset 1m`) + +} + +func TestMetricsqlIsLikelyInvalid_True(t *testing.T) { + f := func(q string) { + t.Helper() + + e, err := metricsql.Parse(q) + if err != nil { + t.Fatal(err) + } + if !metricsql.IsLikelyInvalid(e) { + t.Fatalf("unexpected result for metricsql.IsLikelyInvalid(%q); got false; want true", q) + } + } + f("rate(sum(http_total))") f("rate(rate(http_total))") f("sum(rate(sum(http_total)))") @@ -9543,21 +9563,10 @@ func TestNoImplicitConversionRequiredFalse(t *testing.T) { f("rate(sum(sum(http_total)))") f("avg_over_time(rate(http_total[5m]))") - f("rate(http_total)[5m:1m]") - f("rate(rate(http_total)[5m:1m])") - f("rate(sum(rate(http_total))[5m:1m])") - f("rate(sum(rate(http_total))[5m:1m])") - f("count_over_time(http_total)") - f("avg_over_time((rate(http_total)-rate(http_total))[5m:1m])") - - // https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3974 - f("sum(http_total) offset 1m") - f(`round(sum(sum_over_time(http_total[1m])) by (instance)) offset 1m`) - f("rate(sum(http_total)) - rate(sum(http_total))") f("avg_over_time(rate(http_total)-rate(http_total))") - // https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3996 + // These queries are from https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3996 f("sum_over_time(up{cluster='a'} or up{cluster='b'})") f("sum_over_time(up{cluster='a'}[1m] or up{cluster='b'}[1m])") f("sum(sum_over_time(up{cluster='a'}[1m] or up{cluster='b'}[1m])) by (instance)") diff --git a/docs/MetricsQL.md b/docs/MetricsQL.md index d49ccfb6e9..6104b45a42 100644 --- a/docs/MetricsQL.md +++ b/docs/MetricsQL.md @@ -2230,7 +2230,8 @@ Any [rollup function](#rollup-functions) for something other than [series select Nested rollup functions can be implicit thanks to the [implicit query conversions](#implicit-query-conversions). For example, `delta(sum(m))` is implicitly converted to `delta(sum(default_rollup(m))[1i:1i])`, so it becomes a subquery, since it contains [default_rollup](#default_rollup) nested into [delta](#delta). -This behavior can be disabled or logged via cmd-line flags `-search.disableImplicitConversion` and `-search.logImplicitConversion` since v1.101.0. +This behavior can be disabled or logged via `-search.disableImplicitConversion` and `-search.logImplicitConversion` command-line flags +starting from [`v1.101.0` release](https://docs.victoriametrics.com/changelog/). VictoriaMetrics performs subqueries in the following way: @@ -2263,5 +2264,6 @@ VictoriaMetrics performs the following implicit conversions for incoming queries For example, `avg_over_time(rate(http_requests_total[5m])[1h])` is automatically converted to `avg_over_time(rate(http_requests_total[5m])[1h:1i])`. * If something other than [series selector](https://docs.victoriametrics.com/keyconcepts/#filtering) is passed to [rollup function](#rollup-functions), then a [subquery](#subqueries) with `1i` lookbehind window and `1i` step is automatically formed. - For example, `rate(sum(up))` is automatically converted to `rate((sum(default_rollup(up)))[1i:1i])`. - This behavior can be disabled or logged via cmd-line flags `-search.disableImplicitConversion` and `-search.logImplicitConversion` since v1.101.0. + For example, `rate(sum(up))` is automatically converted to `rate((sum(default_rollup(up)))[1i:1i])`. + This behavior can be disabled or logged via `-search.disableImplicitConversion` and `-search.logImplicitConversion` command-line flags + starting from [`v1.101.0` release](https://docs.victoriametrics.com/changelog/).