From df0502726d8b9793e70af84233410a87c6ddc1c3 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Fri, 12 May 2023 16:25:22 +0530 Subject: [PATCH] fix: resolve gaps identified in the query builder (#2680) --- .../app/metrics/v3/query_builder.go | 24 ++- .../app/metrics/v3/query_builder_test.go | 138 ++++++++++++++++++ pkg/query-service/app/parser.go | 6 - pkg/query-service/app/parser_test.go | 2 +- .../app/queryBuilder/query_builder.go | 7 + .../app/queryBuilder/query_builder_test.go | 9 ++ pkg/query-service/model/v3/v3.go | 22 +-- 7 files changed, 184 insertions(+), 24 deletions(-) diff --git a/pkg/query-service/app/metrics/v3/query_builder.go b/pkg/query-service/app/metrics/v3/query_builder.go index bbba81fdd6..82995d9285 100644 --- a/pkg/query-service/app/metrics/v3/query_builder.go +++ b/pkg/query-service/app/metrics/v3/query_builder.go @@ -61,6 +61,10 @@ func buildMetricsTimeSeriesFilterQuery(fs *v3.FilterSet, groupTags []v3.Attribut toFormat = x[0] } } + + if op == v3.FilterOperatorContains || op == v3.FilterOperatorNotContains { + toFormat = fmt.Sprintf("%%%s%%", toFormat) + } fmtVal := utils.ClickHouseFormattedValue(toFormat) switch op { case v3.FilterOperatorEqual: @@ -92,9 +96,9 @@ func buildMetricsTimeSeriesFilterQuery(fs *v3.FilterSet, groupTags []v3.Attribut case v3.FilterOperatorNotContains: conditions = append(conditions, fmt.Sprintf("notLike(JSONExtractString(labels, '%s'), %s)", item.Key.Key, fmtVal)) case v3.FilterOperatorExists: - conditions = append(conditions, fmt.Sprintf("has(JSONExtractKeys(labels), %s)", item.Key.Key)) + conditions = append(conditions, fmt.Sprintf("has(JSONExtractKeys(labels), '%s')", item.Key.Key)) case v3.FilterOperatorNotExists: - conditions = append(conditions, fmt.Sprintf("not has(JSONExtractKeys(labels), %s)", item.Key.Key)) + conditions = append(conditions, fmt.Sprintf("not has(JSONExtractKeys(labels), '%s')", item.Key.Key)) default: return "", fmt.Errorf("unsupported operation") } @@ -202,7 +206,7 @@ func buildMetricQuery(start, end, step int64, mq *v3.BuilderQuery, tableName str subQuery := fmt.Sprintf( queryTmpl, "any(labels) as labels, "+groupTags, step, op, filterSubQuery, groupBy, orderBy, ) // labels will be same so any should be fine - query := `SELECT %s ts, ` + rateWithoutNegative + ` as value FROM(%s)` + query := `SELECT %s ts, ` + rateWithoutNegative + ` as value FROM(%s) WHERE isNaN(value) = 0` query = fmt.Sprintf(query, "labels as fullLabels,", subQuery) return query, nil @@ -214,7 +218,7 @@ func buildMetricQuery(start, end, step int64, mq *v3.BuilderQuery, tableName str subQuery := fmt.Sprintf( queryTmpl, rateGroupTags, step, op, filterSubQuery, rateGroupBy, rateOrderBy, ) // labels will be same so any should be fine - query := `SELECT %s ts, ` + rateWithoutNegative + `as value FROM(%s)` + query := `SELECT %s ts, ` + rateWithoutNegative + `as value FROM(%s) WHERE isNaN(value) = 0` query = fmt.Sprintf(query, groupTags, subQuery) query = fmt.Sprintf(`SELECT %s ts, sum(value) as value FROM (%s) GROUP BY %s ORDER BY %s ts`, groupTags, query, groupBy, orderBy) return query, nil @@ -225,7 +229,7 @@ func buildMetricQuery(start, end, step int64, mq *v3.BuilderQuery, tableName str v3.AggregateOperatorRateMin: op := fmt.Sprintf("%s(value)", aggregateOperatorToSQLFunc[mq.AggregateOperator]) subQuery := fmt.Sprintf(queryTmpl, groupTags, step, op, filterSubQuery, groupBy, orderBy) - query := `SELECT %s ts, ` + rateWithoutNegative + `as value FROM(%s)` + query := `SELECT %s ts, ` + rateWithoutNegative + `as value FROM(%s) WHERE isNaN(value) = 0` query = fmt.Sprintf(query, groupTags, subQuery) return query, nil case @@ -249,9 +253,9 @@ func buildMetricQuery(start, end, step int64, mq *v3.BuilderQuery, tableName str subQuery := fmt.Sprintf( queryTmpl, rateGroupTags, step, op, filterSubQuery, rateGroupBy, rateOrderBy, ) // labels will be same so any should be fine - query := `SELECT %s ts, ` + rateWithoutNegative + ` as value FROM(%s)` + query := `SELECT %s ts, ` + rateWithoutNegative + ` as value FROM(%s) WHERE isNaN(value) = 0` query = fmt.Sprintf(query, groupTags, subQuery) - query = fmt.Sprintf(`SELECT %s ts, sum(value) as value FROM (%s) GROUP BY %s ORDER BY %s ts`, groupTags, query, groupBy, orderBy) + query = fmt.Sprintf(`SELECT %s ts, sum(value) as value FROM (%s) GROUP BY %s HAVING isNaN(value) = 0 ORDER BY %s ts`, groupTags, query, groupBy, orderBy) value := aggregateOperatorToPercentile[mq.AggregateOperator] query = fmt.Sprintf(`SELECT %s ts, histogramQuantile(arrayMap(x -> toFloat64(x), groupArray(le)), groupArray(value), %.3f) as value FROM (%s) GROUP BY %s ORDER BY %s ts`, groupTagsWithoutLe, value, query, groupByWithoutLe, orderWithoutLe) @@ -350,7 +354,7 @@ func orderByAttributeKeyTags(items []v3.OrderBy, tags []v3.AttributeKey) string func having(items []v3.Having) string { var having []string for _, item := range items { - having = append(having, fmt.Sprintf("%s %s %v", item.ColumnName, item.Operator, utils.ClickHouseFormattedValue(item.Value))) + having = append(having, fmt.Sprintf("%s %s %v", "value", item.Operator, utils.ClickHouseFormattedValue(item.Value))) } return strings.Join(having, " AND ") } @@ -390,6 +394,10 @@ func PrepareMetricQuery(start, end int64, queryType v3.QueryType, panelType v3.P if err != nil { return "", err } + if having(mq.Having) != "" { + query = fmt.Sprintf("SELECT * FROM (%s) HAVING %s", query, having(mq.Having)) + } + if panelType == v3.PanelTypeValue { query, err = reduceQuery(query, mq.ReduceTo, mq.AggregateOperator) } diff --git a/pkg/query-service/app/metrics/v3/query_builder_test.go b/pkg/query-service/app/metrics/v3/query_builder_test.go index bcb9cfc69e..ff35ed15f2 100644 --- a/pkg/query-service/app/metrics/v3/query_builder_test.go +++ b/pkg/query-service/app/metrics/v3/query_builder_test.go @@ -1,6 +1,7 @@ package v3 import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -96,3 +97,140 @@ func TestBuildQueryWithMultipleQueries(t *testing.T) { require.Contains(t, query, rateWithoutNegative) }) } + +func TestBuildQueryOperators(t *testing.T) { + testCases := []struct { + operator v3.FilterOperator + filterSet v3.FilterSet + expectedWhereClause string + }{ + { + operator: v3.FilterOperatorEqual, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "service_name"}, Value: "route", Operator: v3.FilterOperatorEqual}, + }, + }, + expectedWhereClause: "JSONExtractString(labels, 'service_name') = 'route'", + }, + { + operator: v3.FilterOperatorNotEqual, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "service_name"}, Value: "route", Operator: v3.FilterOperatorNotEqual}, + }, + }, + expectedWhereClause: "JSONExtractString(labels, 'service_name') != 'route'", + }, + { + operator: v3.FilterOperatorRegex, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "service_name"}, Value: "out", Operator: v3.FilterOperatorRegex}, + }, + }, + expectedWhereClause: "match(JSONExtractString(labels, 'service_name'), 'out')", + }, + { + operator: v3.FilterOperatorNotRegex, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "service_name"}, Value: "out", Operator: v3.FilterOperatorNotRegex}, + }, + }, + expectedWhereClause: "not match(JSONExtractString(labels, 'service_name'), 'out')", + }, + { + operator: v3.FilterOperatorIn, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "service_name"}, Value: []interface{}{"route", "driver"}, Operator: v3.FilterOperatorIn}, + }, + }, + expectedWhereClause: "JSONExtractString(labels, 'service_name') IN ['route','driver']", + }, + { + operator: v3.FilterOperatorNotIn, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "service_name"}, Value: []interface{}{"route", "driver"}, Operator: v3.FilterOperatorNotIn}, + }, + }, + expectedWhereClause: "JSONExtractString(labels, 'service_name') NOT IN ['route','driver']", + }, + { + operator: v3.FilterOperatorExists, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "horn"}, Operator: v3.FilterOperatorExists}, + }, + }, + expectedWhereClause: "has(JSONExtractKeys(labels), 'horn')", + }, + { + operator: v3.FilterOperatorNotExists, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "horn"}, Operator: v3.FilterOperatorNotExists}, + }, + }, + expectedWhereClause: "not has(JSONExtractKeys(labels), 'horn')", + }, + { + operator: v3.FilterOperatorContains, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "service_name"}, Value: "out", Operator: v3.FilterOperatorContains}, + }, + }, + expectedWhereClause: "like(JSONExtractString(labels, 'service_name'), '%out%')", + }, + { + operator: v3.FilterOperatorNotContains, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "serice_name"}, Value: "out", Operator: v3.FilterOperatorNotContains}, + }, + }, + expectedWhereClause: "notLike(JSONExtractString(labels, 'serice_name'), '%out%')", + }, + { + operator: v3.FilterOperatorLike, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "service_name"}, Value: "dri", Operator: v3.FilterOperatorLike}, + }, + }, + expectedWhereClause: "like(JSONExtractString(labels, 'service_name'), 'dri')", + }, + { + operator: v3.FilterOperatorNotLike, + filterSet: v3.FilterSet{ + Operator: "AND", + Items: []v3.FilterItem{ + {Key: v3.AttributeKey{Key: "serice_name"}, Value: "dri", Operator: v3.FilterOperatorNotLike}, + }, + }, + expectedWhereClause: "notLike(JSONExtractString(labels, 'serice_name'), 'dri')", + }, + } + + for i, tc := range testCases { + t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) { + whereClause, err := buildMetricsTimeSeriesFilterQuery(&tc.filterSet, []v3.AttributeKey{}, "signoz_calls_total", "sum") + require.NoError(t, err) + require.Contains(t, whereClause, tc.expectedWhereClause) + }) + } +} diff --git a/pkg/query-service/app/parser.go b/pkg/query-service/app/parser.go index 5167dbce1b..4804847ea2 100644 --- a/pkg/query-service/app/parser.go +++ b/pkg/query-service/app/parser.go @@ -836,7 +836,6 @@ func parseFilterAttributeKeyRequest(r *http.Request) (*v3.FilterAttributeKeyRequ dataSource := v3.DataSource(r.URL.Query().Get("dataSource")) aggregateOperator := v3.AggregateOperator(r.URL.Query().Get("aggregateOperator")) aggregateAttribute := r.URL.Query().Get("aggregateAttribute") - tagType := v3.TagType(r.URL.Query().Get("tagType")) limit, err := strconv.Atoi(r.URL.Query().Get("limit")) if err != nil { @@ -851,15 +850,10 @@ func parseFilterAttributeKeyRequest(r *http.Request) (*v3.FilterAttributeKeyRequ return nil, err } - if err := tagType.Validate(); err != nil && tagType != v3.TagType("") { - return nil, err - } - req = v3.FilterAttributeKeyRequest{ DataSource: dataSource, AggregateOperator: aggregateOperator, AggregateAttribute: aggregateAttribute, - TagType: tagType, Limit: limit, SearchText: r.URL.Query().Get("searchText"), } diff --git a/pkg/query-service/app/parser_test.go b/pkg/query-service/app/parser_test.go index ea3eb97854..df2a4e79e6 100644 --- a/pkg/query-service/app/parser_test.go +++ b/pkg/query-service/app/parser_test.go @@ -518,7 +518,7 @@ func TestParseQueryRangeParamsCompositeQuery(t *testing.T) { BuilderQueries: map[string]*v3.BuilderQuery{ "A": { QueryName: "A", - DataSource: "traces", + DataSource: "metrics", AggregateOperator: "sum", AggregateAttribute: v3.AttributeKey{}, Expression: "A", diff --git a/pkg/query-service/app/queryBuilder/query_builder.go b/pkg/query-service/app/queryBuilder/query_builder.go index b82a85ca4a..173a139aa9 100644 --- a/pkg/query-service/app/queryBuilder/query_builder.go +++ b/pkg/query-service/app/queryBuilder/query_builder.go @@ -183,5 +183,12 @@ func (qb *QueryBuilder) PrepareQueries(params *v3.QueryRangeParamsV3, args ...in } } } + + // filter out disabled queries + for queryName := range queries { + if compositeQuery.BuilderQueries[queryName].Disabled { + delete(queries, queryName) + } + } return queries, nil } diff --git a/pkg/query-service/app/queryBuilder/query_builder_test.go b/pkg/query-service/app/queryBuilder/query_builder_test.go index 3716e2bb9f..aacb895e75 100644 --- a/pkg/query-service/app/queryBuilder/query_builder_test.go +++ b/pkg/query-service/app/queryBuilder/query_builder_test.go @@ -28,6 +28,7 @@ func TestBuildQueryWithMultipleQueriesAndFormula(t *testing.T) { Expression: "A", }, "B": { + QueryName: "B", AggregateAttribute: v3.AttributeKey{Key: "name2"}, DataSource: v3.DataSourceMetrics, AggregateOperator: v3.AggregateOperatorRateAvg, @@ -112,6 +113,7 @@ func TestBuildQueryWithThreeOrMoreQueriesRefAndFormula(t *testing.T) { Disabled: true, }, "B": { + QueryName: "B", AggregateAttribute: v3.AttributeKey{Key: "name2"}, DataSource: v3.DataSourceMetrics, @@ -120,6 +122,7 @@ func TestBuildQueryWithThreeOrMoreQueriesRefAndFormula(t *testing.T) { Disabled: true, }, "C": { + QueryName: "C", AggregateAttribute: v3.AttributeKey{Key: "name3"}, DataSource: v3.DataSourceMetrics, @@ -175,6 +178,12 @@ func TestBuildQueryWithThreeOrMoreQueriesRefAndFormula(t *testing.T) { require.Contains(t, queries["F5"], "SELECT A.ts as ts, ((A.value - B.value) / B.value) * 100") require.Equal(t, 1, strings.Count(queries["F5"], " ON ")) + for _, query := range q.CompositeQuery.BuilderQueries { + if query.Disabled { + require.NotContains(t, queries, query.QueryName) + } + } + // res := PrepareBuilderMetricQueries(q, "table") // So(res.Err, ShouldBeNil) // queries := res.Queries diff --git a/pkg/query-service/model/v3/v3.go b/pkg/query-service/model/v3/v3.go index bfef25b843..4065fb0668 100644 --- a/pkg/query-service/model/v3/v3.go +++ b/pkg/query-service/model/v3/v3.go @@ -101,14 +101,19 @@ func (a AggregateOperator) Validate() error { // RequireAttribute returns true if the aggregate operator requires an attribute // to be specified. -func (a AggregateOperator) RequireAttribute() bool { - switch a { - case AggregateOperatorNoOp, - AggregateOperatorCount, - AggregateOperatorCountDistinct: - return false +func (a AggregateOperator) RequireAttribute(dataSource DataSource) bool { + switch dataSource { + case DataSourceMetrics: + switch a { + case AggregateOperatorNoOp, + AggregateOperatorCount, + AggregateOperatorCountDistinct: + return false + default: + return true + } default: - return true + return false } } @@ -202,7 +207,6 @@ type FilterAttributeKeyRequest struct { DataSource DataSource `json:"dataSource"` AggregateOperator AggregateOperator `json:"aggregateOperator"` AggregateAttribute string `json:"aggregateAttribute"` - TagType TagType `json:"tagType"` SearchText string `json:"searchText"` Limit int `json:"limit"` } @@ -424,7 +428,7 @@ func (b *BuilderQuery) Validate() error { if err := b.AggregateOperator.Validate(); err != nil { return fmt.Errorf("aggregate operator is invalid: %w", err) } - if b.AggregateAttribute == (AttributeKey{}) && b.AggregateOperator.RequireAttribute() { + if b.AggregateAttribute == (AttributeKey{}) && b.AggregateOperator.RequireAttribute(b.DataSource) { return fmt.Errorf("aggregate attribute is required") } }