From 4ee48c2e772411bc180287dca7ec6b00eff74f61 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Wed, 23 Mar 2022 13:36:25 -0400 Subject: [PATCH] Alerting: Update GetRuleGroupAlertRules to accept optional rule group (#46889) * rename GetRuleGroupAlertRules to GetAlertRules * make rule group optional in GetAlertRulesQuery * simplify FakeStore. the current structure did not support optional rule group --- pkg/services/ngalert/api/api_ruler.go | 12 +- pkg/services/ngalert/api/api_ruler_test.go | 2 +- pkg/services/ngalert/models/alert_rule.go | 6 +- .../ngalert/schedule/schedule_unit_test.go | 6 +- pkg/services/ngalert/store/alert_rule.go | 21 ++- pkg/services/ngalert/store/testing.go | 150 ++++++------------ pkg/services/ngalert/tests/util.go | 6 +- 7 files changed, 73 insertions(+), 130 deletions(-) diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index a058a81fca5..e21d2f25636 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -139,12 +139,12 @@ func (srv RulerSrv) RouteGetRulegGroupConfig(c *models.ReqContext) response.Resp } ruleGroup := web.Params(c.Req)[":Groupname"] - q := ngmodels.ListRuleGroupAlertRulesQuery{ + q := ngmodels.GetAlertRulesQuery{ OrgID: c.SignedInUser.OrgId, NamespaceUID: namespace.Uid, - RuleGroup: ruleGroup, + RuleGroup: &ruleGroup, } - if err := srv.store.GetRuleGroupAlertRules(c.Req.Context(), &q); err != nil { + if err := srv.store.GetAlertRules(c.Req.Context(), &q); err != nil { return ErrResp(http.StatusInternalServerError, err, "failed to get group alert rules") } @@ -419,12 +419,12 @@ func (c *changes) isEmpty() bool { // calculateChanges calculates the difference between rules in the group in the database and the submitted rules. If a submitted rule has UID it tries to find it in the database (in other groups). // returns a list of rules that need to be added, updated and deleted. Deleted considered rules in the database that belong to the group but do not exist in the list of submitted rules. func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int64, namespace *models.Folder, ruleGroupName string, submittedRules []*ngmodels.AlertRule) (*changes, error) { - q := &ngmodels.ListRuleGroupAlertRulesQuery{ + q := &ngmodels.GetAlertRulesQuery{ OrgID: orgId, NamespaceUID: namespace.Uid, - RuleGroup: ruleGroupName, + RuleGroup: &ruleGroupName, } - if err := ruleStore.GetRuleGroupAlertRules(ctx, q); err != nil { + if err := ruleStore.GetAlertRules(ctx, q); err != nil { return nil, fmt.Errorf("failed to query database for rules in the group %s: %w", ruleGroupName, err) } existingGroupRules := q.Result diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index 3b5eb14425a..99d2375d927 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -221,7 +221,7 @@ func TestCalculateChanges(t *testing.T) { expectedErr := errors.New("TEST ERROR") fakeStore.Hook = func(cmd interface{}) error { switch cmd.(type) { - case models.ListRuleGroupAlertRulesQuery: + case models.GetAlertRulesQuery: return expectedErr } return nil diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index dbf5476f13c..8da7640f363 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -250,12 +250,12 @@ type ListNamespaceAlertRulesQuery struct { Result []*AlertRule } -// ListRuleGroupAlertRulesQuery is the query for listing rule group alert rules -type ListRuleGroupAlertRulesQuery struct { +// GetAlertRulesQuery is the query for listing rule group alert rules +type GetAlertRulesQuery struct { OrgID int64 // Namespace is the folder slug NamespaceUID string - RuleGroup string + RuleGroup *string // DashboardUID and PanelID are optional and allow filtering rules // to return just those for a dashboard and panel. diff --git a/pkg/services/ngalert/schedule/schedule_unit_test.go b/pkg/services/ngalert/schedule/schedule_unit_test.go index 6d23f950ca8..a6210307fa6 100644 --- a/pkg/services/ngalert/schedule/schedule_unit_test.go +++ b/pkg/services/ngalert/schedule/schedule_unit_test.go @@ -1117,12 +1117,12 @@ func CreateTestAlertRule(t *testing.T, dbstore *store.FakeRuleStore, intervalSec }) require.NoError(t, err) - q := models.ListRuleGroupAlertRulesQuery{ + q := models.GetAlertRulesQuery{ OrgID: orgID, NamespaceUID: "namespace", - RuleGroup: ruleGroup, + RuleGroup: &ruleGroup, } - err = dbstore.GetRuleGroupAlertRules(ctx, &q) + err = dbstore.GetAlertRules(ctx, &q) require.NoError(t, err) require.NotEmpty(t, q.Result) diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index fdfeac0d80e..97b2fa7ec4c 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -43,7 +43,7 @@ type RuleStore interface { GetAlertRulesForScheduling(ctx context.Context, query *ngmodels.ListAlertRulesQuery) error GetOrgAlertRules(ctx context.Context, query *ngmodels.ListAlertRulesQuery) error GetNamespaceAlertRules(ctx context.Context, query *ngmodels.ListNamespaceAlertRulesQuery) error - GetRuleGroupAlertRules(ctx context.Context, query *ngmodels.ListRuleGroupAlertRulesQuery) error + GetAlertRules(ctx context.Context, query *ngmodels.GetAlertRulesQuery) error GetNamespaces(context.Context, int64, *models.SignedInUser) (map[string]*models.Folder, error) GetNamespaceByTitle(context.Context, string, int64, *models.SignedInUser, bool) (*models.Folder, error) GetOrgRuleGroups(ctx context.Context, query *ngmodels.ListOrgRuleGroupsQuery) error @@ -315,23 +315,22 @@ func (st DBstore) GetNamespaceAlertRules(ctx context.Context, query *ngmodels.Li }) } -// GetRuleGroupAlertRules is a handler for retrieving rule group alert rules of specific organisation. -func (st DBstore) GetRuleGroupAlertRules(ctx context.Context, query *ngmodels.ListRuleGroupAlertRulesQuery) error { +// GetAlertRules is a handler for retrieving rule group alert rules of specific organisation. +func (st DBstore) GetAlertRules(ctx context.Context, query *ngmodels.GetAlertRulesQuery) error { return st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { - q := "SELECT * FROM alert_rule WHERE org_id = ? and namespace_uid = ? and rule_group = ?" - args := []interface{}{query.OrgID, query.NamespaceUID, query.RuleGroup} - + q := sess.Table("alert_rule").Where("org_id = ? AND namespace_uid = ?", query.OrgID, query.NamespaceUID) + if query.RuleGroup != nil { + q = q.Where("rule_group = ?", *query.RuleGroup) + } if query.DashboardUID != "" { - q = fmt.Sprintf("%s and dashboard_uid = ?", q) - args = append(args, query.DashboardUID) + q = q.Where("dashboard_uid = ?", query.DashboardUID) if query.PanelID != 0 { - q = fmt.Sprintf("%s and panel_id = ?", q) - args = append(args, query.PanelID) + q = q.Where("panel_id = ?", query.PanelID) } } alertRules := make([]*ngmodels.AlertRule, 0) - if err := sess.SQL(q, args...).Find(&alertRules); err != nil { + if err := q.Find(&alertRules); err != nil { return err } diff --git a/pkg/services/ngalert/store/testing.go b/pkg/services/ngalert/store/testing.go index 8d54fe57e58..62f37d1dfd5 100644 --- a/pkg/services/ngalert/store/testing.go +++ b/pkg/services/ngalert/store/testing.go @@ -25,7 +25,7 @@ import ( func NewFakeRuleStore(t *testing.T) *FakeRuleStore { return &FakeRuleStore{ t: t, - Rules: map[int64]map[string]map[string][]*models.AlertRule{}, + Rules: map[int64][]*models.AlertRule{}, Hook: func(interface{}) error { return nil }, @@ -37,7 +37,7 @@ type FakeRuleStore struct { t *testing.T mtx sync.Mutex // OrgID -> RuleGroup -> Namespace -> Rules - Rules map[int64]map[string]map[string][]*models.AlertRule + Rules map[int64][]*models.AlertRule Hook func(cmd interface{}) error // use Hook if you need to intercept some query and return an error RecordedOps []interface{} } @@ -48,27 +48,15 @@ func (f *FakeRuleStore) PutRule(_ context.Context, rules ...*models.AlertRule) { defer f.mtx.Unlock() mainloop: for _, r := range rules { - rgs, ok := f.Rules[r.OrgID] - if !ok { - f.Rules[r.OrgID] = map[string]map[string][]*models.AlertRule{} - } - - rg, ok := rgs[r.RuleGroup] - if !ok { - f.Rules[r.OrgID][r.RuleGroup] = map[string][]*models.AlertRule{} - } - - _, ok = rg[r.NamespaceUID] - if !ok { - f.Rules[r.OrgID][r.RuleGroup][r.NamespaceUID] = []*models.AlertRule{} - } - for idx, rulePtr := range f.Rules[r.OrgID][r.RuleGroup][r.NamespaceUID] { + rgs := f.Rules[r.OrgID] + for idx, rulePtr := range rgs { if rulePtr.UID == r.UID { - f.Rules[r.OrgID][r.RuleGroup][r.NamespaceUID][idx] = r + rgs[idx] = r continue mainloop } } - f.Rules[r.OrgID][r.RuleGroup][r.NamespaceUID] = append(f.Rules[r.OrgID][r.RuleGroup][r.NamespaceUID], r) + rgs = append(rgs, r) + f.Rules[r.OrgID] = rgs } } @@ -105,22 +93,17 @@ func (f *FakeRuleStore) GetAlertRuleByUID(_ context.Context, q *models.GetAlertR if err := f.Hook(*q); err != nil { return err } - rgs, ok := f.Rules[q.OrgID] + rules, ok := f.Rules[q.OrgID] if !ok { return nil } - for _, rg := range rgs { - for _, rules := range rg { - for _, r := range rules { - if r.UID == q.UID { - q.Result = r - break - } - } + for _, rule := range rules { + if rule.UID == q.UID { + q.Result = rule + break } } - return nil } @@ -132,14 +115,9 @@ func (f *FakeRuleStore) GetAlertRulesForScheduling(_ context.Context, q *models. if err := f.Hook(*q); err != nil { return err } - for _, rg := range f.Rules { - for _, n := range rg { - for _, r := range n { - q.Result = append(q.Result, r...) - } - } + for _, rules := range f.Rules { + q.Result = append(q.Result, rules...) } - return nil } @@ -148,19 +126,11 @@ func (f *FakeRuleStore) GetOrgAlertRules(_ context.Context, q *models.ListAlertR defer f.mtx.Unlock() f.RecordedOps = append(f.RecordedOps, *q) - if _, ok := f.Rules[q.OrgID]; !ok { + rules, ok := f.Rules[q.OrgID] + if !ok { return nil } - - var rules []*models.AlertRule - for ruleGroup := range f.Rules[q.OrgID] { - for _, storedRules := range f.Rules[q.OrgID][ruleGroup] { - rules = append(rules, storedRules...) - } - } - q.Result = rules - return nil } func (f *FakeRuleStore) GetNamespaceAlertRules(_ context.Context, q *models.ListNamespaceAlertRulesQuery) error { @@ -169,36 +139,28 @@ func (f *FakeRuleStore) GetNamespaceAlertRules(_ context.Context, q *models.List f.RecordedOps = append(f.RecordedOps, *q) return nil } -func (f *FakeRuleStore) GetRuleGroupAlertRules(_ context.Context, q *models.ListRuleGroupAlertRulesQuery) error { +func (f *FakeRuleStore) GetAlertRules(_ context.Context, q *models.GetAlertRulesQuery) error { f.mtx.Lock() defer f.mtx.Unlock() f.RecordedOps = append(f.RecordedOps, *q) if err := f.Hook(*q); err != nil { return err } - rgs, ok := f.Rules[q.OrgID] + rules, ok := f.Rules[q.OrgID] if !ok { return nil } - - rg, ok := rgs[q.RuleGroup] - if !ok { - return nil - } - - if q.NamespaceUID != "" { - r, ok := rg[q.NamespaceUID] - if !ok { - return nil + var result []*models.AlertRule + for _, rule := range rules { + if q.NamespaceUID != rule.NamespaceUID { + continue } - q.Result = r - return nil + if q.RuleGroup != nil && *q.RuleGroup != rule.RuleGroup { + continue + } + result = append(result, rule) } - - for _, r := range rg { - q.Result = append(q.Result, r...) - } - + q.Result = result return nil } func (f *FakeRuleStore) GetNamespaces(_ context.Context, orgID int64, _ *models2.SignedInUser) (map[string]*models2.Folder, error) { @@ -212,12 +174,9 @@ func (f *FakeRuleStore) GetNamespaces(_ context.Context, orgID int64, _ *models2 return namespacesMap, nil } - for rg := range f.Rules[orgID] { - for namespace := range f.Rules[orgID][rg] { - namespacesMap[namespace] = &models2.Folder{} - } + for _, rule := range f.Rules[orgID] { + namespacesMap[rule.NamespaceUID] = &models2.Folder{} } - return namespacesMap, nil } func (f *FakeRuleStore) GetNamespaceByTitle(_ context.Context, _ string, _ int64, _ *models2.SignedInUser, _ bool) (*models2.Folder, error) { @@ -233,18 +192,16 @@ func (f *FakeRuleStore) GetOrgRuleGroups(_ context.Context, q *models.ListOrgRul // If we have namespaces, we want to try and retrieve the list of rules stored. if len(q.NamespaceUIDs) != 0 { - _, ok := f.Rules[q.OrgID] + rules, ok := f.Rules[q.OrgID] if !ok { return nil } var ruleGroups [][]string - for rg := range f.Rules[q.OrgID] { - for storedNamespace := range f.Rules[q.OrgID][rg] { - for _, namespace := range q.NamespaceUIDs { - if storedNamespace == namespace { // if they match, they should go in. - ruleGroups = append(ruleGroups, []string{rg, storedNamespace, storedNamespace}) - } + for _, rule := range rules { + for _, namespace := range q.NamespaceUIDs { + if rule.NamespaceUID == namespace { // if they match, they should go in. + ruleGroups = append(ruleGroups, []string{rule.RuleGroup, rule.NamespaceUID, rule.NamespaceUID}) } } } @@ -263,6 +220,7 @@ func (f *FakeRuleStore) UpsertAlertRules(_ context.Context, q []UpsertRule) erro } return nil } + func (f *FakeRuleStore) UpdateRuleGroup(_ context.Context, cmd UpdateRuleGroupCmd) error { f.mtx.Lock() defer f.mtx.Unlock() @@ -270,29 +228,15 @@ func (f *FakeRuleStore) UpdateRuleGroup(_ context.Context, cmd UpdateRuleGroupCm if err := f.Hook(cmd); err != nil { return err } - rgs, ok := f.Rules[cmd.OrgID] - if !ok { - f.Rules[cmd.OrgID] = map[string]map[string][]*models.AlertRule{} - } + existingRules := f.Rules[cmd.OrgID] - rg, ok := rgs[cmd.RuleGroupConfig.Name] - if !ok { - f.Rules[cmd.OrgID][cmd.RuleGroupConfig.Name] = map[string][]*models.AlertRule{} - } - - _, ok = rg[cmd.NamespaceUID] - if !ok { - f.Rules[cmd.OrgID][cmd.RuleGroupConfig.Name][cmd.NamespaceUID] = []*models.AlertRule{} - } - - rules := []*models.AlertRule{} for _, r := range cmd.RuleGroupConfig.Rules { // TODO: Not sure why this is not being set properly, where is the code that sets this? for i := range r.GrafanaManagedAlert.Data { r.GrafanaManagedAlert.Data[i].DatasourceUID = "-100" } - new := &models.AlertRule{ + newRule := &models.AlertRule{ OrgID: cmd.OrgID, Title: r.GrafanaManagedAlert.Title, Condition: r.GrafanaManagedAlert.Condition, @@ -307,26 +251,26 @@ func (f *FakeRuleStore) UpdateRuleGroup(_ context.Context, cmd UpdateRuleGroupCm } if r.ApiRuleNode != nil { - new.For = time.Duration(r.ApiRuleNode.For) - new.Annotations = r.ApiRuleNode.Annotations - new.Labels = r.ApiRuleNode.Labels + newRule.For = time.Duration(r.ApiRuleNode.For) + newRule.Annotations = r.ApiRuleNode.Annotations + newRule.Labels = r.ApiRuleNode.Labels } - if new.NoDataState == "" { - new.NoDataState = models.NoData + if newRule.NoDataState == "" { + newRule.NoDataState = models.NoData } - if new.ExecErrState == "" { - new.ExecErrState = models.AlertingErrState + if newRule.ExecErrState == "" { + newRule.ExecErrState = models.AlertingErrState } - err := new.PreSave(time.Now) + err := newRule.PreSave(time.Now) require.NoError(f.t, err) - rules = append(rules, new) + existingRules = append(existingRules, newRule) } - f.Rules[cmd.OrgID][cmd.RuleGroupConfig.Name][cmd.NamespaceUID] = rules + f.Rules[cmd.OrgID] = existingRules return nil } diff --git a/pkg/services/ngalert/tests/util.go b/pkg/services/ngalert/tests/util.go index 98d2f4eef78..81de0498c50 100644 --- a/pkg/services/ngalert/tests/util.go +++ b/pkg/services/ngalert/tests/util.go @@ -109,12 +109,12 @@ func CreateTestAlertRuleWithLabels(t *testing.T, ctx context.Context, dbstore *s }) require.NoError(t, err) - q := models.ListRuleGroupAlertRulesQuery{ + q := models.GetAlertRulesQuery{ OrgID: orgID, NamespaceUID: "namespace", - RuleGroup: ruleGroup, + RuleGroup: &ruleGroup, } - err = dbstore.GetRuleGroupAlertRules(ctx, &q) + err = dbstore.GetAlertRules(ctx, &q) require.NoError(t, err) require.NotEmpty(t, q.Result)