From 1b8db233a77c4ac8541151908b43fb0bde7c181e Mon Sep 17 00:00:00 2001 From: Yuri Tseretyan Date: Mon, 10 Feb 2025 09:20:35 -0500 Subject: [PATCH] Alerting: Rule Version API to Ignore versions without diff (#100093) --- pkg/services/ngalert/models/testing.go | 6 ++ pkg/services/ngalert/store/alert_rule.go | 18 ++++- pkg/services/ngalert/store/alert_rule_test.go | 68 +++++++++++++++++++ pkg/services/ngalert/store/models.go | 23 +++++++ 4 files changed, 114 insertions(+), 1 deletion(-) diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index 515b8eed07c..8feca77ec27 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -563,6 +563,12 @@ func (a *AlertRuleMutators) WithKey(key AlertRuleKey) AlertRuleMutator { } } +func (a *AlertRuleMutators) WithVersion(version int64) AlertRuleMutator { + return func(r *AlertRule) { + r.Version = version + } +} + func (g *AlertRuleGenerator) GenerateLabels(min, max int, prefix string) data.Labels { count := max if min > max { diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index fbc9e061385..17f3f294214 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -121,11 +121,12 @@ func (st DBstore) GetAlertRuleByUID(ctx context.Context, query *ngmodels.GetAler func (st DBstore) GetAlertRuleVersions(ctx context.Context, key ngmodels.AlertRuleKey) ([]*ngmodels.AlertRule, error) { alertRules := make([]*ngmodels.AlertRule, 0) err := st.SQLStore.WithDbSession(ctx, func(sess *db.Session) error { - rows, err := sess.Table(new(alertRuleVersion)).Where("rule_org_id = ? AND rule_uid = ?", key.OrgID, key.UID).Desc("id").Rows(new(alertRuleVersion)) + rows, err := sess.Table(new(alertRuleVersion)).Where("rule_org_id = ? AND rule_uid = ?", key.OrgID, key.UID).Asc("id").Rows(new(alertRuleVersion)) if err != nil { return err } // Deserialize each rule separately in case any of them contain invalid JSON. + var previousVersion *alertRuleVersion for rows.Next() { rule := new(alertRuleVersion) err = rows.Scan(rule) @@ -133,11 +134,17 @@ func (st DBstore) GetAlertRuleVersions(ctx context.Context, key ngmodels.AlertRu st.Logger.Error("Invalid rule version found in DB store, ignoring it", "func", "GetAlertRuleVersions", "error", err) continue } + // skip version that has no diff with previous version + // this is pretty basic comparison, it may have false negatives + if previousVersion != nil && previousVersion.EqualSpec(*rule) { + continue + } converted, err := alertRuleToModelsAlertRule(alertRuleVersionToAlertRule(*rule), st.Logger) if err != nil { st.Logger.Error("Invalid rule found in DB store, cannot convert, ignoring it", "func", "GetAlertRuleVersions", "error", err, "version_id", rule.ID) continue } + previousVersion = rule alertRules = append(alertRules, &converted) } return nil @@ -145,6 +152,15 @@ func (st DBstore) GetAlertRuleVersions(ctx context.Context, key ngmodels.AlertRu if err != nil { return nil, err } + slices.SortFunc(alertRules, func(a, b *ngmodels.AlertRule) int { + if a.ID > b.ID { + return -1 + } + if a.ID < b.ID { + return 1 + } + return 0 + }) return alertRules, nil } diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index 10ccf9b9faf..0eba646ab46 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -1543,6 +1543,74 @@ func TestIncreaseVersionForAllRulesInNamespaces(t *testing.T) { }) } +func TestGetRuleVersions(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + cfg := setting.NewCfg() + cfg.UnifiedAlerting = setting.UnifiedAlertingSettings{BaseInterval: time.Duration(rand.Int63n(100)+1) * time.Second} + sqlStore := db.InitTestDB(t) + folderService := setupFolderService(t, sqlStore, cfg, featuremgmt.WithFeatures()) + b := &fakeBus{} + store := createTestStore(sqlStore, folderService, &logtest.Fake{}, cfg.UnifiedAlerting, b) + orgID := int64(1) + gen := models.RuleGen + gen = gen.With(gen.WithIntervalMatching(store.Cfg.BaseInterval), gen.WithOrgID(orgID), gen.WithVersion(1)) + + inserted, err := store.InsertAlertRules(context.Background(), &models.AlertingUserUID, []models.AlertRule{gen.Generate()}) + require.NoError(t, err) + ruleV1, err := store.GetAlertRuleByUID(context.Background(), &models.GetAlertRuleByUIDQuery{UID: inserted[0].UID}) + require.NoError(t, err) + ruleV2 := models.CopyRule(ruleV1, gen.WithTitle(util.GenerateShortUID()), gen.WithGroupIndex(rand.Int())) + + err = store.UpdateAlertRules(context.Background(), &models.AlertingUserUID, []models.UpdateRule{ + { + Existing: ruleV1, + New: *ruleV2, + }, + }) + require.NoError(t, err) + + t.Run("should return rule versions sorted in decreasing order", func(t *testing.T) { + versions, err := store.GetAlertRuleVersions(context.Background(), ruleV2.GetKey()) + require.NoError(t, err) + assert.Len(t, versions, 2) + assert.IsDecreasing(t, versions[0].ID, versions[1].ID) + diff := versions[1].Diff(versions[0], AlertRuleFieldsToIgnoreInDiff[:]...) + assert.ElementsMatch(t, []string{"Title", "RuleGroupIndex"}, diff.Paths()) + }) + + t.Run("should not remove versions without diff", func(t *testing.T) { + for i := 0; i < rand.Intn(2)+1; i++ { + r, err := store.GetAlertRuleByUID(context.Background(), &models.GetAlertRuleByUIDQuery{UID: ruleV2.UID}) + require.NoError(t, err) + rn := models.CopyRule(r) + err = store.UpdateAlertRules(context.Background(), &models.AlertingUserUID, []models.UpdateRule{ + { + Existing: r, + New: *rn, + }, + }) + require.NoError(t, err) + } + ruleV2, err = store.GetAlertRuleByUID(context.Background(), &models.GetAlertRuleByUIDQuery{UID: ruleV2.UID}) + ruleV3 := models.CopyRule(ruleV2, gen.WithGroupName(util.GenerateShortUID()), gen.WithNamespaceUID(util.GenerateShortUID())) + + err = store.UpdateAlertRules(context.Background(), &models.AlertingUserUID, []models.UpdateRule{ + { + Existing: ruleV2, + New: *ruleV3, + }, + }) + + versions, err := store.GetAlertRuleVersions(context.Background(), ruleV3.GetKey()) + require.NoError(t, err) + assert.Len(t, versions, 3) + diff := versions[0].Diff(versions[1], AlertRuleFieldsToIgnoreInDiff[:]...) + assert.ElementsMatch(t, []string{"RuleGroup", "NamespaceUID"}, diff.Paths()) + }) +} + // createAlertRule creates an alert rule in the database and returns it. // If a generator is not specified, uniqueness of primary key is not guaranteed. func createRule(t *testing.T, store *DBstore, generator *models.AlertRuleGenerator) *models.AlertRule { diff --git a/pkg/services/ngalert/store/models.go b/pkg/services/ngalert/store/models.go index 3e47ab90fa4..ed9456036c2 100644 --- a/pkg/services/ngalert/store/models.go +++ b/pkg/services/ngalert/store/models.go @@ -65,6 +65,29 @@ type alertRuleVersion struct { Metadata string `xorm:"metadata"` } +// EqualSpec compares two alertRuleVersion objects for equality based on their specifications and returns true if they match. +// The comparison is very basic and can produce false-negative. Fields excluded: ID, ParentVersion, RestoredFrom, Version, Created and CreatedBy +func (a alertRuleVersion) EqualSpec(b alertRuleVersion) bool { + return a.RuleOrgID == b.RuleOrgID && + a.RuleUID == b.RuleUID && + a.RuleNamespaceUID == b.RuleNamespaceUID && + a.RuleGroup == b.RuleGroup && + a.RuleGroupIndex == b.RuleGroupIndex && + a.Title == b.Title && + a.Condition == b.Condition && + a.Data == b.Data && + a.IntervalSeconds == b.IntervalSeconds && + a.Record == b.Record && + a.NoDataState == b.NoDataState && + a.ExecErrState == b.ExecErrState && + a.For == b.For && + a.Annotations == b.Annotations && + a.Labels == b.Labels && + a.IsPaused == b.IsPaused && + a.NotificationSettings == b.NotificationSettings && + a.Metadata == b.Metadata +} + func (a alertRuleVersion) TableName() string { return "alert_rule_version" }