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 <prashant@signoz.io>

.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: |

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

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, ",")
+		if joinUsing == "" {
-		formulaSubQuery += fmt.Sprintf("(%s) as %s ", query, var_)
+			for _, tag := range groupTags {
-		if idx < len(vars)-1 {
+				joinUsing += fmt.Sprintf("%s.%s as %s, ", var_, tag, tag)
-			formulaSubQuery += "GLOBAL INNER JOIN"
+			}
-		} else if len(vars) > 1 {
+			joinUsing = strings.TrimSuffix(joinUsing, ", ")
-			formulaSubQuery += fmt.Sprintf("USING (%s)", 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

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": {
+					"A": {
-						QueryName:         "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, "WHERE metric_name = 'name'")
-		So(queries["a"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)")
+		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": {
+					"A": {
-						QueryName:  "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, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'a') != 'b'")
-		So(queries["a"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)")
+		So(queries["A"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)")
-		So(queries["a"], ShouldContainSubstring, "not match(JSONExtractString(labels, 'code'), 'ERROR_*')")
+		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": {
+					"A": {
-						QueryName:  "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": {
+					"B": {
-						QueryName:         "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, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'in') IN ['a','b','c']")
-		So(queries["a"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)")
+		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": {
+					"A": {
-						QueryName:  "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": {
+					"C": {
-						QueryName:  "c",
+						QueryName:  "C",
-						Expression: "a/b",
+						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, "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, "WHERE metric_name = 'name' AND JSONExtractString(labels, 'in') IN ['a','b','c']")
-		So(queries["c"], ShouldContainSubstring, "runningDifference(value)/runningDifference(ts)")
+		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)
 	})
 }