From dc855cc2c289098428c57a86a73abe3d5bd77ac8 Mon Sep 17 00:00:00 2001 From: kobecal Date: Fri, 13 Sep 2024 17:42:58 +0800 Subject: [PATCH 1/4] fix: check alert rule queries are all disabled if at least one query is set --- pkg/query-service/rules/api_params.go | 31 ++++++++++ pkg/query-service/rules/api_params_test.go | 70 ++++++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 pkg/query-service/rules/api_params_test.go diff --git a/pkg/query-service/rules/api_params.go b/pkg/query-service/rules/api_params.go index 6d3288ece1..4aa43f6595 100644 --- a/pkg/query-service/rules/api_params.go +++ b/pkg/query-service/rules/api_params.go @@ -168,18 +168,49 @@ func isValidLabelValue(v string) bool { return utf8.ValidString(v) } +func isAllQueriesDisabled(compositeQuery *v3.CompositeQuery) bool { + if compositeQuery == nil { + return false + } + if compositeQuery.BuilderQueries == nil && compositeQuery.PromQueries == nil && compositeQuery.ClickHouseQueries == nil { + return false + } + for _, query := range compositeQuery.BuilderQueries { + if !query.Disabled { + return false + } + } + for _, query := range compositeQuery.PromQueries { + if !query.Disabled { + return false + } + } + for _, query := range compositeQuery.ClickHouseQueries { + if !query.Disabled { + return false + } + } + return true +} + func (r *PostableRule) Validate() error { var errs []error if r.RuleCondition == nil { errs = append(errs, errors.Errorf("rule condition is required")) + // will get panic if we try to access CompositeQuery, so return here + return multierr.Combine(errs...) } else { if r.RuleCondition.CompositeQuery == nil { errs = append(errs, errors.Errorf("composite metric query is required")) } } + if isAllQueriesDisabled(r.RuleCondition.CompositeQuery) { + errs = append(errs, errors.Errorf("all queries are disabled in rule condition")) + } + if r.RuleType == RuleTypeThreshold { if r.RuleCondition.Target == nil { errs = append(errs, errors.Errorf("rule condition missing the threshold")) diff --git a/pkg/query-service/rules/api_params_test.go b/pkg/query-service/rules/api_params_test.go new file mode 100644 index 0000000000..5a29cee91d --- /dev/null +++ b/pkg/query-service/rules/api_params_test.go @@ -0,0 +1,70 @@ +package rules + +import ( + "testing" + + v3 "go.signoz.io/signoz/pkg/query-service/model/v3" +) + +func TestIsAllQueriesDisabled(t *testing.T) { + // Test case for all queries disabled + compositeQuery := &v3.CompositeQuery{ + BuilderQueries: map[string]*v3.BuilderQuery{ + "query1": { + Disabled: true, + }, + "query2": { + Disabled: true, + }, + }, + PromQueries: map[string]*v3.PromQuery{ + "query3": { + Disabled: true, + }, + "query4": { + Disabled: true, + }, + }, + ClickHouseQueries: map[string]*v3.ClickHouseQuery{ + "query5": { + Disabled: true, + }, + "query6": { + Disabled: true, + }, + }, + } + + testCases := []*v3.CompositeQuery{ + compositeQuery, + nil, + &v3.CompositeQuery{}, + &v3.CompositeQuery{ + BuilderQueries: map[string]*v3.BuilderQuery{ + "query1": { + Disabled: true, + }, + "query2": { + Disabled: false, + }, + }, + }, + &v3.CompositeQuery{ + PromQueries: map[string]*v3.PromQuery{ + "query3": { + Disabled: false, + }, + }, + }, + } + + expectedResult := []bool{true, false, false, false, false} + + for index, compositeQuery := range testCases { + expected := expectedResult[index] + actual := isAllQueriesDisabled(compositeQuery) + if actual != expected { + t.Errorf("Expected %v, but got %v", expected, actual) + } + } +} From e56b58b157f8301146dc7891739ff0c6aa96710e Mon Sep 17 00:00:00 2001 From: kobecal Date: Thu, 19 Sep 2024 16:09:58 +0800 Subject: [PATCH 2/4] fix: check alert rule queries are all disabled if at least one query is set --- pkg/query-service/rules/api_params.go | 37 +++++++++++- pkg/query-service/rules/api_params_test.go | 70 ++++++++++++++++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 pkg/query-service/rules/api_params_test.go diff --git a/pkg/query-service/rules/api_params.go b/pkg/query-service/rules/api_params.go index 6d3288ece1..289a2545bf 100644 --- a/pkg/query-service/rules/api_params.go +++ b/pkg/query-service/rules/api_params.go @@ -168,18 +168,53 @@ func isValidLabelValue(v string) bool { return utf8.ValidString(v) } +func isAllQueriesDisabled(compositeQuery *v3.CompositeQuery) bool { + if compositeQuery == nil { + return false + } + if compositeQuery.BuilderQueries == nil && compositeQuery.PromQueries == nil && compositeQuery.ClickHouseQueries == nil { + return false + } + switch compositeQuery.QueryType { + case v3.QueryTypeBuilder: + for _, query := range compositeQuery.BuilderQueries { + if !query.Disabled { + return false + } + } + case v3.QueryTypePromQL: + for _, query := range compositeQuery.PromQueries { + if !query.Disabled { + return false + } + } + case v3.QueryTypeClickHouseSQL: + for _, query := range compositeQuery.ClickHouseQueries { + if !query.Disabled { + return false + } + } + } + return true +} + func (r *PostableRule) Validate() error { var errs []error if r.RuleCondition == nil { - errs = append(errs, errors.Errorf("rule condition is required")) + // will get panic if we try to access CompositeQuery, so return here + return errors.Errorf("rule condition is required") } else { if r.RuleCondition.CompositeQuery == nil { errs = append(errs, errors.Errorf("composite metric query is required")) } } + if isAllQueriesDisabled(r.RuleCondition.CompositeQuery) { + errs = append(errs, errors.Errorf("all queries are disabled in rule condition")) + } + if r.RuleType == RuleTypeThreshold { if r.RuleCondition.Target == nil { errs = append(errs, errors.Errorf("rule condition missing the threshold")) diff --git a/pkg/query-service/rules/api_params_test.go b/pkg/query-service/rules/api_params_test.go new file mode 100644 index 0000000000..5a29cee91d --- /dev/null +++ b/pkg/query-service/rules/api_params_test.go @@ -0,0 +1,70 @@ +package rules + +import ( + "testing" + + v3 "go.signoz.io/signoz/pkg/query-service/model/v3" +) + +func TestIsAllQueriesDisabled(t *testing.T) { + // Test case for all queries disabled + compositeQuery := &v3.CompositeQuery{ + BuilderQueries: map[string]*v3.BuilderQuery{ + "query1": { + Disabled: true, + }, + "query2": { + Disabled: true, + }, + }, + PromQueries: map[string]*v3.PromQuery{ + "query3": { + Disabled: true, + }, + "query4": { + Disabled: true, + }, + }, + ClickHouseQueries: map[string]*v3.ClickHouseQuery{ + "query5": { + Disabled: true, + }, + "query6": { + Disabled: true, + }, + }, + } + + testCases := []*v3.CompositeQuery{ + compositeQuery, + nil, + &v3.CompositeQuery{}, + &v3.CompositeQuery{ + BuilderQueries: map[string]*v3.BuilderQuery{ + "query1": { + Disabled: true, + }, + "query2": { + Disabled: false, + }, + }, + }, + &v3.CompositeQuery{ + PromQueries: map[string]*v3.PromQuery{ + "query3": { + Disabled: false, + }, + }, + }, + } + + expectedResult := []bool{true, false, false, false, false} + + for index, compositeQuery := range testCases { + expected := expectedResult[index] + actual := isAllQueriesDisabled(compositeQuery) + if actual != expected { + t.Errorf("Expected %v, but got %v", expected, actual) + } + } +} From 144adc0a901a780c5801697451b27d1eab2a2947 Mon Sep 17 00:00:00 2001 From: Kobe Cai Date: Thu, 19 Sep 2024 16:25:48 +0800 Subject: [PATCH 3/4] style: switch QueryType and return rule condition is required error directly --- pkg/query-service/rules/api_params.go | 30 +++++++++++++++------------ 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/pkg/query-service/rules/api_params.go b/pkg/query-service/rules/api_params.go index 4aa43f6595..289a2545bf 100644 --- a/pkg/query-service/rules/api_params.go +++ b/pkg/query-service/rules/api_params.go @@ -175,19 +175,24 @@ func isAllQueriesDisabled(compositeQuery *v3.CompositeQuery) bool { if compositeQuery.BuilderQueries == nil && compositeQuery.PromQueries == nil && compositeQuery.ClickHouseQueries == nil { return false } - for _, query := range compositeQuery.BuilderQueries { - if !query.Disabled { - return false + switch compositeQuery.QueryType { + case v3.QueryTypeBuilder: + for _, query := range compositeQuery.BuilderQueries { + if !query.Disabled { + return false + } } - } - for _, query := range compositeQuery.PromQueries { - if !query.Disabled { - return false + case v3.QueryTypePromQL: + for _, query := range compositeQuery.PromQueries { + if !query.Disabled { + return false + } } - } - for _, query := range compositeQuery.ClickHouseQueries { - if !query.Disabled { - return false + case v3.QueryTypeClickHouseSQL: + for _, query := range compositeQuery.ClickHouseQueries { + if !query.Disabled { + return false + } } } return true @@ -198,9 +203,8 @@ func (r *PostableRule) Validate() error { var errs []error if r.RuleCondition == nil { - errs = append(errs, errors.Errorf("rule condition is required")) // will get panic if we try to access CompositeQuery, so return here - return multierr.Combine(errs...) + return errors.Errorf("rule condition is required") } else { if r.RuleCondition.CompositeQuery == nil { errs = append(errs, errors.Errorf("composite metric query is required")) From b31d4b80a5c800149e75abdcf3e599a9c471b635 Mon Sep 17 00:00:00 2001 From: kobecal Date: Thu, 19 Sep 2024 16:44:14 +0800 Subject: [PATCH 4/4] test: update is all queries disabled test case --- pkg/query-service/rules/api_params.go | 9 +++ pkg/query-service/rules/api_params_test.go | 76 +++++++++++++--------- 2 files changed, 55 insertions(+), 30 deletions(-) diff --git a/pkg/query-service/rules/api_params.go b/pkg/query-service/rules/api_params.go index 289a2545bf..eb747873f0 100644 --- a/pkg/query-service/rules/api_params.go +++ b/pkg/query-service/rules/api_params.go @@ -177,18 +177,27 @@ func isAllQueriesDisabled(compositeQuery *v3.CompositeQuery) bool { } switch compositeQuery.QueryType { case v3.QueryTypeBuilder: + if len(compositeQuery.BuilderQueries) == 0 { + return false + } for _, query := range compositeQuery.BuilderQueries { if !query.Disabled { return false } } case v3.QueryTypePromQL: + if len(compositeQuery.PromQueries) == 0 { + return false + } for _, query := range compositeQuery.PromQueries { if !query.Disabled { return false } } case v3.QueryTypeClickHouseSQL: + if len(compositeQuery.ClickHouseQueries) == 0 { + return false + } for _, query := range compositeQuery.ClickHouseQueries { if !query.Disabled { return false diff --git a/pkg/query-service/rules/api_params_test.go b/pkg/query-service/rules/api_params_test.go index 5a29cee91d..6a1245d0fe 100644 --- a/pkg/query-service/rules/api_params_test.go +++ b/pkg/query-service/rules/api_params_test.go @@ -7,39 +7,24 @@ import ( ) func TestIsAllQueriesDisabled(t *testing.T) { - // Test case for all queries disabled - compositeQuery := &v3.CompositeQuery{ - BuilderQueries: map[string]*v3.BuilderQuery{ - "query1": { - Disabled: true, - }, - "query2": { - Disabled: true, - }, - }, - PromQueries: map[string]*v3.PromQuery{ - "query3": { - Disabled: true, - }, - "query4": { - Disabled: true, - }, - }, - ClickHouseQueries: map[string]*v3.ClickHouseQuery{ - "query5": { - Disabled: true, - }, - "query6": { - Disabled: true, + testCases := []*v3.CompositeQuery{ + &v3.CompositeQuery{ + BuilderQueries: map[string]*v3.BuilderQuery{ + "query1": { + Disabled: true, + }, + "query2": { + Disabled: true, + }, }, + QueryType: v3.QueryTypeBuilder, }, - } - - testCases := []*v3.CompositeQuery{ - compositeQuery, nil, - &v3.CompositeQuery{}, &v3.CompositeQuery{ + QueryType: v3.QueryTypeBuilder, + }, + &v3.CompositeQuery{ + QueryType: v3.QueryTypeBuilder, BuilderQueries: map[string]*v3.BuilderQuery{ "query1": { Disabled: true, @@ -50,15 +35,46 @@ func TestIsAllQueriesDisabled(t *testing.T) { }, }, &v3.CompositeQuery{ + QueryType: v3.QueryTypePromQL, + }, + &v3.CompositeQuery{ + QueryType: v3.QueryTypePromQL, PromQueries: map[string]*v3.PromQuery{ "query3": { Disabled: false, }, }, }, + &v3.CompositeQuery{ + QueryType: v3.QueryTypePromQL, + PromQueries: map[string]*v3.PromQuery{ + "query3": { + Disabled: true, + }, + }, + }, + &v3.CompositeQuery{ + QueryType: v3.QueryTypeClickHouseSQL, + }, + &v3.CompositeQuery{ + QueryType: v3.QueryTypeClickHouseSQL, + ClickHouseQueries: map[string]*v3.ClickHouseQuery{ + "query4": { + Disabled: false, + }, + }, + }, + &v3.CompositeQuery{ + QueryType: v3.QueryTypeClickHouseSQL, + ClickHouseQueries: map[string]*v3.ClickHouseQuery{ + "query4": { + Disabled: true, + }, + }, + }, } - expectedResult := []bool{true, false, false, false, false} + expectedResult := []bool{true, false, false, false, false, false, true, false, false, true} for index, compositeQuery := range testCases { expected := expectedResult[index]