Skip to content

Commit

Permalink
Try a single outer struct with all fields embedded
Browse files Browse the repository at this point in the history
  • Loading branch information
alexweav committed May 3, 2024
1 parent 715134a commit e1248ab
Show file tree
Hide file tree
Showing 10 changed files with 46 additions and 57 deletions.
3 changes: 1 addition & 2 deletions pkg/services/ngalert/api/api_ruler_validation.go
Expand Up @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions pkg/services/ngalert/api/api_ruler_validation_test.go
Expand Up @@ -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)
},
},
{
Expand Down
3 changes: 1 addition & 2 deletions pkg/services/ngalert/api/compat.go
Expand Up @@ -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
}

Expand Down
39 changes: 30 additions & 9 deletions 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,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
Expand Down Expand Up @@ -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)
}

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

Expand Down
7 changes: 5 additions & 2 deletions pkg/services/ngalert/schedule/registry.go
Expand Up @@ -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())
}
6 changes: 2 additions & 4 deletions pkg/services/ngalert/schedule/registry_test.go
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 0 additions & 2 deletions pkg/services/ngalert/store/alert_rule.go
Expand Up @@ -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,
})
}
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 2 additions & 3 deletions pkg/services/ngalert/store/alert_rule_test.go
Expand Up @@ -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)
}
})

Expand Down
28 changes: 4 additions & 24 deletions pkg/services/sqlstore/migrations/ualert/recording_rules_mig.go
Expand Up @@ -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,
}))
}

0 comments on commit e1248ab

Please sign in to comment.