Kiu/VictoriaMetrics
Kiu/VictoriaMetrics
1
0
Fork 0
mirror of https://github.com/VictoriaMetrics/VictoriaMetrics.git synced 2024-12-19 15:06:25 +01:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

app/vmselect/promql: follow-up for dd0d2c77c8 and 6149adbe10

Browse Source 
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
This commit is contained in:
Aliaksandr Valialkin 2024-07-03 00:37:37 +02:00
parent 82748b2b9d
commit bb7406e9c0
No known key found for this signature in database
GPG Key ID: 52C003EE2BCDB9EB
3 changed files with 47 additions and 97 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
app/vmselect/promql/exec.go
View File

@ -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
}

65
app/vmselect/promql/exec_test.go
View File

@ -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)")

8
docs/MetricsQL.md
View File

@ -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/).