From 4cac3158c76bb3f36ca01dadfa1ac3e7df7615d3 Mon Sep 17 00:00:00 2001 From: Yuri Tseretyan Date: Tue, 11 Feb 2025 09:46:02 -0500 Subject: [PATCH] Alerting: Fix alert rule copy to include metadata (#100212) * copy metadata * add tests for copy and generator * extract copy rule to a production method and update usages * fix tests --- pkg/services/ngalert/models/alert_rule.go | 75 ++++++++++++++++ .../ngalert/models/alert_rule_test.go | 66 +++++++++++++- pkg/services/ngalert/models/testing.go | 88 ++++--------------- .../ngalert/schedule/schedule_unit_test.go | 2 + pkg/services/ngalert/store/alert_rule.go | 4 +- pkg/services/ngalert/store/deltas.go | 2 +- pkg/services/ngalert/store/deltas_test.go | 4 +- 7 files changed, 166 insertions(+), 75 deletions(-) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 26f7d84eee1..e11c5eb1add 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -680,6 +680,81 @@ func (alertRule *AlertRule) Type() RuleType { return RuleTypeAlerting } +// Copy creates and returns a deep copy of the AlertRule instance, duplicating all fields and nested data structures. +func (alertRule *AlertRule) Copy() *AlertRule { + if alertRule == nil { + return nil + } + result := AlertRule{ + ID: alertRule.ID, + OrgID: alertRule.OrgID, + Title: alertRule.Title, + Condition: alertRule.Condition, + Updated: alertRule.Updated, + UpdatedBy: alertRule.UpdatedBy, + IntervalSeconds: alertRule.IntervalSeconds, + Version: alertRule.Version, + UID: alertRule.UID, + NamespaceUID: alertRule.NamespaceUID, + RuleGroup: alertRule.RuleGroup, + RuleGroupIndex: alertRule.RuleGroupIndex, + NoDataState: alertRule.NoDataState, + ExecErrState: alertRule.ExecErrState, + For: alertRule.For, + Record: alertRule.Record, + IsPaused: alertRule.IsPaused, + Metadata: alertRule.Metadata, + } + + if alertRule.DashboardUID != nil { + dash := *alertRule.DashboardUID + result.DashboardUID = &dash + } + if alertRule.PanelID != nil { + p := *alertRule.PanelID + result.PanelID = &p + } + + for _, d := range alertRule.Data { + q := AlertQuery{ + RefID: d.RefID, + QueryType: d.QueryType, + RelativeTimeRange: d.RelativeTimeRange, + DatasourceUID: d.DatasourceUID, + } + q.Model = make([]byte, 0, cap(d.Model)) + q.Model = append(q.Model, d.Model...) + result.Data = append(result.Data, q) + } + + if alertRule.Annotations != nil { + result.Annotations = make(map[string]string, len(alertRule.Annotations)) + for s, s2 := range alertRule.Annotations { + result.Annotations[s] = s2 + } + } + + if alertRule.Labels != nil { + result.Labels = make(map[string]string, len(alertRule.Labels)) + for s, s2 := range alertRule.Labels { + result.Labels[s] = s2 + } + } + + if alertRule.Record != nil { + result.Record = &Record{ + From: alertRule.Record.From, + Metric: alertRule.Record.Metric, + } + } + + for _, s := range alertRule.NotificationSettings { + result.NotificationSettings = append(result.NotificationSettings, CopyNotificationSettings(s)) + } + + return &result +} + func ClearRecordingRuleIgnoredFields(rule *AlertRule) { rule.NoDataState = "" rule.ExecErrState = "" diff --git a/pkg/services/ngalert/models/alert_rule_test.go b/pkg/services/ngalert/models/alert_rule_test.go index 7bb1b44a39b..6594732c04b 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -15,6 +15,7 @@ import ( "github.com/prometheus/common/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/exp/maps" "gopkg.in/yaml.v3" "github.com/grafana/grafana/pkg/util" @@ -407,7 +408,7 @@ func TestDiff(t *testing.T) { rule1 := RuleGen.GenerateRef() rule2 := RuleGen.GenerateRef() - diffs := rule1.Diff(rule2, "Data", "Annotations", "Labels", "NotificationSettings") // these fields will be tested separately + diffs := rule1.Diff(rule2, "Data", "Annotations", "Labels", "NotificationSettings", "Metadata") // these fields will be tested separately difCnt := 0 if rule1.ID != rule2.ID { @@ -839,6 +840,24 @@ func TestDiff(t *testing.T) { }) } }) + + t.Run("should detect changes in Metadata", func(t *testing.T) { + rule1 := RuleGen.With(RuleGen.WithMetadata(AlertRuleMetadata{EditorSettings: EditorSettings{ + SimplifiedQueryAndExpressionsSection: false, + SimplifiedNotificationsSection: false, + }})).GenerateRef() + + rule2 := CopyRule(rule1, RuleGen.WithMetadata(AlertRuleMetadata{EditorSettings: EditorSettings{ + SimplifiedQueryAndExpressionsSection: true, + SimplifiedNotificationsSection: true, + }})) + + diff := rule1.Diff(rule2) + assert.ElementsMatch(t, []string{ + "Metadata.EditorSettings.SimplifiedQueryAndExpressionsSection", + "Metadata.EditorSettings.SimplifiedNotificationsSection", + }, diff.Paths()) + }) } func TestSortByGroupIndex(t *testing.T) { @@ -919,3 +938,48 @@ func TestAlertRuleGetKeyWithGroup(t *testing.T) { require.Equal(t, expected, rule.GetKeyWithGroup()) }) } + +func TestAlertRuleCopy(t *testing.T) { + for i := 0; i < 100; i++ { + rule := RuleGen.GenerateRef() + copied := rule.Copy() + require.Empty(t, rule.Diff(copied)) + } +} + +// This test makes sure the default generator +func TestGeneratorFillsAllFields(t *testing.T) { + ignoredFields := map[string]struct{}{ + "ID": {}, + "IsPaused": {}, + "Record": {}, + } + + tpe := reflect.TypeOf(AlertRule{}) + fields := make(map[string]struct{}, tpe.NumField()) + for i := 0; i < tpe.NumField(); i++ { + if _, ok := ignoredFields[tpe.Field(i).Name]; ok { + continue + } + fields[tpe.Field(i).Name] = struct{}{} + } + + for i := 0; i < 1000; i++ { + rule := RuleGen.Generate() + v := reflect.ValueOf(rule) + + for j := 0; j < tpe.NumField(); j++ { + field := tpe.Field(j) + value := v.Field(j) + if !value.IsValid() || value.Kind() == reflect.Ptr && value.IsNil() || value.IsZero() { + continue + } + delete(fields, field.Name) + if len(fields) == 0 { + return + } + } + } + + require.FailNow(t, "AlertRule generator does not populate fields", "skipped fields: %v", maps.Keys(fields)) +} diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index 8feca77ec27..04869e53750 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -123,6 +123,7 @@ func (g *AlertRuleGenerator) Generate() AlertRule { Annotations: annotations, Labels: labels, NotificationSettings: ns, + Metadata: GenerateMetadata(), } for _, mutator := range g.mutators { @@ -569,6 +570,12 @@ func (a *AlertRuleMutators) WithVersion(version int64) AlertRuleMutator { } } +func (a *AlertRuleMutators) WithMetadata(meta AlertRuleMetadata) AlertRuleMutator { + return func(r *AlertRule) { + r.Metadata = meta + } +} + func (g *AlertRuleGenerator) GenerateLabels(min, max int, prefix string) data.Labels { count := max if min > max { @@ -643,79 +650,13 @@ func GenerateGroupKey(orgID int64) AlertRuleGroupKey { // CopyRule creates a deep copy of AlertRule func CopyRule(r *AlertRule, mutators ...AlertRuleMutator) *AlertRule { - result := AlertRule{ - ID: r.ID, - OrgID: r.OrgID, - Title: r.Title, - Condition: r.Condition, - Updated: r.Updated, - UpdatedBy: r.UpdatedBy, - IntervalSeconds: r.IntervalSeconds, - Version: r.Version, - UID: r.UID, - NamespaceUID: r.NamespaceUID, - RuleGroup: r.RuleGroup, - RuleGroupIndex: r.RuleGroupIndex, - NoDataState: r.NoDataState, - ExecErrState: r.ExecErrState, - For: r.For, - Record: r.Record, - IsPaused: r.IsPaused, - } - - if r.DashboardUID != nil { - dash := *r.DashboardUID - result.DashboardUID = &dash - } - if r.PanelID != nil { - p := *r.PanelID - result.PanelID = &p - } - - for _, d := range r.Data { - q := AlertQuery{ - RefID: d.RefID, - QueryType: d.QueryType, - RelativeTimeRange: d.RelativeTimeRange, - DatasourceUID: d.DatasourceUID, - } - q.Model = make([]byte, 0, cap(d.Model)) - q.Model = append(q.Model, d.Model...) - result.Data = append(result.Data, q) - } - - if r.Annotations != nil { - result.Annotations = make(map[string]string, len(r.Annotations)) - for s, s2 := range r.Annotations { - result.Annotations[s] = s2 - } - } - - if r.Labels != nil { - result.Labels = make(map[string]string, len(r.Labels)) - for s, s2 := range r.Labels { - result.Labels[s] = s2 - } - } - - if r.Record != nil { - result.Record = &Record{ - From: r.Record.From, - Metric: r.Record.Metric, - } - } - - for _, s := range r.NotificationSettings { - result.NotificationSettings = append(result.NotificationSettings, CopyNotificationSettings(s)) - } - + result := r.Copy() if len(mutators) > 0 { for _, mutator := range mutators { - mutator(&result) + mutator(result) } } - - return &result + return result } func CreateClassicConditionExpression(refID string, inputRefID string, reducer string, operation string, threshold int) AlertQuery { @@ -862,6 +803,15 @@ func CreateHysteresisExpression(t *testing.T, refID string, inputRefID string, t return q } +func GenerateMetadata() AlertRuleMetadata { + return AlertRuleMetadata{ + EditorSettings: EditorSettings{ + SimplifiedQueryAndExpressionsSection: rand.Int()%2 == 0, + SimplifiedNotificationsSection: rand.Int()%2 == 0, + }, + } +} + type AlertInstanceMutator func(*AlertInstance) // AlertInstanceGen provides a factory function that generates a random AlertInstance. diff --git a/pkg/services/ngalert/schedule/schedule_unit_test.go b/pkg/services/ngalert/schedule/schedule_unit_test.go index 575f19cda27..390c966786f 100644 --- a/pkg/services/ngalert/schedule/schedule_unit_test.go +++ b/pkg/services/ngalert/schedule/schedule_unit_test.go @@ -784,6 +784,7 @@ func TestSchedule_updateRulesMetrics(t *testing.T) { alertRuleWithAdvancedSettings := models.RuleGen.With( models.RuleGen.WithOrgID(firstOrgID), + models.RuleGen.WithEditorSettingsSimplifiedNotificationsSection(false), models.RuleGen.WithEditorSettingsSimplifiedQueryAndExpressionsSection(false), ).GenerateRef() @@ -818,6 +819,7 @@ func TestSchedule_updateRulesMetrics(t *testing.T) { alertRule2 := models.RuleGen.With( models.RuleGen.WithOrgID(secondOrgID), + models.RuleGen.WithEditorSettingsSimplifiedNotificationsSection(false), models.RuleGen.WithEditorSettingsSimplifiedQueryAndExpressionsSection(true), ).GenerateRef() diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index 17f3f294214..4a50990c5c1 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -984,7 +984,7 @@ func (st DBstore) RenameReceiverInNotificationSettings(ctx context.Context, orgI continue } - r := ngmodels.CopyRule(rule) + r := rule.Copy() for idx := range r.NotificationSettings { if r.NotificationSettings[idx].Receiver == oldReceiver { r.NotificationSettings[idx].Receiver = newReceiver @@ -1059,7 +1059,7 @@ func (st DBstore) RenameTimeIntervalInNotificationSettings( continue } - r := ngmodels.CopyRule(rule) + r := rule.Copy() for idx := range r.NotificationSettings { for mtIdx := range r.NotificationSettings[idx].MuteTimeIntervals { if r.NotificationSettings[idx].MuteTimeIntervals[mtIdx] == oldTimeInterval { diff --git a/pkg/services/ngalert/store/deltas.go b/pkg/services/ngalert/store/deltas.go index e08bc3c5cfc..a7b00d27adf 100644 --- a/pkg/services/ngalert/store/deltas.go +++ b/pkg/services/ngalert/store/deltas.go @@ -175,7 +175,7 @@ func UpdateCalculatedRuleFields(ch *GroupDelta) *GroupDelta { } if groupKey != ch.GroupKey { if rule.RuleGroupIndex != idx { - upd.New = models.CopyRule(rule) + upd.New = rule.Copy() upd.New.RuleGroupIndex = idx upd.Diff = rule.Diff(upd.New, AlertRuleFieldsToIgnoreInDiff[:]...) } diff --git a/pkg/services/ngalert/store/deltas_test.go b/pkg/services/ngalert/store/deltas_test.go index 904c0da5164..fde5a8e252f 100644 --- a/pkg/services/ngalert/store/deltas_test.go +++ b/pkg/services/ngalert/store/deltas_test.go @@ -81,7 +81,7 @@ func TestCalculateChanges(t *testing.T) { submittedMap := groupByUID(t, rules) submitted := make([]*models.AlertRuleWithOptionals, 0, len(rules)) for _, rule := range rules { - submitted = append(submitted, &models.AlertRuleWithOptionals{AlertRule: *rule}) + submitted = append(submitted, &models.AlertRuleWithOptionals{AlertRule: *rule, HasMetadata: true}) } fakeStore := fakes.NewRuleStore(t) @@ -216,7 +216,7 @@ func TestCalculateChanges(t *testing.T) { submittedMap := groupByUID(t, rules) submitted := make([]*models.AlertRuleWithOptionals, 0, len(rules)) for _, rule := range rules { - submitted = append(submitted, &models.AlertRuleWithOptionals{AlertRule: *rule}) + submitted = append(submitted, &models.AlertRuleWithOptionals{AlertRule: *rule, HasMetadata: true}) } changes, err := CalculateChanges(context.Background(), fakeStore, groupKey, submitted)