From be31bdc88cbc7a4605cdfa9e493d8be7f2d4a34e Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Sun, 16 Jul 2023 23:44:05 -0700 Subject: [PATCH] app/vmselect/promql: recommend to use `(a op b) keep_metric_names` instead of `a op b keep_metric_names` The `a op b keep_metric_names` is ambigouos to `a op (b keep_metric_names)` when `b` is a transform or rollup function. For example, `a + rate(b) keep_metric_names`. So it is better to use more clear syntax: `(a op b) keep_metric_names` Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3710 --- app/vmselect/promql/exec_test.go | 21 +++---- docs/CHANGELOG.md | 2 +- docs/MetricsQL.md | 2 +- go.mod | 2 +- go.sum | 4 +- .../VictoriaMetrics/metricsql/parser.go | 61 ++++++++++++++----- vendor/modules.txt | 2 +- 7 files changed, 61 insertions(+), 33 deletions(-) diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index 90b85345f..a22135358 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -3005,7 +3005,7 @@ func TestExecSuccess(t *testing.T) { }) t.Run(`vector / scalar keep_metric_names`, func(t *testing.T) { t.Parallel() - q := `sort_desc((label_set(time(), "foo", "bar", "__name__", "q1") or label_set(10, "foo", "qwert", "__name__", "q2")) / 2 keep_metric_names)` + q := `sort_desc(((label_set(time(), "foo", "bar", "__name__", "q1") or label_set(10, "foo", "qwert", "__name__", "q2")) / 2) keep_metric_names)` r1 := netstorage.Result{ MetricName: storage.MetricName{ MetricGroup: []byte("q1"), @@ -3068,7 +3068,7 @@ func TestExecSuccess(t *testing.T) { }) t.Run(`scalar * vector keep_metric_names`, func(t *testing.T) { t.Parallel() - q := `sort_desc(2 * (label_set(time(), "foo", "bar", "__name__", "q1") or label_set(10, "foo", "qwert", "__name__", "q2")) keep_metric_names)` + q := `sort_desc(2 * (label_set(time(), "foo", "bar", "__name__", "q1"), label_set(10, "foo", "qwert", "__name__", "q2")) keep_metric_names)` r1 := netstorage.Result{ MetricName: storage.MetricName{ MetricGroup: []byte("q1"), @@ -3120,7 +3120,7 @@ func TestExecSuccess(t *testing.T) { }) t.Run(`scalar * on() group_right vector keep_metric_names`, func(t *testing.T) { t.Parallel() - q := `sort_desc(2 * on() group_right() (label_set(time(), "foo", "bar", "__name__", "q1") or label_set(10, "foo", "qwert", "__name__", "q2")) keep_metric_names)` + q := `sort_desc(2 * on() group_right() (label_set(time(), "foo", "bar", "__name__", "q1"), label_set(10, "foo", "qwert", "__name__", "q2")) keep_metric_names)` r1 := netstorage.Result{ MetricName: storage.MetricName{ MetricGroup: []byte("q1"), @@ -3233,11 +3233,10 @@ func TestExecSuccess(t *testing.T) { }) t.Run(`vector * on(foo) scalar keep_metric_names`, func(t *testing.T) { t.Parallel() - q := `sort_desc( - ( + q := `(( label_set(time(), "foo", "bar", "xx", "yy", "__name__", "q1"), label_set(10, "foo", "qwert", "__name__", "q2") - ) * on(foo) (label_set(2, "foo","bar","aa","bb", "__name__", "q2")) keep_metric_names)` + ) * on(foo) label_set(2, "foo","bar","aa","bb", "__name__", "q2")) keep_metric_names` r := netstorage.Result{ MetricName: metricNameExpected, Values: []float64{2000, 2400, 2800, 3200, 3600, 4000}, @@ -3483,11 +3482,11 @@ func TestExecSuccess(t *testing.T) { }) t.Run(`vector + vector partial matching keep_metric_names`, func(t *testing.T) { t.Parallel() - q := `sort_desc( - (label_set(time(), "t1", "v1", "__name__", "q1") or label_set(10, "t2", "v2", "__name__", "q2")) - + - (label_set(100, "t1", "v1", "__name__", "q3") or label_set(time(), "t2", "v3")) keep_metric_names - )` + q := `( + (label_set(time(), "t1", "v1", "__name__", "q1") or label_set(10, "t2", "v2", "__name__", "q2")) + + + (label_set(100, "t1", "v1", "__name__", "q3") or label_set(time(), "t2", "v3")) + ) keep_metric_names` r := netstorage.Result{ MetricName: metricNameExpected, Values: []float64{1100, 1300, 1500, 1700, 1900, 2100}, diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 7f273581a..310b871e0 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -29,7 +29,7 @@ The following tip changes can be tested by building VictoriaMetrics components f * FEATURE: reduce memory usage by up to 5x for setups with [high churn rate](https://docs.victoriametrics.com/FAQ.html#what-is-high-churn-rate) and long [retention](https://docs.victoriametrics.com/#retention). See [description for this change](https://github.com/VictoriaMetrics/VictoriaMetrics/commit/7094fa38bc207c7bd7330ea8a834310a310ce5e3) for details. * FEATURE: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): allow selecting time series matching at least one of multiple `or` filters. For example, `{env="prod",job="a" or env="dev",job="b"}` selects series with either `{env="prod",job="a"}` or `{env="dev",job="b"}` labels. This functionality allows passing the selected series to [rollup functions](https://docs.victoriametrics.com/MetricsQL.html#rollup-functions) without the need to use [subqueries](https://docs.victoriametrics.com/MetricsQL.html#subqueries). See [these docs](https://docs.victoriametrics.com/keyConcepts.html#filtering-by-multiple-or-filters). -* FEATURE: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): add ability to preserve metric names for binary operation results via `keep_metric_names` modifier. For example, `{__name__=~"foo|bar"} / 10 keep_metric_names` leaves `foo` and `bar` metric names in division results. See [these docs](https://docs.victoriametrics.com/MetricsQL.html#keep_metric_names). This helps to address issues like [this one](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3710). +* FEATURE: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): add ability to preserve metric names for binary operation results via `keep_metric_names` modifier. For example, `({__name__=~"foo|bar"} / 10) keep_metric_names` leaves `foo` and `bar` metric names in division results. See [these docs](https://docs.victoriametrics.com/MetricsQL.html#keep_metric_names). This helps to address issues like [this one](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3710). * FEATURE: [vmctl](https://docs.victoriametrics.com/vmctl.html): add verbose output for docker installations or when TTY isn't available. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4081). * FEATURE: [vmctl](https://docs.victoriametrics.com/vmctl.html): interrupt backoff retries when import process is cancelled. The change makes vmctl more responsive in case of errors during the import. See [this pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4442). * FEATURE: [vmctl](https://docs.victoriametrics.com/vmctl.html): update backoff policy on retries to reduce probability of overloading for `source` or `destination` databases. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4402). diff --git a/docs/MetricsQL.md b/docs/MetricsQL.md index 26476bff9..a57dace4c 100644 --- a/docs/MetricsQL.md +++ b/docs/MetricsQL.md @@ -120,7 +120,7 @@ This error can be fixed by applying `keep_metric_names` modifier to the function For example: - `rate({__name__=~"foo|bar"}) keep_metric_names` leaves `foo` and `bar` metric names in the returned time series. -- `{__name__=~"foo|bar"} / 10 keep_metric_names` leaves `foo` and `bar` metric names in the returned time series. +- `({__name__=~"foo|bar"} / 10) keep_metric_names` leaves `foo` and `bar` metric names in the returned time series. ## MetricsQL functions diff --git a/go.mod b/go.mod index d6a707e0d..25b68546e 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( // like https://github.com/valyala/fasthttp/commit/996610f021ff45fdc98c2ce7884d5fa4e7f9199b github.com/VictoriaMetrics/fasthttp v1.2.0 github.com/VictoriaMetrics/metrics v1.24.0 - github.com/VictoriaMetrics/metricsql v0.58.1 + github.com/VictoriaMetrics/metricsql v0.58.2 github.com/aws/aws-sdk-go-v2 v1.18.1 github.com/aws/aws-sdk-go-v2/config v1.18.27 github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.11.71 diff --git a/go.sum b/go.sum index 72a5bd0f0..c023317f9 100644 --- a/go.sum +++ b/go.sum @@ -69,8 +69,8 @@ github.com/VictoriaMetrics/fasthttp v1.2.0 h1:nd9Wng4DlNtaI27WlYh5mGXCJOmee/2c2b github.com/VictoriaMetrics/fasthttp v1.2.0/go.mod h1:zv5YSmasAoSyv8sBVexfArzFDIGGTN4TfCKAtAw7IfE= github.com/VictoriaMetrics/metrics v1.24.0 h1:ILavebReOjYctAGY5QU2F9X0MYvkcrG3aEn2RKa1Zkw= github.com/VictoriaMetrics/metrics v1.24.0/go.mod h1:eFT25kvsTidQFHb6U0oa0rTrDRdz4xTYjpL8+UPohys= -github.com/VictoriaMetrics/metricsql v0.58.1 h1:zg7xN1HptC5oloWvdt4PtgDaBEyMdsgERikUg0hFZB4= -github.com/VictoriaMetrics/metricsql v0.58.1/go.mod h1:k4UaP/+CjuZslIjd+kCigNG9TQmUqh5v0TP/nMEy90I= +github.com/VictoriaMetrics/metricsql v0.58.2 h1:xECDKK13ZR7LCpepVM7nuzJTnEkQI3r0qNxQIY6uv60= +github.com/VictoriaMetrics/metricsql v0.58.2/go.mod h1:k4UaP/+CjuZslIjd+kCigNG9TQmUqh5v0TP/nMEy90I= github.com/VividCortex/ewma v1.2.0 h1:f58SaIzcDXrSy3kWaHNvuJgJ3Nmz59Zji6XoJR/q1ow= github.com/VividCortex/ewma v1.2.0/go.mod h1:nz4BbCtbLyFDeC9SUHbtcT5644juEuWfUAUnGx7j5l4= github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= diff --git a/vendor/github.com/VictoriaMetrics/metricsql/parser.go b/vendor/github.com/VictoriaMetrics/metricsql/parser.go index 2fb8d73af..116bb1c11 100644 --- a/vendor/github.com/VictoriaMetrics/metricsql/parser.go +++ b/vendor/github.com/VictoriaMetrics/metricsql/parser.go @@ -573,6 +573,14 @@ func (p *parser) parseParensExpr() (*parensExpr, error) { if err := p.lex.Next(); err != nil { return nil, err } + if len(exprs) == 1 { + if be, ok := exprs[0].(*BinaryOpExpr); ok && isKeepMetricNames(p.lex.Token) { + if err := p.lex.Next(); err != nil { + return nil, err + } + be.KeepMetricNames = true + } + } pe := parensExpr(exprs) return &pe, nil } @@ -1632,7 +1640,18 @@ type BinaryOpExpr struct { // AppendString appends string representation of be to dst and returns the result. func (be *BinaryOpExpr) AppendString(dst []byte) []byte { - if _, ok := be.Left.(*BinaryOpExpr); ok { + if be.KeepMetricNames { + dst = append(dst, '(') + dst = be.appendStringNoKeepMetricNames(dst) + dst = append(dst, ") keep_metric_names"...) + } else { + dst = be.appendStringNoKeepMetricNames(dst) + } + return dst +} + +func (be *BinaryOpExpr) appendStringNoKeepMetricNames(dst []byte) []byte { + if be.needLeftParens() { dst = appendArgInParens(dst, be.Left) } else { dst = be.Left.AppendString(dst) @@ -1656,30 +1675,40 @@ func (be *BinaryOpExpr) AppendString(dst []byte) []byte { } else { dst = be.Right.AppendString(dst) } - if be.KeepMetricNames { - dst = append(dst, " keep_metric_names"...) - } return dst } +func (be *BinaryOpExpr) needLeftParens() bool { + return needBinaryOpArgParens(be.Left) +} + func (be *BinaryOpExpr) needRightParens() bool { - r := be.Right - if _, ok := r.(*BinaryOpExpr); ok { + if needBinaryOpArgParens(be.Right) { return true } - if me, ok := r.(*MetricExpr); ok { - metricName := me.getMetricName() + switch t := be.Right.(type) { + case *MetricExpr: + metricName := t.getMetricName() return isReservedBinaryOpIdent(metricName) - } - if fe, ok := r.(*FuncExpr); ok && isReservedBinaryOpIdent(fe.Name) { - return true - } - if !be.KeepMetricNames { + case *FuncExpr: + if isReservedBinaryOpIdent(t.Name) { + return true + } + return be.KeepMetricNames + default: return false } - switch r.(type) { - case *FuncExpr: +} + +func needBinaryOpArgParens(arg Expr) bool { + switch t := arg.(type) { + case *BinaryOpExpr: return true + case *RollupExpr: + if be, ok := t.Expr.(*BinaryOpExpr); ok && be.KeepMetricNames { + return true + } + return t.Offset != nil || t.At != nil default: return false } @@ -1708,7 +1737,7 @@ type ModifierExpr struct { // AppendString appends string representation of me to dst and returns the result. func (me *ModifierExpr) AppendString(dst []byte) []byte { dst = append(dst, me.Op...) - dst = append(dst, "("...) + dst = append(dst, '(') for i, arg := range me.Args { dst = appendEscapedIdent(dst, arg) if i+1 < len(me.Args) { diff --git a/vendor/modules.txt b/vendor/modules.txt index 7ad3700a5..d525096ab 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -99,7 +99,7 @@ github.com/VictoriaMetrics/fasthttp/stackless # github.com/VictoriaMetrics/metrics v1.24.0 ## explicit; go 1.20 github.com/VictoriaMetrics/metrics -# github.com/VictoriaMetrics/metricsql v0.58.1 +# github.com/VictoriaMetrics/metricsql v0.58.2 ## explicit; go 1.13 github.com/VictoriaMetrics/metricsql github.com/VictoriaMetrics/metricsql/binaryop