Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alerting: Add database migration for recording rule fields #87012

Merged
merged 16 commits into from May 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/services/ngalert/api/api_ruler_validation.go
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions pkg/services/ngalert/api/api_ruler_validation_test.go
Expand Up @@ -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)
},
},
{
Expand Down
3 changes: 3 additions & 0 deletions pkg/services/ngalert/api/compat.go
Expand Up @@ -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
}

Expand Down
34 changes: 33 additions & 1 deletion pkg/services/ngalert/models/alert_rule.go
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -757,3 +766,26 @@ func GroupByAlertRuleGroupKey(rules []*AlertRule) map[AlertRuleGroupKey]RulesGro
}
return result
}

// Record contains mapping information for Recording Rules.
type Record struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshedding nit: WDYT about Recording?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field name or the struct name? It's a field called Record, of type Record right now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think field name should remain (record also has meaning in prometheus). Struct name i'm totally good with changing if we want - but this can be done without DB migration anytime in future

// 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())
}
7 changes: 7 additions & 0 deletions pkg/services/ngalert/models/alert_rule_test.go
Expand Up @@ -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")

Expand Down
5 changes: 5 additions & 0 deletions pkg/services/ngalert/schedule/registry.go
Expand Up @@ -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())
}
2 changes: 2 additions & 0 deletions pkg/services/ngalert/schedule/registry_test.go
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions pkg/services/ngalert/store/alert_rule.go
Expand Up @@ -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,
})
}
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 6 additions & 0 deletions pkg/services/ngalert/store/alert_rule_test.go
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions pkg/services/sqlstore/migrations/migrations.go
Expand Up @@ -121,6 +121,8 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) {
accesscontrol.AddAlertingScopeRemovalMigration(mg)

accesscontrol.AddManagedFolderAlertingSilencesActionsMigrator(mg)

ualert.AddRecordingRuleColumns(mg)
}

func addStarMigrations(mg *Migrator) {
Expand Down
18 changes: 18 additions & 0 deletions 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,
}))
}