From b8a284fb81be7af56ee2e7ccaeefde81253ec4a0 Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Tue, 14 May 2024 14:50:06 -0500 Subject: [PATCH] Alerting: Fix xorm serialization of Record field struct, add tests for storing and reading (#87857) Fix sub struct ser and deser, add tests --- pkg/services/ngalert/models/alert_rule.go | 12 +-- pkg/services/ngalert/store/alert_rule_test.go | 76 ++++++++++++++----- 2 files changed, 63 insertions(+), 25 deletions(-) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 50e373afc61..19ff0cc32cf 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -243,7 +243,7 @@ type AlertRule struct { PanelID *int64 `xorm:"panel_id"` RuleGroup string RuleGroupIndex int `xorm:"rule_group_idx"` - Record *Record `xorm:"text null 'record'"` + Record *Record `xorm:"json"` NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -518,10 +518,6 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting } } - if alertRule.Record != nil { - return fmt.Errorf("%w: storing recording rules is not yet allowed", ErrAlertRuleFailedValidation) - } - if len(alertRule.NotificationSettings) > 0 { if len(alertRule.NotificationSettings) != 1 { return fmt.Errorf("%w: only one notification settings entry is allowed", ErrAlertRuleFailedValidation) @@ -552,6 +548,10 @@ func (alertRule *AlertRule) GetFolderKey() FolderKey { } } +func (alertRule *AlertRule) IsRecordingRule() bool { + return alertRule.Record != nil +} + // AlertRuleVersion is the model for alert rule versions in unified alerting. type AlertRuleVersion struct { ID int64 `xorm:"pk autoincr 'id'"` @@ -569,7 +569,7 @@ type AlertRuleVersion struct { Condition string Data []AlertQuery IntervalSeconds int64 - Record *Record `xorm:"text null 'record'"` + Record *Record `xorm:"json"` NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index bb567809d37..7016aa9bedd 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -610,18 +610,32 @@ func TestIntegrationInsertAlertRules(t *testing.T) { Cfg: cfg.UnifiedAlerting, } - gen := models.RuleGen - rules := gen.With( - gen.WithOrgID(orgID), - gen.WithIntervalMatching(store.Cfg.BaseInterval), - ).GenerateManyRef(5) + gen := models.RuleGen.With( + models.RuleGen.WithOrgID(orgID), + models.RuleGen.WithIntervalMatching(store.Cfg.BaseInterval), + ) - deref := make([]models.AlertRule, 0, len(rules)) - for _, rule := range rules { - deref = append(deref, *rule) + generateRecordingRules := func(n int) []models.AlertRule { + rrs := gen.GenerateMany(n) + for i := range rrs { + rrs[i].Condition = "" + // TODO: These fields do not apply to recording rules - for now, we just use the default values of them. This is validated at the storage level. + // TODO: Consider making it so recording rules allow for empty string here, or use some other sentinel value in the future. + rrs[i].NoDataState = models.NoData + rrs[i].ExecErrState = models.ErrorErrState + rrs[i].For = 0 + rrs[i].NotificationSettings = nil + rrs[i].Record = &models.Record{ + Metric: "my_metric", + From: "A", + } + } + return rrs } - ids, err := store.InsertAlertRules(context.Background(), deref) + rules := append(gen.GenerateMany(5), generateRecordingRules(5)...) + + ids, err := store.InsertAlertRules(context.Background(), rules) require.NoError(t, err) require.Len(t, ids, len(rules)) @@ -645,29 +659,53 @@ func TestIntegrationInsertAlertRules(t *testing.T) { t.Run("inserted alerting rules should have nil recording rule fields on model", func(t *testing.T) { for _, rule := range dbRules { - require.Nil(t, rule.Record) + if !rule.IsRecordingRule() { + require.Nil(t, rule.Record) + } + } + }) + + t.Run("inserted recording rules map identical fields when listed", func(t *testing.T) { + for _, rule := range dbRules { + if rule.IsRecordingRule() { + require.NotNil(t, rule.Record) + require.Equal(t, "my_metric", rule.Record.Metric) + require.Equal(t, "A", rule.Record.From) + } + } + }) + + t.Run("inserted recording rules have empty or default alert-specific settings", func(t *testing.T) { + for _, rule := range dbRules { + if rule.IsRecordingRule() { + require.Empty(t, rule.Condition) + require.Equal(t, models.NoData, rule.NoDataState) + require.Equal(t, models.ErrorErrState, rule.ExecErrState) + require.Zero(t, rule.For) + require.Nil(t, rule.NotificationSettings) + } } }) t.Run("fail to insert rules with same ID", func(t *testing.T) { - _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{deref[0]}) + _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{rules[0]}) require.ErrorIs(t, err, models.ErrAlertRuleConflictBase) }) t.Run("fail insert rules with the same title in a folder", func(t *testing.T) { - cp := models.CopyRule(&deref[0]) + cp := models.CopyRule(&rules[0]) cp.UID = cp.UID + "-new" _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{*cp}) require.ErrorIs(t, err, models.ErrAlertRuleConflictBase) require.ErrorIs(t, err, models.ErrAlertRuleUniqueConstraintViolation) - require.NotEqual(t, deref[0].UID, "") - require.NotEqual(t, deref[0].Title, "") - require.NotEqual(t, deref[0].NamespaceUID, "") - require.ErrorContains(t, err, deref[0].UID) - require.ErrorContains(t, err, deref[0].Title) - require.ErrorContains(t, err, deref[0].NamespaceUID) + require.NotEqual(t, rules[0].UID, "") + require.NotEqual(t, rules[0].Title, "") + require.NotEqual(t, rules[0].NamespaceUID, "") + require.ErrorContains(t, err, rules[0].UID) + require.ErrorContains(t, err, rules[0].Title) + require.ErrorContains(t, err, rules[0].NamespaceUID) }) t.Run("should not let insert rules with the same UID", func(t *testing.T) { - cp := models.CopyRule(&deref[0]) + cp := models.CopyRule(&rules[0]) cp.Title = "unique-test-title" _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{*cp}) require.ErrorIs(t, err, models.ErrAlertRuleConflictBase)