Repository: VictoriaMetrics/VictoriaMetrics
Pull request #6180 (2024-04-24 07:40:59)

app/vmselect: implement cmd-line flags `-search.disableImplicitConver…
…sions` and `-search.logImplicitConversions`

### Describe Your Changes
address https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4338
support disable or log [implicit conversions](https://docs.victoriametrics.com/metricsql/#implicit-query-conversions) for subquery with cmd-line flags `-search.disableImplicitConversion` and `-search.logImplicitConversion`

### Checklist

The following checks are **mandatory**:

- [x] My change adheres [VictoriaMetrics contributing guidelines](https://docs.victoriametrics.com/contributing/).


Haleygo: > @Haleygo please see my updates [e3c9a44](https://github.com/VictoriaMetrics/VictoriaMetrics/commit/e3c9a44a8ac3bafa2d64bc8ed5e9b13422f1a6a6)
> 
> Let me know if you have feedback. Thanks!

LGTM!
nit: small comments

Patch:
diff --git a/app/vmselect/promql/exec.go b/app/vmselect/promql/exec.go
index 81ae717b7..5414387ab 100644
--- a/app/vmselect/promql/exec.go
+++ b/app/vmselect/promql/exec.go
@@ -13,6 +13,7 @@ import (
 	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmselect/netstorage"
 	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmselect/querystats"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/decimal"
+	"github.com/VictoriaMetrics/VictoriaMetrics/lib/logger"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/querytracer"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/storage"
 	"github.com/VictoriaMetrics/metrics"
@@ -26,6 +27,12 @@ var (
 		`For example, foo{bar=~"a.b.c"} will be automatically converted to foo{bar=~"a\\.b\\.c"}, i.e. all the dots in regexp filters will be automatically escaped `+
 		`in order to match only dot char instead of matching any char. Dots in ".+", ".*" and ".{n}" regexps aren't escaped. `+
 		`This option is DEPRECATED in favor of {__graphite__="a.*.c"} syntax for selecting metrics matching the given Graphite metrics filter`)
+	disableImplicitConversion = flag.Bool("search.disableImplicitConversion", false, "Whether to return an error for queries that rely on implicit subquery conversions, "+
+		"see https://docs.victoriametrics.com/metricsql/#subqueries for details. "+
+		"See also -search.logImplicitConversion.")
+	logImplicitConversion = flag.Bool("search.logImplicitConversion", false, "Whether to log queries with implicit subquery conversions, "+
+		"see https://docs.victoriametrics.com/metricsql/#subqueries for details. "+
+		"Such conversion can be disabled using -search.disableImplicitConversion.")
 )
 
 // UserReadableError is a type of error which supposed to be returned to the user without additional context.
@@ -63,6 +70,16 @@ func Exec(qt *querytracer.Tracer, ec *EvalConfig, q string, isFirstPointOnly boo
 		return nil, err
 	}
 
+	if *disableImplicitConversion || *logImplicitConversion {
+		complete := isSubQueryComplete(e, false)
+		if !complete && *disableImplicitConversion {
+			return nil, fmt.Errorf("query contains subquery that requires implicit conversion and is rejected according to `-search.disableImplicitConversion=true` setting. See https://docs.victoriametrics.com/metricsql/#subqueries for details")
+		}
+		if !complete && *logImplicitConversion {
+			logger.Warnf("query=%q contains subquery that requires implicit conversion, see https://docs.victoriametrics.com/metricsql/#subqueries for details", e.AppendString(nil))
+		}
+	}
+
 	qid := activeQueriesV.Add(ec, q)
 	rv, err := evalExpr(qt, ec, e)
 	activeQueriesV.Remove(qid)
@@ -404,3 +421,55 @@ func (pc *parseCache) Put(q string, pcv *parseCacheValue) {
 	pc.m[q] = pcv
 	pc.mu.Unlock()
 }
+
+// isSubQueryComplete checks if expr contains incomplete subquery
+func isSubQueryComplete(e metricsql.Expr, isSubExpr bool) bool {
+	switch exp := e.(type) {
+	case *metricsql.FuncExpr:
+		if isSubExpr {
+			return false
+		}
+		fe := e.(*metricsql.FuncExpr)
+		for _, arg := range exp.Args {
+			if getRollupFunc(fe.Name) != nil {
+				isSubExpr = true
+			}
+			if !isSubQueryComplete(arg, isSubExpr) {
+				return false
+			}
+		}
+	case *metricsql.RollupExpr:
+		if _, ok := exp.Expr.(*metricsql.MetricExpr); ok {
+			return true
+		}
+		// exp.Step is optional in subqueries
+		if exp.Window == nil {
+			return false
+		}
+		return isSubQueryComplete(exp.Expr, false)
+	case *metricsql.AggrFuncExpr:
+		if isSubExpr {
+			return false
+		}
+		for _, arg := range exp.Args {
+			if !isSubQueryComplete(arg, false) {
+				return false
+			}
+		}
+	case *metricsql.BinaryOpExpr:
+		if isSubExpr {
+			return false
+		}
+		if !isSubQueryComplete(exp.Left, false) {
+			return false
+		}
+		if !isSubQueryComplete(exp.Right, false) {
+			return false
+		}
+	case *metricsql.MetricExpr:
+		return true
+	default:
+		return true
+	}
+	return true
+}
diff --git a/app/vmui/packages/vmui/src/assets/MetricsQL.md b/app/vmui/packages/vmui/src/assets/MetricsQL.md
index 7f3ccc498..5c517e4ed 100644
--- a/app/vmui/packages/vmui/src/assets/MetricsQL.md
+++ b/app/vmui/packages/vmui/src/assets/MetricsQL.md
@@ -2211,7 +2211,8 @@ MetricsQL supports and extends PromQL subqueries. See [this article](https://val
 Any [rollup function](#rollup-functions) for something other than [series selector](https://docs.victoriametrics.com/keyconcepts/#filtering) form a subquery.
 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).
+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.
 
 VictoriaMetrics performs subqueries in the following way:
 
@@ -2241,7 +2242,8 @@ VictoriaMetrics performs the following implicit conversions for incoming queries
   * `abs(temperature)` is transformed to `abs(default_rollup(temperature))`, because [abs](#abs) isn't a [rollup function](#rollup-functions) -
     it is [transform function](#transform-functions)
 * If `step` in square brackets is missing inside [subquery](#subqueries), then `1i` step is automatically added there.
-  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])`.
+  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])`.
+  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.
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index 03d041086..51d5a671c 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -33,6 +33,7 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/).
 * FEATURE: [MetricsQL](https://docs.victoriametrics.com/metricsql/): support filtering by multiple numeric constants via `q == (C1, ..., CN)` and `q != (C1, ..., CN)` syntax. For example, `status_code == (200, 201, 300)` returns `status_code` metrics with any of `200`, `201` or `300` values, while `status_code != (400, 404, 500)` returns `status_code` metrics with all the values except of `400`, `404` and `500`.
 * FEATURE: [VictoriaMetrics cluster](https://docs.victoriametrics.com/cluster-victoriametrics/): add support for fault domain awareness to `vmselect`. It can be configured to return full responses if up to `-globalReplicationFactor - 1` fault domains (aka `vmstorage` groups) are unavailable. See [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6054) and [these docs](https://docs.victoriametrics.com/cluster-victoriametrics/#vmstorage-groups-at-vmselect).
 * FEATURE: all VictoriaMetrics [enterprise](https://docs.victoriametrics.com/enterprise/) components: add support for automatic issuing of TLS certificates for HTTPS server at `-httpListenAddr` via [Let's Encrypt service](https://letsencrypt.org/). See [these docs](https://docs.victoriametrics.com/#automatic-issuing-of-tls-certificates) and [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5949).
+* FEATURE: [vmsingle](https://docs.victoriametrics.com/single-server-victoriametrics/): support disable or log [implicit conversions](https://docs.victoriametrics.com/metricsql/#implicit-query-conversions) for subquery with cmd-line flags `-search.disableImplicitConversion` and `-search.logImplicitConversion`. See [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4338).
 * FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent/): support data replication additionally to sharding among remote storage systems if `-remoteWrite.shardByURLReplicas=N` command-line flag is set additionally to `-remoteWrite.shardByURL` command-line flag, where `N` is desired replication factor. This allows setting up data replication among failure domains when the replication factor is smaller than the number of failure domains. See [these docs](https://docs.victoriametrics.com/vmagent/#sharding-among-remote-storages) and [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6054).
 * FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent/): reduce CPU usage when [sharding among remote storage systems](https://docs.victoriametrics.com/vmagent/#sharding-among-remote-storages) is enabled.
 * FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent/): support [DNS SRV](https://en.wikipedia.org/wiki/SRV_record) addresses in `-remoteWrite.url` command-line option and in scrape target urls. For example, `-remoteWrite.url=http://srv+victoria-metrics/api/v1/write` automatically resolves the `victoria-metrics` DNS SRV to a list of hostnames with TCP ports and then sends the collected metrics to these TCP addresses. See [these docs](https://docs.victoriametrics.com/vmagent/#srv-urls) and [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6053).
diff --git a/docs/MetricsQL.md b/docs/MetricsQL.md
index cdfa3039f..fec10e5dc 100644
--- a/docs/MetricsQL.md
+++ b/docs/MetricsQL.md
@@ -2229,6 +2229,7 @@ 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.
 
 VictoriaMetrics performs subqueries in the following way:
 
@@ -2261,4 +2262,5 @@ 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])`.
+  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.


Test patch:
diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go
index 0e687f8d4..9923160a2 100644
--- a/app/vmselect/promql/exec_test.go
+++ b/app/vmselect/promql/exec_test.go
@@ -9432,3 +9432,104 @@ func testAddLabels(t *testing.T, mn *storage.MetricName, labels ...string) {
 		})
 	}
 }
+
+func TestIsSubQueryCompleteTrue(t *testing.T) {
+	f := func(q string) {
+		t.Helper()
+		e, err := metricsql.Parse(q)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !isSubQueryComplete(e, false) {
+			t.Fatalf("query should be complete: %s", e.AppendString(nil))
+		}
+	}
+
+	f("rate(http_total)")
+	f("sum(http_total)")
+	f("absent(http_total)")
+	f("rate(http_total[1m])")
+	f("avg_over_time(up[1m])")
+	f("sum(http_total[1m])")
+	f("sum(rate(http_total))")
+	f("sum(sum(http_total))")
+	f(`sum(sum_over_time(http_total[1m] )) by (instance)`)
+	f("sum(up{cluster='a'}[1m] or up{cluster='b'}[1m])")
+	f("(avg_over_time(alarm_test1[1m]) - avg_over_time(alarm_test1[1m] offset 5m)) > 0.1")
+	f("http_total[1m] offset 1m")
+
+	// subquery
+	f("rate(http_total)[5m:1m]")
+	f("rate(sum(http_total)[5m:1m])")
+	f("rate(rate(http_total)[5m:1m])")
+	f("sum(rate(http_total[1m]))")
+	f("sum(rate(sum(http_total)[5m:1m]))")
+	f("rate(sum(rate(http_total))[5m:1m])")
+	f("rate(sum(sum(http_total))[5m:1m])")
+	f("rate(sum(rate(http_total))[5m:1m])")
+	f("rate(sum(sum(http_total))[5m:1m])")
+	f("avg_over_time(rate(http_total[5m])[5m:1m])")
+	f("delta(avg_over_time(up[1m])[5m:1m]) > 0.1")
+	f("avg_over_time(avg by (site) (metric)[2m:1m])")
+
+	f("sum(http_total)[5m:1m] offset 1m")
+	f("round(sum(sum_over_time(http_total[1m])) by (instance)) [5m:1m] offset 1m")
+
+	f("rate(sum(http_total)[5m:1m]) - rate(sum(http_total)[5m:1m])")
+	f("avg_over_time((rate(http_total)-rate(http_total))[5m:1m])")
+
+	f("sum_over_time((up{cluster='a'} or up{cluster='b'})[5m:1m])")
+	f("sum_over_time((up{cluster='a'} or up{cluster='b'})[5m:1m])")
+	f("sum(sum_over_time((up{cluster='a'} or up{cluster='b'})[5m:1m])) by (instance)")
+
+	// step (or resolution) is optional in subqueries
+	f("max_over_time(rate(my_counter_total[5m])[1h:])")
+	f("max_over_time(rate(my_counter_total[5m])[1h:1m])[5m:1m]")
+	f("max_over_time(rate(my_counter_total[5m])[1h:])[5m:]")
+
+	f(`
+WITH (
+    cpuSeconds = node_cpu_seconds_total{instance=~"$node:$port",job=~"$job"},
+    cpuIdle = rate(cpuSeconds{mode='idle'}[5m])
+)
+max_over_time(cpuIdle[1h:])`)
+}
+
+func TestIsSubQueryCompleteFalse(t *testing.T) {
+	f := func(q string) {
+		t.Helper()
+		e, err := metricsql.Parse(q)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if isSubQueryComplete(e, false) {
+			t.Fatalf("expect to detect incomplete subquery: %s", e.AppendString(nil))
+		}
+	}
+
+	f("rate(sum(http_total))")
+	f("rate(rate(http_total))")
+	f("sum(rate(sum(http_total)))")
+	f("rate(sum(rate(http_total)))")
+	f("rate(sum(sum(http_total)))")
+	f("avg_over_time(rate(http_total[5m]))")
+
+	// 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
+	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)")
+
+	f(`
+WITH (
+    cpuSeconds = node_cpu_seconds_total{instance=~"$node:$port",job=~"$job"},
+    cpuIdle = rate(cpuSeconds{mode='idle'}[5m])
+)
+max_over_time(cpuIdle)`)
+}