From b87f3bdb50e468dd2698ba456f17f230f9b7d4ce Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Wed, 11 Jan 2023 16:12:47 +0530 Subject: [PATCH] fix: query builder formula fails to eval (#1999) * fix: query builder formula fails to eval * fix: result label set without reference * chore: update tests Co-authored-by: Prashant Shahi --- .github/workflows/build.yaml | 4 + Makefile | 3 + .../app/metrics/query_builder.go | 46 ++++- .../app/metrics/query_builder_test.go | 166 ++++++++++++++---- 4 files changed, 181 insertions(+), 38 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index a7bcbd1ad2..ec417a851b 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -32,6 +32,10 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v2 + - name: Run tests + shell: bash + run: | + make test - name: Build query-service image shell: bash run: | diff --git a/Makefile b/Makefile index a651c84103..0847cc7dfe 100644 --- a/Makefile +++ b/Makefile @@ -135,3 +135,6 @@ clear-standalone-data: clear-swarm-data: @docker run --rm -v "$(PWD)/$(SWARM_DIRECTORY)/data:/pwd" busybox \ sh -c "cd /pwd && rm -rf alertmanager/* clickhouse*/* signoz/* zookeeper-*/*" + +test: + go test ./pkg/query-service/app/metrics/... diff --git a/pkg/query-service/app/metrics/query_builder.go b/pkg/query-service/app/metrics/query_builder.go index 65125dd580..784b727514 100644 --- a/pkg/query-service/app/metrics/query_builder.go +++ b/pkg/query-service/app/metrics/query_builder.go @@ -340,6 +340,7 @@ func varToQuery(qp *model.QueryRangeParamsV2, tableName string) (map[string]stri evalFuncs := GoValuateFuncs() varToQuery := make(map[string]string) for _, builderQuery := range qp.CompositeMetricQuery.BuilderQueries { + // err should be nil here since the expression is already validated expression, _ := govaluate.NewEvaluableExpressionWithFunctions(builderQuery.Expression, evalFuncs) // Use the parsed expression and build the query for each variable @@ -347,7 +348,11 @@ func varToQuery(qp *model.QueryRangeParamsV2, tableName string) (map[string]stri var errs []error for _, _var := range expression.Vars() { if _, ok := varToQuery[_var]; !ok { - mq := qp.CompositeMetricQuery.BuilderQueries[_var] + mq, varExists := qp.CompositeMetricQuery.BuilderQueries[_var] + if !varExists { + errs = append(errs, fmt.Errorf("variable %s not found in builder queries", _var)) + continue + } query, err := BuildMetricQuery(qp, mq, tableName) if err != nil { errs = append(errs, err) @@ -369,10 +374,22 @@ func varToQuery(qp *model.QueryRangeParamsV2, tableName string) (map[string]stri return varToQuery, nil } +func unique(slice []string) []string { + keys := make(map[string]struct{}) + list := []string{} + for _, entry := range slice { + if _, value := keys[entry]; !value { + keys[entry] = struct{}{} + list = append(list, entry) + } + } + return list +} + // expressionToQuery constructs the query for the expression func expressionToQuery(qp *model.QueryRangeParamsV2, varToQuery map[string]string, expression *govaluate.EvaluableExpression) (string, error) { var formulaQuery string - vars := expression.Vars() + vars := unique(expression.Vars()) for idx, var_ := range vars[1:] { x, y := vars[idx], var_ if !reflect.DeepEqual(qp.CompositeMetricQuery.BuilderQueries[x].GroupingTags, qp.CompositeMetricQuery.BuilderQueries[y].GroupingTags) { @@ -389,21 +406,34 @@ func expressionToQuery(qp *model.QueryRangeParamsV2, varToQuery map[string]strin } modified = append(modified, token) } + // err should be nil here since the expression is already validated formula, _ := govaluate.NewEvaluableExpressionFromTokens(modified) var formulaSubQuery string var joinUsing string + var prevVar string for idx, var_ := range vars { query := varToQuery[var_] groupTags := qp.CompositeMetricQuery.BuilderQueries[var_].GroupingTags groupTags = append(groupTags, "ts") - joinUsing = strings.Join(groupTags, ",") - formulaSubQuery += fmt.Sprintf("(%s) as %s ", query, var_) - if idx < len(vars)-1 { - formulaSubQuery += "GLOBAL INNER JOIN" - } else if len(vars) > 1 { - formulaSubQuery += fmt.Sprintf("USING (%s)", joinUsing) + if joinUsing == "" { + for _, tag := range groupTags { + joinUsing += fmt.Sprintf("%s.%s as %s, ", var_, tag, tag) + } + joinUsing = strings.TrimSuffix(joinUsing, ", ") } + formulaSubQuery += fmt.Sprintf("(%s) as %s ", query, var_) + if idx > 0 { + formulaSubQuery += " ON " + for _, tag := range groupTags { + formulaSubQuery += fmt.Sprintf("%s.%s = %s.%s AND ", prevVar, tag, var_, tag) + } + formulaSubQuery = strings.TrimSuffix(formulaSubQuery, " AND ") + } + if idx < len(vars)-1 { + formulaSubQuery += " GLOBAL INNER JOIN" + } + prevVar = var_ } formulaQuery = fmt.Sprintf("SELECT %s, %s as value FROM ", joinUsing, formula.ExpressionString()) + formulaSubQuery return formulaQuery, nil diff --git a/pkg/query-service/app/metrics/query_builder_test.go b/pkg/query-service/app/metrics/query_builder_test.go index 3b15e1f464..92bc60c5b0 100644 --- a/pkg/query-service/app/metrics/query_builder_test.go +++ b/pkg/query-service/app/metrics/query_builder_test.go @@ -1,6 +1,7 @@ package metrics import ( + "strings" "testing" . "github.com/smartystreets/goconvey/convey" @@ -15,19 +16,19 @@ func TestBuildQuery(t *testing.T) { Step: 60, CompositeMetricQuery: &model.CompositeMetricQuery{ BuilderQueries: map[string]*model.MetricQuery{ - "a": { - QueryName: "a", + "A": { + QueryName: "A", MetricName: "name", AggregateOperator: model.RATE_MAX, - Expression: "a", + Expression: "A", }, }, }, } queries := PrepareBuilderMetricQueries(q, "table").Queries So(len(queries), ShouldEqual, 1) - So(queries["a"], ShouldContainSubstring, "WHERE metric_name = 'name'") - So(queries["a"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)") + So(queries["A"], ShouldContainSubstring, "WHERE metric_name = 'name'") + So(queries["A"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)") }) } @@ -39,15 +40,15 @@ func TestBuildQueryWithFilters(t *testing.T) { Step: 60, CompositeMetricQuery: &model.CompositeMetricQuery{ BuilderQueries: map[string]*model.MetricQuery{ - "a": { - QueryName: "a", + "A": { + QueryName: "A", MetricName: "name", TagFilters: &model.FilterSet{Operator: "AND", Items: []model.FilterItem{ {Key: "a", Value: "b", Operator: "neq"}, {Key: "code", Value: "ERROR_*", Operator: "nmatch"}, }}, AggregateOperator: model.RATE_MAX, - Expression: "a", + Expression: "A", }, }, }, @@ -55,9 +56,9 @@ func TestBuildQueryWithFilters(t *testing.T) { queries := PrepareBuilderMetricQueries(q, "table").Queries So(len(queries), ShouldEqual, 1) - So(queries["a"], ShouldContainSubstring, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'a') != 'b'") - So(queries["a"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)") - So(queries["a"], ShouldContainSubstring, "not match(JSONExtractString(labels, 'code'), 'ERROR_*')") + So(queries["A"], ShouldContainSubstring, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'a') != 'b'") + So(queries["A"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)") + So(queries["A"], ShouldContainSubstring, "not match(JSONExtractString(labels, 'code'), 'ERROR_*')") }) } @@ -69,28 +70,28 @@ func TestBuildQueryWithMultipleQueries(t *testing.T) { Step: 60, CompositeMetricQuery: &model.CompositeMetricQuery{ BuilderQueries: map[string]*model.MetricQuery{ - "a": { - QueryName: "a", + "A": { + QueryName: "A", MetricName: "name", TagFilters: &model.FilterSet{Operator: "AND", Items: []model.FilterItem{ {Key: "in", Value: []interface{}{"a", "b", "c"}, Operator: "in"}, }}, AggregateOperator: model.RATE_AVG, - Expression: "a", + Expression: "A", }, - "b": { - QueryName: "b", + "B": { + QueryName: "B", MetricName: "name2", AggregateOperator: model.RATE_MAX, - Expression: "b", + Expression: "B", }, }, }, } queries := PrepareBuilderMetricQueries(q, "table").Queries So(len(queries), ShouldEqual, 2) - So(queries["a"], ShouldContainSubstring, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'in') IN ['a','b','c']") - So(queries["a"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)") + So(queries["A"], ShouldContainSubstring, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'in') IN ['a','b','c']") + So(queries["A"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)") }) } @@ -102,31 +103,136 @@ func TestBuildQueryWithMultipleQueriesAndFormula(t *testing.T) { Step: 60, CompositeMetricQuery: &model.CompositeMetricQuery{ BuilderQueries: map[string]*model.MetricQuery{ - "a": { - QueryName: "a", + "A": { + QueryName: "A", MetricName: "name", TagFilters: &model.FilterSet{Operator: "AND", Items: []model.FilterItem{ {Key: "in", Value: []interface{}{"a", "b", "c"}, Operator: "in"}, }}, AggregateOperator: model.RATE_MAX, - Expression: "a", + Expression: "A", }, - "b": { + "B": { MetricName: "name2", AggregateOperator: model.RATE_AVG, - Expression: "b", + Expression: "B", }, - "c": { - QueryName: "c", - Expression: "a/b", + "C": { + QueryName: "C", + Expression: "A/B", }, }, }, } queries := PrepareBuilderMetricQueries(q, "table").Queries So(len(queries), ShouldEqual, 3) - So(queries["c"], ShouldContainSubstring, "SELECT ts, a.value / b.value") - So(queries["c"], ShouldContainSubstring, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'in') IN ['a','b','c']") - So(queries["c"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)") + So(queries["C"], ShouldContainSubstring, "SELECT A.ts as ts, A.value / B.value") + So(queries["C"], ShouldContainSubstring, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'in') IN ['a','b','c']") + So(queries["C"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)") + }) +} + +func TestBuildQueryWithIncorrectQueryRef(t *testing.T) { + Convey("TestBuildQueryWithFilters", t, func() { + q := &model.QueryRangeParamsV2{ + Start: 1650991982000, + End: 1651078382000, + Step: 60, + CompositeMetricQuery: &model.CompositeMetricQuery{ + BuilderQueries: map[string]*model.MetricQuery{ + "A": { + QueryName: "A", + MetricName: "name", + TagFilters: &model.FilterSet{Operator: "AND", Items: []model.FilterItem{ + {Key: "in", Value: []interface{}{"a", "b", "c"}, Operator: "in"}, + }}, + AggregateOperator: model.RATE_MAX, + Expression: "A", + }, + "C": { + QueryName: "C", + Expression: "D*2", + }, + }, + }, + } + res := PrepareBuilderMetricQueries(q, "table") + So(res.Err, ShouldNotBeNil) + So(res.Err.Error(), ShouldContainSubstring, "variable D not found in builder queries") + }) +} + +func TestBuildQueryWithThreeOrMoreQueriesRefAndFormula(t *testing.T) { + Convey("TestBuildQueryWithFilters", t, func() { + q := &model.QueryRangeParamsV2{ + Start: 1650991982000, + End: 1651078382000, + Step: 60, + CompositeMetricQuery: &model.CompositeMetricQuery{ + BuilderQueries: map[string]*model.MetricQuery{ + "A": { + QueryName: "A", + MetricName: "name", + TagFilters: &model.FilterSet{Operator: "AND", Items: []model.FilterItem{ + {Key: "in", Value: []interface{}{"a", "b", "c"}, Operator: "in"}, + }}, + AggregateOperator: model.RATE_MAX, + Expression: "A", + Disabled: true, + }, + "B": { + MetricName: "name2", + AggregateOperator: model.RATE_AVG, + Expression: "B", + Disabled: true, + }, + "C": { + MetricName: "name3", + AggregateOperator: model.SUM_RATE, + Expression: "C", + Disabled: true, + }, + "F1": { + QueryName: "F1", + Expression: "A/B", + }, + "F2": { + QueryName: "F2", + Expression: "A/(B+C)", + }, + "F3": { + QueryName: "F3", + Expression: "A*A", + }, + "F4": { + QueryName: "F4", + Expression: "A*B*C", + }, + "F5": { + QueryName: "F5", + Expression: "((A - B) / B) * 100", + }, + }, + }, + } + res := PrepareBuilderMetricQueries(q, "table") + So(res.Err, ShouldBeNil) + queries := res.Queries + So(len(queries), ShouldEqual, 5) + So(queries["F1"], ShouldContainSubstring, "SELECT A.ts as ts, A.value / B.value") + So(strings.Count(queries["F1"], " ON "), ShouldEqual, 1) + + So(queries["F2"], ShouldContainSubstring, "SELECT A.ts as ts, A.value / (B.value + C.value)") + So(strings.Count(queries["F2"], " ON "), ShouldEqual, 2) + + // Working with same query multiple times should not join on itself + So(queries["F3"], ShouldNotContainSubstring, " ON ") + + So(queries["F4"], ShouldContainSubstring, "SELECT A.ts as ts, A.value * B.value * C.value") + // Number of times JOIN ON appears is N-1 where N is number of unique queries + So(strings.Count(queries["F4"], " ON "), ShouldEqual, 2) + + So(queries["F5"], ShouldContainSubstring, "SELECT A.ts as ts, ((A.value - B.value) / B.value) * 100") + So(strings.Count(queries["F5"], " ON "), ShouldEqual, 1) }) }