From 36ef611cf43d0c12a6fb100b014047f60b30eb52 Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Thu, 9 May 2024 12:12:44 -0500 Subject: [PATCH] Alerting: Add database migration for recording rule fields (#87012) * Create recording rule fields in model * Add migration * Write to database, support in version table * extend fingerprint * Force fields to be empty on validate * Another storage spot, tests for fingerprint * Explicitly set defaults in provisioning API * Tests for main API validation * Add diff tests even though fields are unpopulated for now * Use struct tag approach instead of FromDB/ToDB hooks as it better handles nulls when deserializing * test for deser * Backout RecordTo for now since it's not decided in the doc * back out of migration too * Drop datasourceref for now * address linter complaints * Try a single outer struct with all fields embedded --- .../ngalert/api/api_ruler_validation.go | 3 ++ .../ngalert/api/api_ruler_validation_test.go | 1 + pkg/services/ngalert/api/compat.go | 3 ++ pkg/services/ngalert/models/alert_rule.go | 34 ++++++++++++++++++- .../ngalert/models/alert_rule_test.go | 7 ++++ pkg/services/ngalert/schedule/registry.go | 5 +++ .../ngalert/schedule/registry_test.go | 2 ++ pkg/services/ngalert/store/alert_rule.go | 2 ++ pkg/services/ngalert/store/alert_rule_test.go | 6 ++++ .../sqlstore/migrations/migrations.go | 2 ++ .../migrations/ualert/recording_rules_mig.go | 18 ++++++++++ 11 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go diff --git a/pkg/services/ngalert/api/api_ruler_validation.go b/pkg/services/ngalert/api/api_ruler_validation.go index 91a012b0b1e..2fa3100ac58 100644 --- a/pkg/services/ngalert/api/api_ruler_validation.go +++ b/pkg/services/ngalert/api/api_ruler_validation.go @@ -108,6 +108,9 @@ func validateRuleNode( RuleGroup: groupName, NoDataState: noDataState, ExecErrState: errorState, + // Recording Rule fields will be implemented in the future. + // For now, no rules can be recording rules. So, we force these to be empty. + Record: nil, } if ruleNode.GrafanaManagedAlert.NotificationSettings != nil { diff --git a/pkg/services/ngalert/api/api_ruler_validation_test.go b/pkg/services/ngalert/api/api_ruler_validation_test.go index f2332211129..46d6406bb17 100644 --- a/pkg/services/ngalert/api/api_ruler_validation_test.go +++ b/pkg/services/ngalert/api/api_ruler_validation_test.go @@ -339,6 +339,7 @@ func TestValidateRuleNode_NoUID(t *testing.T) { require.Equal(t, time.Duration(*api.ApiRuleNode.For), alert.For) require.Equal(t, api.ApiRuleNode.Annotations, alert.Annotations) require.Equal(t, api.ApiRuleNode.Labels, alert.Labels) + require.Nil(t, alert.Record) }, }, { diff --git a/pkg/services/ngalert/api/compat.go b/pkg/services/ngalert/api/compat.go index 8f671b11341..a0c2390254c 100644 --- a/pkg/services/ngalert/api/compat.go +++ b/pkg/services/ngalert/api/compat.go @@ -31,6 +31,9 @@ func AlertRuleFromProvisionedAlertRule(a definitions.ProvisionedAlertRule) (mode Labels: a.Labels, IsPaused: a.IsPaused, NotificationSettings: NotificationSettingsFromAlertRuleNotificationSettings(a.NotificationSettings), + // Recording Rule fields will be implemented in the future. + // For now, no rules can be recording rules. So, we force these to be empty. + Record: nil, }, nil } diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index d28ff8ecb70..50e373afc61 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -5,14 +5,17 @@ import ( "encoding/json" "errors" "fmt" + "hash/fnv" "sort" "strconv" "strings" "time" + "unsafe" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" alertingModels "github.com/grafana/alerting/models" + "github.com/grafana/grafana-plugin-sdk-go/data" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/setting" @@ -239,7 +242,8 @@ type AlertRule struct { DashboardUID *string `xorm:"dashboard_uid"` PanelID *int64 `xorm:"panel_id"` RuleGroup string - RuleGroupIndex int `xorm:"rule_group_idx"` + RuleGroupIndex int `xorm:"rule_group_idx"` + Record *Record `xorm:"text null 'record'"` NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -514,6 +518,10 @@ 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) @@ -561,6 +569,7 @@ type AlertRuleVersion struct { Condition string Data []AlertQuery IntervalSeconds int64 + Record *Record `xorm:"text null 'record'"` NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -757,3 +766,26 @@ func GroupByAlertRuleGroupKey(rules []*AlertRule) map[AlertRuleGroupKey]RulesGro } return result } + +// Record contains mapping information for Recording Rules. +type Record struct { + // Metric indicates a metric name to send results to. + Metric string + // From contains a query RefID, indicating which expression node is the output of the recording rule. + From string +} + +func (r *Record) Fingerprint() data.Fingerprint { + h := fnv.New64() + + writeString := func(s string) { + // save on extra slice allocation when string is converted to bytes. + _, _ = h.Write(unsafe.Slice(unsafe.StringData(s), len(s))) //nolint:gosec + // ignore errors returned by Write method because fnv never returns them. + _, _ = h.Write([]byte{255}) // use an invalid utf-8 sequence as separator + } + + writeString(r.Metric) + writeString(r.From) + return data.Fingerprint(h.Sum64()) +} diff --git a/pkg/services/ngalert/models/alert_rule_test.go b/pkg/services/ngalert/models/alert_rule_test.go index 414507582f5..3cfbb07a392 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -505,6 +505,13 @@ func TestDiff(t *testing.T) { assert.Equal(t, rule2.RuleGroupIndex, diff[0].Right.Interface()) difCnt++ } + if rule1.Record != rule2.Record { + diff := diffs.GetDiffsForField("Record") + assert.Len(t, diff, 1) + assert.Equal(t, rule1.Record, diff[0].Left.String()) + assert.Equal(t, rule2.Record, diff[0].Right.String()) + difCnt++ + } require.Lenf(t, diffs, difCnt, "Got some unexpected diffs. Either add to ignore or add assert to it") diff --git a/pkg/services/ngalert/schedule/registry.go b/pkg/services/ngalert/schedule/registry.go index 6a8ec87f19a..d2214da9aaf 100644 --- a/pkg/services/ngalert/schedule/registry.go +++ b/pkg/services/ngalert/schedule/registry.go @@ -311,5 +311,10 @@ func (r ruleWithFolder) Fingerprint() fingerprint { writeInt(int64(rule.RuleGroupIndex)) writeString(string(rule.NoDataState)) writeString(string(rule.ExecErrState)) + if rule.Record != nil { + binary.LittleEndian.PutUint64(tmp, uint64(rule.Record.Fingerprint())) + writeBytes(tmp) + } + return fingerprint(sum.Sum64()) } diff --git a/pkg/services/ngalert/schedule/registry_test.go b/pkg/services/ngalert/schedule/registry_test.go index d7628cdd356..b485d9cc024 100644 --- a/pkg/services/ngalert/schedule/registry_test.go +++ b/pkg/services/ngalert/schedule/registry_test.go @@ -192,6 +192,7 @@ func TestRuleWithFolderFingerprint(t *testing.T) { RuleGroupIndex: 1, NoDataState: "test-nodata", ExecErrState: "test-err", + Record: &models.Record{Metric: "my_metric", From: "A"}, For: 12, Annotations: map[string]string{ "key-annotation": "value-annotation", @@ -230,6 +231,7 @@ func TestRuleWithFolderFingerprint(t *testing.T) { RuleGroupIndex: 22, NoDataState: "test-nodata2", ExecErrState: "test-err2", + Record: &models.Record{Metric: "my_metric2", From: "B"}, For: 1141, Annotations: map[string]string{ "key-annotation2": "value-annotation", diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index 09dfc1e0fad..53a31f18d69 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -161,6 +161,7 @@ func (st DBstore) InsertAlertRules(ctx context.Context, rules []ngmodels.AlertRu For: r.For, Annotations: r.Annotations, Labels: r.Labels, + Record: r.Record, NotificationSettings: r.NotificationSettings, }) } @@ -238,6 +239,7 @@ func (st DBstore) UpdateAlertRules(ctx context.Context, rules []ngmodels.UpdateR IntervalSeconds: r.New.IntervalSeconds, NoDataState: r.New.NoDataState, ExecErrState: r.New.ExecErrState, + Record: r.New.Record, For: r.New.For, Annotations: r.New.Annotations, Labels: r.New.Labels, diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index 7722b403949..bb567809d37 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -643,6 +643,12 @@ func TestIntegrationInsertAlertRules(t *testing.T) { require.Truef(t, found, "Rule with key %#v was not found in database", keyWithID) } + 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) + } + }) + t.Run("fail to insert rules with same ID", func(t *testing.T) { _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{deref[0]}) require.ErrorIs(t, err, models.ErrAlertRuleConflictBase) diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index b16c49b9e74..ae88297da96 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -121,6 +121,8 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) { accesscontrol.AddAlertingScopeRemovalMigration(mg) accesscontrol.AddManagedFolderAlertingSilencesActionsMigrator(mg) + + ualert.AddRecordingRuleColumns(mg) } func addStarMigrations(mg *Migrator) { diff --git a/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go b/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go new file mode 100644 index 00000000000..b593836a692 --- /dev/null +++ b/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go @@ -0,0 +1,18 @@ +package ualert + +import "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + +// AddRecordingRuleColumns adds columns to alert_rule to represent recording rules. +func AddRecordingRuleColumns(mg *migrator.Migrator) { + mg.AddMigration("add record column to alert_rule table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule"}, &migrator.Column{ + Name: "record", + Type: migrator.DB_Text, // Text, to allow for future growth, as this contains a JSON-ified struct. + Nullable: true, + })) + + mg.AddMigration("add record column to alert_rule_version table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule_version"}, &migrator.Column{ + Name: "record", + Type: migrator.DB_Text, + Nullable: true, + })) +}