Merge branch 'main' into add-validation

pkg/query-service/model/v3/v3.go

@@ -597,11 +597,31 @@ func (c *CompositeQuery) Validate() error {
 		return fmt.Errorf("composite query is required")
 	}
 
-	if c.BuilderQueries == nil && c.ClickHouseQueries == nil && c.PromQueries == nil {
+	if err := c.PanelType.Validate(); err != nil {
-		return fmt.Errorf("composite query must contain at least one query type")
+		return fmt.Errorf("panel type is invalid: %w", err)
+	}
+
+	if err := c.QueryType.Validate(); err != nil {
+		return fmt.Errorf("query type is invalid: %w", err)
 	}
 
 	if c.QueryType == QueryTypeBuilder {
+		if c.BuilderQueries == nil {
+			return fmt.Errorf("at least one builder query must be provided")
+		}
+
+		// there should be at least one enabled query
+		enabled := false
+		for _, query := range c.BuilderQueries {
+			if !query.Disabled {
+				enabled = true
+				break
+			}
+		}
+		if !enabled {
+			return fmt.Errorf("at least one builder query must be enabled")
+		}
+
 		for name, query := range c.BuilderQueries {
 			if err := query.Validate(c.PanelType); err != nil {
 				return fmt.Errorf("builder query %s is invalid: %w", name, err)
@@ -610,6 +630,22 @@ func (c *CompositeQuery) Validate() error {
 	}
 
 	if c.QueryType == QueryTypeClickHouseSQL {
+		if c.ClickHouseQueries == nil {
+			return fmt.Errorf("at least one clickhouse query must be provided")
+		}
+
+		// there should be at least one enabled query
+		enabled := false
+		for _, query := range c.ClickHouseQueries {
+			if !query.Disabled {
+				enabled = true
+				break
+			}
+		}
+		if !enabled {
+			return fmt.Errorf("at least one clickhouse query must be enabled")
+		}
+
 		for name, query := range c.ClickHouseQueries {
 			if err := query.Validate(); err != nil {
 				return fmt.Errorf("clickhouse query %s is invalid: %w", name, err)
@@ -618,6 +654,22 @@ func (c *CompositeQuery) Validate() error {
 	}
 
 	if c.QueryType == QueryTypePromQL {
+		if c.PromQueries == nil {
+			return fmt.Errorf("at least one prom query must be provided")
+		}
+
+		// there should be at least one enabled query
+		enabled := false
+		for _, query := range c.PromQueries {
+			if !query.Disabled {
+				enabled = true
+				break
+			}
+		}
+		if !enabled {
+			return fmt.Errorf("at least one prom query must be enabled")
+		}
+
 		for name, query := range c.PromQueries {
 			if err := query.Validate(); err != nil {
 				return fmt.Errorf("prom query %s is invalid: %w", name, err)
@@ -625,14 +677,6 @@ func (c *CompositeQuery) Validate() error {
 		}
 	}
 
-	if err := c.PanelType.Validate(); err != nil {
-		return fmt.Errorf("panel type is invalid: %w", err)
-	}
-
-	if err := c.QueryType.Validate(); err != nil {
-		return fmt.Errorf("query type is invalid: %w", err)
-	}
-
 	return nil
 }
 

pkg/query-service/rules/alerting.go

@@ -94,6 +94,44 @@ const (
 	ValueOutsideBounds CompareOp = "7"
 )
 
+var (
+	supportedCompareOps = []CompareOp{CompareOpNone, ValueIsAbove, ValueIsBelow, ValueIsEq, ValueIsNotEq, ValueAboveOrEq, ValueBelowOrEq, ValueOutsideBounds}
+)
+
+func (co CompareOp) String() string {
+	switch co {
+	case CompareOpNone:
+		return "None: Enum value 0"
+	case ValueIsAbove:
+		return "ValueIsAbove: Enum value 1"
+	case ValueIsBelow:
+		return "ValueIsBelow: Enum value 2"
+	case ValueIsEq:
+		return "ValueIsEq: Enum value 3"
+	case ValueIsNotEq:
+		return "ValueIsNotEq: Enum value 4"
+	case ValueAboveOrEq:
+		return "ValueAboveOrEq: Enum value 5"
+	case ValueBelowOrEq:
+		return "ValueBelowOrEq: Enum value 6"
+	case ValueOutsideBounds:
+		return "ValueOutsideBounds: Enum value 7"
+	}
+	return "Unknown: Enum value"
+}
+
+func (co CompareOp) Validate() error {
+	var msg string
+	for _, op := range supportedCompareOps {
+		msg += op.String() + ", "
+	}
+	switch co {
+	case ValueIsAbove, ValueIsBelow, ValueIsEq, ValueIsNotEq, ValueAboveOrEq, ValueBelowOrEq, ValueOutsideBounds:
+		return nil
+	}
+	return fmt.Errorf("invalid compare op: %s supported ops are %s", co, msg)
+}
+
 type MatchType string
 
 const (
@@ -105,6 +143,40 @@ const (
 	Last          MatchType = "5"
 )
 
+var (
+	supportedMatchTypes = []MatchType{MatchTypeNone, AtleastOnce, AllTheTimes, OnAverage, InTotal, Last}
+)
+
+func (mt MatchType) String() string {
+	switch mt {
+	case MatchTypeNone:
+		return "None: Enum value 0"
+	case AtleastOnce:
+		return "AtleastOnce: Enum value 1"
+	case AllTheTimes:
+		return "AllTheTimes: Enum value 2"
+	case OnAverage:
+		return "OnAverage: Enum value 3"
+	case InTotal:
+		return "InTotal: Enum value 4"
+	case Last:
+		return "Last: Enum value 5"
+	}
+	return "Unknown: Enum value"
+}
+
+func (mt MatchType) Validate() error {
+	var msg string
+	for _, op := range supportedMatchTypes {
+		msg += op.String() + ", "
+	}
+	switch mt {
+	case MatchTypeNone, AtleastOnce, AllTheTimes, OnAverage, InTotal, Last:
+		return nil
+	}
+	return fmt.Errorf("invalid match type: %s supported ops are %s", mt, msg)
+}
+
 type RuleCondition struct {
 	CompositeQuery    *v3.CompositeQuery `json:"compositeQuery,omitempty" yaml:"compositeQuery,omitempty"`
 	CompareOp         CompareOp          `yaml:"op,omitempty" json:"op,omitempty"`
@@ -161,27 +233,34 @@ func (rc *RuleCondition) GetSelectedQueryName() string {
 	return ""
 }
 
-func (rc *RuleCondition) IsValid() bool {
+func (rc *RuleCondition) Validate() error {
 
 	if rc.CompositeQuery == nil {
-		return false
+		return ErrCompositeQueryRequired
 	}
 
 	if rc.QueryType() == v3.QueryTypeBuilder {
 		if rc.Target == nil {
-			return false
+			return ErrTargetRequired
 		}
 		if rc.CompareOp == "" {
-			return false
+			return ErrCompareOpRequired
+		}
+		if err := rc.CompareOp.Validate(); err != nil {
+			return err
+		}
+		if rc.MatchType == "" {
+			return ErrMatchTypeRequired
+		}
+		if err := rc.MatchType.Validate(); err != nil {
+			return err
 		}
 	}
-	if rc.QueryType() == v3.QueryTypePromQL {
 
-		if len(rc.CompositeQuery.PromQueries) == 0 {
+	if err := rc.CompositeQuery.Validate(); err != nil {
-			return false
+		return err
-		}
 	}
-	return true
+	return nil
 }
 
 // QueryType is a short hand method to get query type

pkg/query-service/rules/api_params.go

@@ -11,6 +11,7 @@ import (
 	v3 "github.com/SigNoz/signoz/pkg/query-service/model/v3"
 	"github.com/pkg/errors"
 	"go.uber.org/multierr"
+	"go.uber.org/zap"
 
 	"github.com/SigNoz/signoz/pkg/query-service/utils/times"
 	"github.com/SigNoz/signoz/pkg/query-service/utils/timestamp"
@@ -34,14 +35,16 @@ const (
 )
 
 var (
-	ErrFailedToParseJSON = errors.New("failed to parse json")
+	ErrFailedToParseJSON      = errors.New("failed to parse json")
-	ErrFailedToParseYAML = errors.New("failed to parse yaml")
+	ErrFailedToParseYAML      = errors.New("failed to parse yaml")
-	ErrInvalidDataType   = errors.New("invalid data type")
+	ErrInvalidDataType        = errors.New("invalid data type")
+	ErrRuleConditionRequired  = errors.New("rule condition is required")
+	ErrCompositeQueryRequired = errors.New("composite query is required")
+	ErrCompareOpRequired      = errors.New("compare op is required")
+	ErrTargetRequired         = errors.New("target is required")
+	ErrMatchTypeRequired      = errors.New("match type is required")
 )
 
-// this file contains api request and responses to be
-// served over http
-
 // PostableRule is used to create alerting rule from HTTP api
 type PostableRule struct {
 	AlertName   string    `yaml:"alert,omitempty" json:"alert,omitempty"`
@@ -65,8 +68,8 @@ type PostableRule struct {
 	Version string `json:"version,omitempty"`
 
 	// legacy
-	Expr    string `yaml:"expr,omitempty" json:"expr,omitempty"`
+	// TODO(srikanthccv): remove this if there are no legacy rules
-	OldYaml string `json:"yaml,omitempty"`
+	Expr string `yaml:"expr,omitempty" json:"expr,omitempty"`
 }
 
 func ParsePostableRule(content []byte) (*PostableRule, error) {
@@ -96,6 +99,8 @@ func parseIntoRule(initRule PostableRule, content []byte, kind RuleDataKind) (*P
 	}
 
 	if rule.RuleCondition == nil && rule.Expr != "" {
+		// there should be no legacy rules but just in case
+		zap.L().Info("legacy rule detected", zap.Any("rule", rule))
 		// account for legacy rules
 		rule.RuleType = RuleTypeProm
 		rule.EvalWindow = Duration(5 * time.Minute)
@@ -159,72 +164,16 @@ 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:
-		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
-			}
-		}
-	}
-	return true
-}
-
 func (r *PostableRule) Validate() error {
 
 	var errs []error
 
 	if r.RuleCondition == nil {
-		// will get panic if we try to access CompositeQuery, so return here
+		return ErrRuleConditionRequired
-		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) {
+	if err := r.RuleCondition.Validate(); err != nil {
-		errs = append(errs, errors.Errorf("all queries are disabled in rule condition"))
+		return err
-	}
-
-	if r.RuleType == RuleTypeThreshold {
-		if r.RuleCondition.Target == nil {
-			errs = append(errs, errors.Errorf("rule condition missing the threshold"))
-		}
-		if r.RuleCondition.CompareOp == "" {
-			errs = append(errs, errors.Errorf("rule condition missing the compare op"))
-		}
-		if r.RuleCondition.MatchType == "" {
-			errs = append(errs, errors.Errorf("rule condition missing the match option"))
-		}
 	}
 
 	for k, v := range r.Labels {

pkg/query-service/rules/api_params_test.go

@@ -7,80 +7,137 @@ import (
 )
 
 func TestIsAllQueriesDisabled(t *testing.T) {
-	testCases := []*v3.CompositeQuery{
+	type testCase struct {
+		compositeQuery *v3.CompositeQuery
+		expectedErr    bool
+	}
+
+	testCases := []testCase{
 		{
-			BuilderQueries: map[string]*v3.BuilderQuery{
+			compositeQuery: &v3.CompositeQuery{
-				"query1": {
+				BuilderQueries: map[string]*v3.BuilderQuery{
-					Disabled: true,
+					"query1": {
+						Disabled: true,
+					},
+					"query2": {
+						Disabled: true,
+					},
 				},
-				"query2": {
+				PanelType: v3.PanelTypeGraph,
-					Disabled: true,
+				QueryType: v3.QueryTypeBuilder,
+			},
+			expectedErr: true,
+		},
+		{
+			compositeQuery: &v3.CompositeQuery{
+				PanelType: v3.PanelTypeGraph,
+				QueryType: v3.QueryTypeBuilder,
+			},
+			expectedErr: true,
+		},
+		{
+			compositeQuery: &v3.CompositeQuery{
+				PanelType: v3.PanelTypeGraph,
+				QueryType: v3.QueryTypeBuilder,
+				BuilderQueries: map[string]*v3.BuilderQuery{
+					"query1": {
+						Disabled:     true,
+						QueryName:    "query1",
+						StepInterval: 60,
+						AggregateAttribute: v3.AttributeKey{
+							Key: "durationNano",
+						},
+						AggregateOperator: v3.AggregateOperatorP95,
+						DataSource:        v3.DataSourceTraces,
+						Expression:        "query1",
+					},
+					"query2": {
+						Disabled:     false,
+						QueryName:    "query2",
+						StepInterval: 60,
+						AggregateAttribute: v3.AttributeKey{
+							Key: "durationNano",
+						},
+						AggregateOperator: v3.AggregateOperatorP95,
+						DataSource:        v3.DataSourceTraces,
+						Expression:        "query2",
+					},
 				},
 			},
-			QueryType: v3.QueryTypeBuilder,
+			expectedErr: false,
-		},
-		nil,
-		{
-			QueryType: v3.QueryTypeBuilder,
 		},
 		{
-			QueryType: v3.QueryTypeBuilder,
+			compositeQuery: &v3.CompositeQuery{
-			BuilderQueries: map[string]*v3.BuilderQuery{
+				QueryType: v3.QueryTypePromQL,
-				"query1": {
+				PanelType: v3.PanelTypeGraph,
-					Disabled: true,
+			},
-				},
+			expectedErr: true,
-				"query2": {
+		},
-					Disabled: false,
+		{
+			compositeQuery: &v3.CompositeQuery{
+				QueryType: v3.QueryTypePromQL,
+				PanelType: v3.PanelTypeGraph,
+				PromQueries: map[string]*v3.PromQuery{
+					"query3": {
+						Disabled: false,
+						Query:    "query3",
+					},
 				},
 			},
+			expectedErr: true,
 		},
 		{
-			QueryType: v3.QueryTypePromQL,
+			compositeQuery: &v3.CompositeQuery{
-		},
+				QueryType: v3.QueryTypePromQL,
-		{
+				PanelType: v3.PanelTypeGraph,
-			QueryType: v3.QueryTypePromQL,
+				PromQueries: map[string]*v3.PromQuery{
-			PromQueries: map[string]*v3.PromQuery{
+					"query3": {
-				"query3": {
+						Disabled: true,
-					Disabled: false,
+					},
 				},
 			},
+			expectedErr: true,
 		},
 		{
-			QueryType: v3.QueryTypePromQL,
+			compositeQuery: &v3.CompositeQuery{
-			PromQueries: map[string]*v3.PromQuery{
+				QueryType: v3.QueryTypeClickHouseSQL,
-				"query3": {
+				PanelType: v3.PanelTypeGraph,
-					Disabled: true,
+			},
+			expectedErr: true,
+		},
+		{
+			compositeQuery: &v3.CompositeQuery{
+				QueryType: v3.QueryTypeClickHouseSQL,
+				PanelType: v3.PanelTypeGraph,
+				ClickHouseQueries: map[string]*v3.ClickHouseQuery{
+					"query4": {
+						Disabled: false,
+						Query:    "query4",
+					},
 				},
 			},
+			expectedErr: false,
 		},
 		{
-			QueryType: v3.QueryTypeClickHouseSQL,
+			compositeQuery: &v3.CompositeQuery{
-		},
+				QueryType: v3.QueryTypeClickHouseSQL,
-		{
+				PanelType: v3.PanelTypeGraph,
-			QueryType: v3.QueryTypeClickHouseSQL,
+				ClickHouseQueries: map[string]*v3.ClickHouseQuery{
-			ClickHouseQueries: map[string]*v3.ClickHouseQuery{
+					"query4": {
-				"query4": {
+						Disabled: true,
-					Disabled: false,
+					},
-				},
-			},
-		},
-		{
-			QueryType: v3.QueryTypeClickHouseSQL,
-			ClickHouseQueries: map[string]*v3.ClickHouseQuery{
-				"query4": {
-					Disabled: true,
 				},
 			},
+			expectedErr: true,
 		},
 	}
 
-	expectedResult := []bool{true, false, false, false, false, false, true, false, false, true}
+	for idx, testCase := range testCases {
-
+		err := testCase.compositeQuery.Validate()
-	for index, compositeQuery := range testCases {
+		if err != nil {
-		expected := expectedResult[index]
+			if !testCase.expectedErr {
-		actual := isAllQueriesDisabled(compositeQuery)
+				t.Errorf("Expected nil for test case %d, but got %v", idx, err)
-		if actual != expected {
+			}
-			t.Errorf("Expected %v, but got %v", expected, actual)
 		}
 	}
 }

pkg/query-service/rules/base_rule.go

@@ -116,8 +116,11 @@ func WithSQLStore(sqlstore sqlstore.SQLStore) RuleOption {
 }
 
 func NewBaseRule(id string, p *PostableRule, reader interfaces.Reader, opts ...RuleOption) (*BaseRule, error) {
-	if p.RuleCondition == nil || !p.RuleCondition.IsValid() {
+	if p.RuleCondition == nil {
-		return nil, fmt.Errorf("invalid rule condition")
+		return nil, ErrRuleConditionRequired
+	}
+	if err := p.RuleCondition.Validate(); err != nil {
+		return nil, err
 	}
 
 	baseRule := &BaseRule{