From 042f7c9eb485100dd7f3b853beb128639b335b7c Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 15:31:59 -0500 Subject: [PATCH 01/16] Create recording rule fields in model --- pkg/services/ngalert/models/alert_rule.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index d28ff8ecb705..675f55b61fbc 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -240,6 +240,9 @@ type AlertRule struct { PanelID *int64 `xorm:"panel_id"` RuleGroup string RuleGroupIndex int `xorm:"rule_group_idx"` + Record string + RecordFrom string + RecordTo *DataSourceRef NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -757,3 +760,8 @@ func GroupByAlertRuleGroupKey(rules []*AlertRule) map[AlertRuleGroupKey]RulesGro } return result } + +type DataSourceRef struct { + Type string + UID string +} From 206f961a38eee6fd6f8db0904d22e9da139d260b Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 16:02:07 -0500 Subject: [PATCH 02/16] Add migration --- .../sqlstore/migrations/migrations.go | 2 + .../migrations/ualert/recording_rules_mig.go | 50 +++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index b16c49b9e74e..ae88297da962 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 000000000000..bd3e50f16c3f --- /dev/null +++ b/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go @@ -0,0 +1,50 @@ +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_NVarchar, + Length: DefaultFieldMaxLength, + Default: "''", + Nullable: false, + })) + + 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_NVarchar, + Length: DefaultFieldMaxLength, + Default: "''", + Nullable: false, + })) + + mg.AddMigration("add record_from column to alert_rule table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule"}, &migrator.Column{ + Name: "record_from", + Type: migrator.DB_NVarchar, + Length: DefaultFieldMaxLength, + Default: "''", + Nullable: false, + })) + + mg.AddMigration("add record_from column to alert_rule_version table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule_version"}, &migrator.Column{ + Name: "record_from", + Type: migrator.DB_NVarchar, + Length: DefaultFieldMaxLength, + Default: "''", + Nullable: false, + })) + + mg.AddMigration("add record_to column to alert_rule table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule"}, &migrator.Column{ + Name: "record_to", + Type: migrator.DB_Text, // Text, to allow for future growth, as this contains a JSON-ified struct. + Nullable: true, + })) + + mg.AddMigration("add record_to column to alert_rule_version table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule_version"}, &migrator.Column{ + Name: "record_to", + Type: migrator.DB_Text, // Text, to allow for future growth, as this contains a JSON-ified struct. + Nullable: true, + })) +} From c916d4900e53369b80e98ee94423697ad29aa2f2 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 16:07:50 -0500 Subject: [PATCH 03/16] Write to database, support in version table --- pkg/services/ngalert/models/alert_rule.go | 13 +++++++++++++ pkg/services/ngalert/store/alert_rule.go | 3 +++ 2 files changed, 16 insertions(+) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 675f55b61fbc..5f73accf8bc0 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -564,6 +564,9 @@ type AlertRuleVersion struct { Condition string Data []AlertQuery IntervalSeconds int64 + Record string + RecordFrom string + RecordTo *DataSourceRef NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -765,3 +768,13 @@ type DataSourceRef struct { Type string UID string } + +// FromDB implements Conversion from xorm, and tells the ORM to store this object as a JSON blob in the database. +func (dsr *DataSourceRef) FromDB(b []byte) error { + return json.Unmarshal(b, dsr) +} + +// ToDB implements Conversion from xorm, and tells the ORM to store this object as a JSON blob in the database. +func (dsr *DataSourceRef) ToDB() ([]byte, error) { + return json.Marshal(dsr) +} diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index 09dfc1e0fad4..76647ea736ed 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -161,6 +161,9 @@ func (st DBstore) InsertAlertRules(ctx context.Context, rules []ngmodels.AlertRu For: r.For, Annotations: r.Annotations, Labels: r.Labels, + Record: r.Record, + RecordFrom: r.RecordFrom, + RecordTo: r.RecordTo, NotificationSettings: r.NotificationSettings, }) } From b45677f8711ff31ccc508c16eefece7b2085ba40 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 16:21:45 -0500 Subject: [PATCH 04/16] extend fingerprint --- pkg/services/ngalert/schedule/registry.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/services/ngalert/schedule/registry.go b/pkg/services/ngalert/schedule/registry.go index 6a8ec87f19a9..4410b2434222 100644 --- a/pkg/services/ngalert/schedule/registry.go +++ b/pkg/services/ngalert/schedule/registry.go @@ -311,5 +311,11 @@ func (r ruleWithFolder) Fingerprint() fingerprint { writeInt(int64(rule.RuleGroupIndex)) writeString(string(rule.NoDataState)) writeString(string(rule.ExecErrState)) + writeString(string(rule.Record)) + writeString(string(rule.RecordFrom)) + if rule.RecordTo != nil { + writeString(string(rule.RecordTo.Type)) + writeString(string(rule.RecordTo.UID)) + } return fingerprint(sum.Sum64()) } From 09189aa510b22f07b4e40fffa48079ddf078df31 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 16:27:24 -0500 Subject: [PATCH 05/16] Force fields to be empty on validate --- pkg/services/ngalert/api/api_ruler_validation.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/services/ngalert/api/api_ruler_validation.go b/pkg/services/ngalert/api/api_ruler_validation.go index 91a012b0b1e0..ad3eeca2baf7 100644 --- a/pkg/services/ngalert/api/api_ruler_validation.go +++ b/pkg/services/ngalert/api/api_ruler_validation.go @@ -108,6 +108,11 @@ 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: "", + RecordFrom: "", + RecordTo: nil, } if ruleNode.GrafanaManagedAlert.NotificationSettings != nil { From 2e8b27f7f9ea21b02c49fb98af4bbae627e97e64 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 16:34:20 -0500 Subject: [PATCH 06/16] Another storage spot, tests for fingerprint --- pkg/services/ngalert/schedule/registry_test.go | 6 ++++++ pkg/services/ngalert/store/alert_rule.go | 3 +++ 2 files changed, 9 insertions(+) diff --git a/pkg/services/ngalert/schedule/registry_test.go b/pkg/services/ngalert/schedule/registry_test.go index d7628cdd3568..ecadd6ac313b 100644 --- a/pkg/services/ngalert/schedule/registry_test.go +++ b/pkg/services/ngalert/schedule/registry_test.go @@ -192,6 +192,9 @@ func TestRuleWithFolderFingerprint(t *testing.T) { RuleGroupIndex: 1, NoDataState: "test-nodata", ExecErrState: "test-err", + Record: "my_metric", + RecordFrom: "A", + RecordTo: &models.DataSourceRef{Type: "prometheus", UID: "prometheus-ds-uid"}, For: 12, Annotations: map[string]string{ "key-annotation": "value-annotation", @@ -230,6 +233,9 @@ func TestRuleWithFolderFingerprint(t *testing.T) { RuleGroupIndex: 22, NoDataState: "test-nodata2", ExecErrState: "test-err2", + Record: "my_metric2", + RecordFrom: "B", + RecordTo: &models.DataSourceRef{Type: "prometheus2", UID: "prometheus-ds-uid-2"}, 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 76647ea736ed..6a6e8047194d 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -241,6 +241,9 @@ 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, + RecordFrom: r.New.RecordFrom, + RecordTo: r.New.RecordTo, For: r.New.For, Annotations: r.New.Annotations, Labels: r.New.Labels, From 1e55c6c8ab6c005a7f8a2e2452a9e531dae8d1ec Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 16:44:50 -0500 Subject: [PATCH 07/16] Explicitly set defaults in provisioning API --- pkg/services/ngalert/api/compat.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/services/ngalert/api/compat.go b/pkg/services/ngalert/api/compat.go index 8f671b113416..e8f2405f9488 100644 --- a/pkg/services/ngalert/api/compat.go +++ b/pkg/services/ngalert/api/compat.go @@ -31,6 +31,11 @@ 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: "", + RecordFrom: "", + RecordTo: nil, }, nil } From 9786625742d663aa00735f5974c7d779b0659568 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 16:56:26 -0500 Subject: [PATCH 08/16] Tests for main API validation --- pkg/services/ngalert/api/api_ruler_validation_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/services/ngalert/api/api_ruler_validation_test.go b/pkg/services/ngalert/api/api_ruler_validation_test.go index f2332211129b..d6c9c046a325 100644 --- a/pkg/services/ngalert/api/api_ruler_validation_test.go +++ b/pkg/services/ngalert/api/api_ruler_validation_test.go @@ -339,6 +339,9 @@ 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.Empty(t, alert.Record) + require.Empty(t, alert.RecordFrom) + require.Nil(t, alert.RecordTo) }, }, { From a9e57f187857fee811196d07d6d3224f8a1249e7 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Thu, 25 Apr 2024 17:04:04 -0500 Subject: [PATCH 09/16] Add diff tests even though fields are unpopulated for now --- pkg/services/ngalert/models/alert_rule.go | 10 ++++++++++ .../ngalert/models/alert_rule_test.go | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 5f73accf8bc0..480eed1c8997 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -517,6 +517,16 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting } } + if alertRule.Record != "" { + return fmt.Errorf("%w: Storing recording rules is not yet allowed.", ErrAlertRuleFailedValidation) + } + if alertRule.RecordFrom != "" { + return fmt.Errorf("%w: Storing recording rules is not yet allowed.", ErrAlertRuleFailedValidation) + } + if alertRule.RecordTo != 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) diff --git a/pkg/services/ngalert/models/alert_rule_test.go b/pkg/services/ngalert/models/alert_rule_test.go index 414507582f59..11dfbdc4a08f 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -505,6 +505,25 @@ 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++ + } + if rule1.RecordFrom != rule2.RecordFrom { + diff := diffs.GetDiffsForField("RecordFrom") + assert.Len(t, diff, 1) + assert.Equal(t, rule1.RecordFrom, diff[0].Left.String()) + assert.Equal(t, rule2.RecordFrom, diff[0].Right.String()) + difCnt++ + } + if rule1.RecordTo != rule2.RecordTo { + diff := diffs.GetDiffsForField("RecordTo") + assert.Len(t, diff, 1) + difCnt++ + } require.Lenf(t, diffs, difCnt, "Got some unexpected diffs. Either add to ignore or add assert to it") From 84616f981b712da91bfb3f1f78b95170268ff59b Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Fri, 26 Apr 2024 12:38:30 -0500 Subject: [PATCH 10/16] Use struct tag approach instead of FromDB/ToDB hooks as it better handles nulls when deserializing --- pkg/services/ngalert/models/alert_rule.go | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 480eed1c8997..8c76911e4c00 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -242,7 +242,7 @@ type AlertRule struct { RuleGroupIndex int `xorm:"rule_group_idx"` Record string RecordFrom string - RecordTo *DataSourceRef + RecordTo *DataSourceRef `xorm:"text null 'record_to'"` // tells xorm to store this field as a nullable JSON string NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -576,7 +576,7 @@ type AlertRuleVersion struct { IntervalSeconds int64 Record string RecordFrom string - RecordTo *DataSourceRef + RecordTo *DataSourceRef `xorm:"text null 'record_to'"` NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -778,13 +778,3 @@ type DataSourceRef struct { Type string UID string } - -// FromDB implements Conversion from xorm, and tells the ORM to store this object as a JSON blob in the database. -func (dsr *DataSourceRef) FromDB(b []byte) error { - return json.Unmarshal(b, dsr) -} - -// ToDB implements Conversion from xorm, and tells the ORM to store this object as a JSON blob in the database. -func (dsr *DataSourceRef) ToDB() ([]byte, error) { - return json.Marshal(dsr) -} From 844d9401142b14ed9993637b1553b47002e70109 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Fri, 26 Apr 2024 15:49:42 -0500 Subject: [PATCH 11/16] test for deser --- pkg/services/ngalert/store/alert_rule_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index 7722b4039490..c9c1106d2f60 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -643,6 +643,14 @@ 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 default recording rule fields on model", func(t *testing.T) { + for _, rule := range dbRules { + require.Empty(t, rule.Record) + require.Empty(t, rule.RecordFrom) + require.Nil(t, rule.RecordTo) + } + }) + 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) From 422018ee29837a65f471e93ca5f9c0a196dc90d8 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Fri, 26 Apr 2024 15:53:41 -0500 Subject: [PATCH 12/16] Backout RecordTo for now since it's not decided in the doc --- pkg/services/ngalert/api/api_ruler_validation.go | 1 - pkg/services/ngalert/api/api_ruler_validation_test.go | 1 - pkg/services/ngalert/api/compat.go | 1 - pkg/services/ngalert/models/alert_rule.go | 5 ----- pkg/services/ngalert/models/alert_rule_test.go | 5 ----- pkg/services/ngalert/schedule/registry.go | 4 ---- pkg/services/ngalert/schedule/registry_test.go | 2 -- pkg/services/ngalert/store/alert_rule.go | 2 -- pkg/services/ngalert/store/alert_rule_test.go | 1 - 9 files changed, 22 deletions(-) diff --git a/pkg/services/ngalert/api/api_ruler_validation.go b/pkg/services/ngalert/api/api_ruler_validation.go index ad3eeca2baf7..ebac6fc37fad 100644 --- a/pkg/services/ngalert/api/api_ruler_validation.go +++ b/pkg/services/ngalert/api/api_ruler_validation.go @@ -112,7 +112,6 @@ func validateRuleNode( // For now, no rules can be recording rules. So, we force these to be empty. Record: "", RecordFrom: "", - RecordTo: 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 d6c9c046a325..c79e125c0f78 100644 --- a/pkg/services/ngalert/api/api_ruler_validation_test.go +++ b/pkg/services/ngalert/api/api_ruler_validation_test.go @@ -341,7 +341,6 @@ func TestValidateRuleNode_NoUID(t *testing.T) { require.Equal(t, api.ApiRuleNode.Labels, alert.Labels) require.Empty(t, alert.Record) require.Empty(t, alert.RecordFrom) - require.Nil(t, alert.RecordTo) }, }, { diff --git a/pkg/services/ngalert/api/compat.go b/pkg/services/ngalert/api/compat.go index e8f2405f9488..d37d9b047e20 100644 --- a/pkg/services/ngalert/api/compat.go +++ b/pkg/services/ngalert/api/compat.go @@ -35,7 +35,6 @@ func AlertRuleFromProvisionedAlertRule(a definitions.ProvisionedAlertRule) (mode // For now, no rules can be recording rules. So, we force these to be empty. Record: "", RecordFrom: "", - RecordTo: nil, }, nil } diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 8c76911e4c00..36aaf2f3866c 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -242,7 +242,6 @@ type AlertRule struct { RuleGroupIndex int `xorm:"rule_group_idx"` Record string RecordFrom string - RecordTo *DataSourceRef `xorm:"text null 'record_to'"` // tells xorm to store this field as a nullable JSON string NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -523,9 +522,6 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting if alertRule.RecordFrom != "" { return fmt.Errorf("%w: Storing recording rules is not yet allowed.", ErrAlertRuleFailedValidation) } - if alertRule.RecordTo != nil { - return fmt.Errorf("%w: Storing recording rules is not yet allowed.", ErrAlertRuleFailedValidation) - } if len(alertRule.NotificationSettings) > 0 { if len(alertRule.NotificationSettings) != 1 { @@ -576,7 +572,6 @@ type AlertRuleVersion struct { IntervalSeconds int64 Record string RecordFrom string - RecordTo *DataSourceRef `xorm:"text null 'record_to'"` NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration diff --git a/pkg/services/ngalert/models/alert_rule_test.go b/pkg/services/ngalert/models/alert_rule_test.go index 11dfbdc4a08f..34703da4712e 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -519,11 +519,6 @@ func TestDiff(t *testing.T) { assert.Equal(t, rule2.RecordFrom, diff[0].Right.String()) difCnt++ } - if rule1.RecordTo != rule2.RecordTo { - diff := diffs.GetDiffsForField("RecordTo") - assert.Len(t, diff, 1) - 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 4410b2434222..e2fcd2476ca7 100644 --- a/pkg/services/ngalert/schedule/registry.go +++ b/pkg/services/ngalert/schedule/registry.go @@ -313,9 +313,5 @@ func (r ruleWithFolder) Fingerprint() fingerprint { writeString(string(rule.ExecErrState)) writeString(string(rule.Record)) writeString(string(rule.RecordFrom)) - if rule.RecordTo != nil { - writeString(string(rule.RecordTo.Type)) - writeString(string(rule.RecordTo.UID)) - } return fingerprint(sum.Sum64()) } diff --git a/pkg/services/ngalert/schedule/registry_test.go b/pkg/services/ngalert/schedule/registry_test.go index ecadd6ac313b..36044daafc75 100644 --- a/pkg/services/ngalert/schedule/registry_test.go +++ b/pkg/services/ngalert/schedule/registry_test.go @@ -194,7 +194,6 @@ func TestRuleWithFolderFingerprint(t *testing.T) { ExecErrState: "test-err", Record: "my_metric", RecordFrom: "A", - RecordTo: &models.DataSourceRef{Type: "prometheus", UID: "prometheus-ds-uid"}, For: 12, Annotations: map[string]string{ "key-annotation": "value-annotation", @@ -235,7 +234,6 @@ func TestRuleWithFolderFingerprint(t *testing.T) { ExecErrState: "test-err2", Record: "my_metric2", RecordFrom: "B", - RecordTo: &models.DataSourceRef{Type: "prometheus2", UID: "prometheus-ds-uid-2"}, 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 6a6e8047194d..dbad2a18b8aa 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -163,7 +163,6 @@ func (st DBstore) InsertAlertRules(ctx context.Context, rules []ngmodels.AlertRu Labels: r.Labels, Record: r.Record, RecordFrom: r.RecordFrom, - RecordTo: r.RecordTo, NotificationSettings: r.NotificationSettings, }) } @@ -243,7 +242,6 @@ func (st DBstore) UpdateAlertRules(ctx context.Context, rules []ngmodels.UpdateR ExecErrState: r.New.ExecErrState, Record: r.New.Record, RecordFrom: r.New.RecordFrom, - RecordTo: r.New.RecordTo, 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 c9c1106d2f60..179362b2d0b3 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -647,7 +647,6 @@ func TestIntegrationInsertAlertRules(t *testing.T) { for _, rule := range dbRules { require.Empty(t, rule.Record) require.Empty(t, rule.RecordFrom) - require.Nil(t, rule.RecordTo) } }) From 7cbc5f99a81ab3c61161bf291f040f7d9eef93ed Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Fri, 26 Apr 2024 15:57:04 -0500 Subject: [PATCH 13/16] back out of migration too --- .../migrations/ualert/recording_rules_mig.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go b/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go index bd3e50f16c3f..8f034cc08d77 100644 --- a/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go +++ b/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go @@ -35,16 +35,4 @@ func AddRecordingRuleColumns(mg *migrator.Migrator) { Default: "''", Nullable: false, })) - - mg.AddMigration("add record_to column to alert_rule table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule"}, &migrator.Column{ - Name: "record_to", - Type: migrator.DB_Text, // Text, to allow for future growth, as this contains a JSON-ified struct. - Nullable: true, - })) - - mg.AddMigration("add record_to column to alert_rule_version table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule_version"}, &migrator.Column{ - Name: "record_to", - Type: migrator.DB_Text, // Text, to allow for future growth, as this contains a JSON-ified struct. - Nullable: true, - })) } From 5df33ee7f52dfa440f4f1cbc5b80ce51ddc6a038 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Fri, 26 Apr 2024 16:03:47 -0500 Subject: [PATCH 14/16] Drop datasourceref for now --- pkg/services/ngalert/models/alert_rule.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 36aaf2f3866c..33f7ec3a89b8 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -768,8 +768,3 @@ func GroupByAlertRuleGroupKey(rules []*AlertRule) map[AlertRuleGroupKey]RulesGro } return result } - -type DataSourceRef struct { - Type string - UID string -} From 715134ad474bfe85f232d4adb163ec4f3fa7e8da Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Mon, 29 Apr 2024 12:21:59 -0500 Subject: [PATCH 15/16] address linter complaints --- pkg/services/ngalert/models/alert_rule.go | 4 ++-- pkg/services/ngalert/schedule/registry.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 33f7ec3a89b8..050991ceb437 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -517,10 +517,10 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting } if alertRule.Record != "" { - return fmt.Errorf("%w: Storing recording rules is not yet allowed.", ErrAlertRuleFailedValidation) + return fmt.Errorf("%w: storing recording rules is not yet allowed", ErrAlertRuleFailedValidation) } if alertRule.RecordFrom != "" { - return fmt.Errorf("%w: Storing recording rules is not yet allowed.", ErrAlertRuleFailedValidation) + return fmt.Errorf("%w: storing recording rules is not yet allowed", ErrAlertRuleFailedValidation) } if len(alertRule.NotificationSettings) > 0 { diff --git a/pkg/services/ngalert/schedule/registry.go b/pkg/services/ngalert/schedule/registry.go index e2fcd2476ca7..c8d8a92a82f6 100644 --- a/pkg/services/ngalert/schedule/registry.go +++ b/pkg/services/ngalert/schedule/registry.go @@ -311,7 +311,7 @@ func (r ruleWithFolder) Fingerprint() fingerprint { writeInt(int64(rule.RuleGroupIndex)) writeString(string(rule.NoDataState)) writeString(string(rule.ExecErrState)) - writeString(string(rule.Record)) - writeString(string(rule.RecordFrom)) + writeString(rule.Record) + writeString(rule.RecordFrom) return fingerprint(sum.Sum64()) } From e1248ab859d2fa59798e478625fd2de5b814fd8e Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Fri, 3 May 2024 16:38:24 -0500 Subject: [PATCH 16/16] Try a single outer struct with all fields embedded --- .../ngalert/api/api_ruler_validation.go | 3 +- .../ngalert/api/api_ruler_validation_test.go | 3 +- pkg/services/ngalert/api/compat.go | 3 +- pkg/services/ngalert/models/alert_rule.go | 39 ++++++++++++++----- .../ngalert/models/alert_rule_test.go | 7 ---- pkg/services/ngalert/schedule/registry.go | 7 +++- .../ngalert/schedule/registry_test.go | 6 +-- pkg/services/ngalert/store/alert_rule.go | 2 - pkg/services/ngalert/store/alert_rule_test.go | 5 +-- .../migrations/ualert/recording_rules_mig.go | 28 ++----------- 10 files changed, 46 insertions(+), 57 deletions(-) diff --git a/pkg/services/ngalert/api/api_ruler_validation.go b/pkg/services/ngalert/api/api_ruler_validation.go index ebac6fc37fad..2fa3100ac586 100644 --- a/pkg/services/ngalert/api/api_ruler_validation.go +++ b/pkg/services/ngalert/api/api_ruler_validation.go @@ -110,8 +110,7 @@ func validateRuleNode( 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: "", - RecordFrom: "", + 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 c79e125c0f78..46d6406bb172 100644 --- a/pkg/services/ngalert/api/api_ruler_validation_test.go +++ b/pkg/services/ngalert/api/api_ruler_validation_test.go @@ -339,8 +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.Empty(t, alert.Record) - require.Empty(t, alert.RecordFrom) + require.Nil(t, alert.Record) }, }, { diff --git a/pkg/services/ngalert/api/compat.go b/pkg/services/ngalert/api/compat.go index d37d9b047e20..a0c2390254c7 100644 --- a/pkg/services/ngalert/api/compat.go +++ b/pkg/services/ngalert/api/compat.go @@ -33,8 +33,7 @@ func AlertRuleFromProvisionedAlertRule(a definitions.ProvisionedAlertRule) (mode 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: "", - RecordFrom: "", + Record: nil, }, nil } diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 050991ceb437..50e373afc614 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,9 +242,8 @@ type AlertRule struct { DashboardUID *string `xorm:"dashboard_uid"` PanelID *int64 `xorm:"panel_id"` RuleGroup string - RuleGroupIndex int `xorm:"rule_group_idx"` - Record string - RecordFrom string + RuleGroupIndex int `xorm:"rule_group_idx"` + Record *Record `xorm:"text null 'record'"` NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -516,10 +518,7 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting } } - if alertRule.Record != "" { - return fmt.Errorf("%w: storing recording rules is not yet allowed", ErrAlertRuleFailedValidation) - } - if alertRule.RecordFrom != "" { + if alertRule.Record != nil { return fmt.Errorf("%w: storing recording rules is not yet allowed", ErrAlertRuleFailedValidation) } @@ -570,8 +569,7 @@ type AlertRuleVersion struct { Condition string Data []AlertQuery IntervalSeconds int64 - Record string - RecordFrom string + Record *Record `xorm:"text null 'record'"` NoDataState NoDataState ExecErrState ExecutionErrorState // ideally this field should have been apimodels.ApiDuration @@ -768,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 34703da4712e..3cfbb07a3924 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -512,13 +512,6 @@ func TestDiff(t *testing.T) { assert.Equal(t, rule2.Record, diff[0].Right.String()) difCnt++ } - if rule1.RecordFrom != rule2.RecordFrom { - diff := diffs.GetDiffsForField("RecordFrom") - assert.Len(t, diff, 1) - assert.Equal(t, rule1.RecordFrom, diff[0].Left.String()) - assert.Equal(t, rule2.RecordFrom, 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 c8d8a92a82f6..d2214da9aaf5 100644 --- a/pkg/services/ngalert/schedule/registry.go +++ b/pkg/services/ngalert/schedule/registry.go @@ -311,7 +311,10 @@ func (r ruleWithFolder) Fingerprint() fingerprint { writeInt(int64(rule.RuleGroupIndex)) writeString(string(rule.NoDataState)) writeString(string(rule.ExecErrState)) - writeString(rule.Record) - writeString(rule.RecordFrom) + 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 36044daafc75..b485d9cc024a 100644 --- a/pkg/services/ngalert/schedule/registry_test.go +++ b/pkg/services/ngalert/schedule/registry_test.go @@ -192,8 +192,7 @@ func TestRuleWithFolderFingerprint(t *testing.T) { RuleGroupIndex: 1, NoDataState: "test-nodata", ExecErrState: "test-err", - Record: "my_metric", - RecordFrom: "A", + Record: &models.Record{Metric: "my_metric", From: "A"}, For: 12, Annotations: map[string]string{ "key-annotation": "value-annotation", @@ -232,8 +231,7 @@ func TestRuleWithFolderFingerprint(t *testing.T) { RuleGroupIndex: 22, NoDataState: "test-nodata2", ExecErrState: "test-err2", - Record: "my_metric2", - RecordFrom: "B", + 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 dbad2a18b8aa..53a31f18d69b 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -162,7 +162,6 @@ func (st DBstore) InsertAlertRules(ctx context.Context, rules []ngmodels.AlertRu Annotations: r.Annotations, Labels: r.Labels, Record: r.Record, - RecordFrom: r.RecordFrom, NotificationSettings: r.NotificationSettings, }) } @@ -241,7 +240,6 @@ func (st DBstore) UpdateAlertRules(ctx context.Context, rules []ngmodels.UpdateR NoDataState: r.New.NoDataState, ExecErrState: r.New.ExecErrState, Record: r.New.Record, - RecordFrom: r.New.RecordFrom, 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 179362b2d0b3..bb567809d37a 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -643,10 +643,9 @@ 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 default recording rule fields on model", func(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.Empty(t, rule.Record) - require.Empty(t, rule.RecordFrom) + require.Nil(t, rule.Record) } }) diff --git a/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go b/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go index 8f034cc08d77..b593836a6929 100644 --- a/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go +++ b/pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go @@ -6,33 +6,13 @@ import "github.com/grafana/grafana/pkg/services/sqlstore/migrator" 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_NVarchar, - Length: DefaultFieldMaxLength, - Default: "''", - Nullable: false, + 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_NVarchar, - Length: DefaultFieldMaxLength, - Default: "''", - Nullable: false, - })) - - mg.AddMigration("add record_from column to alert_rule table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule"}, &migrator.Column{ - Name: "record_from", - Type: migrator.DB_NVarchar, - Length: DefaultFieldMaxLength, - Default: "''", - Nullable: false, - })) - - mg.AddMigration("add record_from column to alert_rule_version table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule_version"}, &migrator.Column{ - Name: "record_from", - Type: migrator.DB_NVarchar, - Length: DefaultFieldMaxLength, - Default: "''", - Nullable: false, + Type: migrator.DB_Text, + Nullable: true, })) }