From 072380e113943858840defaafe44799883884a2e Mon Sep 17 00:00:00 2001 From: Mateusz Hawrus <48822818+nieomylnieja@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:16:17 +0100 Subject: [PATCH] fix: Adjust validation for missing metric spec [PC-15176] (#613) ## Release Notes When an SLO is defined with missing metric spec for either count or raw metrics, the SDK validation will now return an error. --- manifest/v1alpha/slo/metrics_validation.go | 10 +- manifest/v1alpha/slo/validation.go | 3 - manifest/v1alpha/slo/validation_test.go | 103 ++++++++++++++------- 3 files changed, 78 insertions(+), 38 deletions(-) diff --git a/manifest/v1alpha/slo/metrics_validation.go b/manifest/v1alpha/slo/metrics_validation.go index e704eb36..601d921e 100644 --- a/manifest/v1alpha/slo/metrics_validation.go +++ b/manifest/v1alpha/slo/metrics_validation.go @@ -149,6 +149,13 @@ var singleQueryMetricSpecValidation = govy.New[MetricSpec]( ) var metricSpecValidation = govy.New[MetricSpec]( + govy.For(govy.GetSelf[MetricSpec]()). + Rules(govy.NewRule(func(m MetricSpec) error { + if m == (MetricSpec{}) { + return errors.New("exactly one valid metric spec has to be provided (e.g. 'prometheus')") + } + return nil + })), govy.ForPointer(func(m MetricSpec) *AppDynamicsMetric { return m.AppDynamics }). WithName("appDynamics"). Include(appDynamicsValidation), @@ -403,9 +410,6 @@ func validateExactlyOneMetricSpecType(metrics ...*MetricSpec) error { } } } - if onlyType == 0 { - return errors.New("must have exactly one metric spec type, none were provided") - } return nil } diff --git a/manifest/v1alpha/slo/validation.go b/manifest/v1alpha/slo/validation.go index e9c167a2..774023c9 100644 --- a/manifest/v1alpha/slo/validation.go +++ b/manifest/v1alpha/slo/validation.go @@ -165,9 +165,6 @@ var specValidation = govy.New[Spec]( Rules(rules.SliceLength[[]TimeWindow](1, 1)). IncludeForEach(timeWindowsValidation). RulesForEach(timeWindowValidationRule()), - govy.ForSlice(func(s Spec) []Objective { return s.Objectives }). - WithName("objectives"). - RulesForEach(), govy.ForSlice(func(s Spec) []Objective { return s.Objectives }). WithName("objectives"). Cascade(govy.CascadeModeStop). diff --git a/manifest/v1alpha/slo/validation_test.go b/manifest/v1alpha/slo/validation_test.go index c62ddedb..b7dc2635 100644 --- a/manifest/v1alpha/slo/validation_test.go +++ b/manifest/v1alpha/slo/validation_test.go @@ -12,12 +12,12 @@ import ( "github.com/stretchr/testify/assert" - validationV1Alpha "github.com/nobl9/nobl9-go/internal/manifest/v1alpha" - internal "github.com/nobl9/nobl9-go/internal/manifest/v1alpha/slo" - "github.com/nobl9/govy/pkg/govy" "github.com/nobl9/govy/pkg/rules" + validationV1Alpha "github.com/nobl9/nobl9-go/internal/manifest/v1alpha" + internal "github.com/nobl9/nobl9-go/internal/manifest/v1alpha/slo" + "github.com/nobl9/nobl9-go/internal/manifest/v1alphatest" "github.com/nobl9/nobl9-go/internal/testutils" "github.com/nobl9/nobl9-go/manifest" @@ -1074,7 +1074,7 @@ func TestValidate_Spec_Objectives_RawMetric(t *testing.T) { } func TestValidate_Spec(t *testing.T) { - t.Run("exactly one metric type - both provided", func(t *testing.T) { + t.Run("exactly one metric type - both provided within the same objective", func(t *testing.T) { slo := validSLO() slo.Spec.Objectives[0].RawMetric = &RawMetricSpec{ MetricQuery: validMetricSpec(v1alpha.Prometheus), @@ -1087,18 +1087,49 @@ func TestValidate_Spec(t *testing.T) { err := validate(slo) testutils.AssertContainsErrors(t, slo, err, 1, testutils.ExpectedError{ - Prop: "spec", - Code: rules.ErrorCodeMutuallyExclusive, + Prop: "spec", + Message: "[countMetrics, rawMetrics] properties are mutually exclusive, provide only one of them", + Code: rules.ErrorCodeMutuallyExclusive, }) }) - t.Run("exactly one metric type - both missing", func(t *testing.T) { + t.Run("exactly one metric type - both missing within the same objective", func(t *testing.T) { slo := validSLO() slo.Spec.Objectives[0].RawMetric = nil slo.Spec.Objectives[0].CountMetrics = nil err := validate(slo) testutils.AssertContainsErrors(t, slo, err, 1, testutils.ExpectedError{ - Prop: "spec", - Code: rules.ErrorCodeMutuallyExclusive, + Prop: "spec", + Message: "one of [composite, countMetrics, rawMetrics] properties must be set, none was provided", + Code: rules.ErrorCodeMutuallyExclusive, + }) + }) + t.Run("exactly one metric type - both provided in different objectives", func(t *testing.T) { + slo := validSLO() + slo.Spec.Objectives[0].RawMetric = &RawMetricSpec{ + MetricQuery: validMetricSpec(v1alpha.Prometheus), + } + slo.Spec.Objectives = append(slo.Spec.Objectives, validCountMetricsObjective()) + err := validate(slo) + testutils.AssertContainsErrors(t, slo, err, 2, + testutils.ExpectedError{ + Prop: "spec", + Message: "[countMetrics, rawMetrics] properties are mutually exclusive, provide only one of them", + Code: rules.ErrorCodeMutuallyExclusive, + }) + }) + t.Run("exactly one metric type - both missing from different objectives", func(t *testing.T) { + slo := validSLO() + slo.Spec.Objectives[0].RawMetric = nil + slo.Spec.Objectives[0].CountMetrics = nil + slo.Spec.Objectives[0].Value = ptr(1.0) + slo.Spec.Objectives = append(slo.Spec.Objectives, validCountMetricsObjective()) + slo.Spec.Objectives[1].Value = ptr(2.0) + slo.Spec.Objectives[1].CountMetrics = nil + err := validate(slo) + testutils.AssertContainsErrors(t, slo, err, 1, testutils.ExpectedError{ + Prop: "spec", + Message: "one of [composite, countMetrics, rawMetrics] properties must be set, none was provided", + Code: rules.ErrorCodeMutuallyExclusive, }) }) t.Run("required time slice target for budgeting method", func(t *testing.T) { @@ -1137,8 +1168,8 @@ func TestValidate_Spec_RawMetrics(t *testing.T) { slo.Spec.Objectives[0].RawMetric.MetricQuery.Prometheus = nil err := validate(slo) testutils.AssertContainsErrors(t, slo, err, 1, testutils.ExpectedError{ - Prop: "spec", - Message: "must have exactly one metric spec type, none were provided", + Prop: "spec.objectives[0].rawMetric.query", + Message: "exactly one valid metric spec has to be provided (e.g. 'prometheus')", }) }) t.Run("exactly one metric spec type", func(t *testing.T) { @@ -1187,13 +1218,19 @@ func TestValidate_Spec_RawMetrics(t *testing.T) { func TestValidate_Spec_CountMetrics(t *testing.T) { t.Run("no metric spec provided", func(t *testing.T) { slo := validCountMetricSLO(v1alpha.Prometheus) - slo.Spec.Objectives[0].CountMetrics.TotalMetric.Prometheus = nil slo.Spec.Objectives[0].CountMetrics.GoodMetric.Prometheus = nil + slo.Spec.Objectives[0].CountMetrics.TotalMetric.Prometheus = nil err := validate(slo) - testutils.AssertContainsErrors(t, slo, err, 1, testutils.ExpectedError{ - Prop: "spec", - Message: "must have exactly one metric spec type, none were provided", - }) + testutils.AssertContainsErrors(t, slo, err, 2, + testutils.ExpectedError{ + Prop: "spec.objectives[0].countMetrics.good", + Message: "exactly one valid metric spec has to be provided (e.g. 'prometheus')", + }, + testutils.ExpectedError{ + Prop: "spec.objectives[0].countMetrics.total", + Message: "exactly one valid metric spec has to be provided (e.g. 'prometheus')", + }, + ) }) t.Run("bad over total enabled", func(t *testing.T) { for _, typ := range internal.BadOverTotalEnabledSources { @@ -1486,22 +1523,7 @@ func validSLO() SLO { Kind: manifest.KindAgent, }, }, - Objectives: []Objective{ - { - ObjectiveBase: ObjectiveBase{ - DisplayName: "Good", - Value: ptr(120.), - Name: "good", - }, - BudgetTarget: ptr(0.9), - CountMetrics: &CountMetricsSpec{ - Incremental: ptr(false), - TotalMetric: validMetricSpec(v1alpha.Prometheus), - GoodMetric: validMetricSpec(v1alpha.Prometheus), - }, - Operator: ptr(v1alpha.LessThan.String()), - }, - }, + Objectives: []Objective{validCountMetricsObjective()}, TimeWindows: []TimeWindow{ { Unit: "Day", @@ -1513,6 +1535,23 @@ func validSLO() SLO { ) } +func validCountMetricsObjective() Objective { + return Objective{ + ObjectiveBase: ObjectiveBase{ + DisplayName: "Good", + Value: ptr(120.), + Name: "good", + }, + BudgetTarget: ptr(0.9), + CountMetrics: &CountMetricsSpec{ + Incremental: ptr(false), + TotalMetric: validMetricSpec(v1alpha.Prometheus), + GoodMetric: validMetricSpec(v1alpha.Prometheus), + }, + Operator: ptr(v1alpha.LessThan.String()), + } +} + func validCompositeObjective() Objective { return Objective{ ObjectiveBase: ObjectiveBase{