From 48519f9ebb60b69e5c2ac7feb2ab2bc6c750f564 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Mon, 11 Apr 2022 10:54:29 -0400 Subject: [PATCH] Alerting: reduce database calls in prometheus-comptible rules API (#47080) * move validation at the beginning of method * remove usage of GetOrgRuleGroups because it is not necessary. All information is already available in memory. * remove unused method --- pkg/services/ngalert/api/api_prometheus.go | 70 +++++++------------ .../ngalert/api/api_prometheus_test.go | 27 +++---- pkg/services/ngalert/store/alert_rule.go | 50 +------------ pkg/services/ngalert/store/testing.go | 32 +-------- 4 files changed, 45 insertions(+), 134 deletions(-) diff --git a/pkg/services/ngalert/api/api_prometheus.go b/pkg/services/ngalert/api/api_prometheus.go index 37b84ac21bb..31398c304c8 100644 --- a/pkg/services/ngalert/api/api_prometheus.go +++ b/pkg/services/ngalert/api/api_prometheus.go @@ -101,6 +101,15 @@ func getPanelIDFromRequest(r *http.Request) (int64, error) { } func (srv PrometheusSrv) RouteGetRuleStatuses(c *models.ReqContext) response.Response { + dashboardUID := c.Query("dashboard_uid") + panelID, err := getPanelIDFromRequest(c.Req) + if err != nil { + return ErrResp(http.StatusBadRequest, err, "invalid panel_id") + } + if dashboardUID == "" && panelID != 0 { + return ErrResp(http.StatusBadRequest, errors.New("panel_id must be set with dashboard_uid"), "") + } + ruleResponse := apimodels.RuleResponse{ DiscoveryBase: apimodels.DiscoveryBase{ Status: "success", @@ -130,33 +139,12 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *models.ReqContext) response.Res namespaceUIDs = append(namespaceUIDs, k) } - dashboardUID := c.Query("dashboard_uid") - panelID, err := getPanelIDFromRequest(c.Req) - if err != nil { - return ErrResp(http.StatusBadRequest, err, "invalid panel_id") - } - if dashboardUID == "" && panelID != 0 { - return ErrResp(http.StatusBadRequest, errors.New("panel_id must be set with dashboard_uid"), "") - } - - ruleGroupQuery := ngmodels.ListOrgRuleGroupsQuery{ + alertRuleQuery := ngmodels.ListAlertRulesQuery{ OrgID: c.SignedInUser.OrgId, NamespaceUIDs: namespaceUIDs, DashboardUID: dashboardUID, PanelID: panelID, } - if err := srv.store.GetOrgRuleGroups(c.Req.Context(), &ruleGroupQuery); err != nil { - ruleResponse.DiscoveryBase.Status = "error" - ruleResponse.DiscoveryBase.Error = fmt.Sprintf("failure getting rule groups: %s", err.Error()) - ruleResponse.DiscoveryBase.ErrorType = apiv1.ErrServer - return response.JSON(http.StatusInternalServerError, ruleResponse) - } - - alertRuleQuery := ngmodels.ListAlertRulesQuery{ - OrgID: c.SignedInUser.OrgId, - DashboardUID: dashboardUID, - PanelID: panelID, - } if err := srv.store.GetOrgAlertRules(c.Req.Context(), &alertRuleQuery); err != nil { ruleResponse.DiscoveryBase.Status = "error" ruleResponse.DiscoveryBase.Error = fmt.Sprintf("failure getting rules: %s", err.Error()) @@ -166,29 +154,23 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *models.ReqContext) response.Res groupMap := make(map[string]*apimodels.RuleGroup) - for _, r := range ruleGroupQuery.Result { - if len(r) < 3 { - continue - } - groupId, namespaceUID, namespace := r[0], r[1], r[2] - - newGroup := &apimodels.RuleGroup{ - Name: groupId, - File: namespace, // file is what Prometheus uses for provisioning, we replace it with namespace. - LastEvaluation: time.Time{}, - EvaluationTime: 0, // TODO: see if we are able to pass this along with evaluation results - } - - groupMap[groupId+"-"+namespaceUID] = newGroup - - ruleResponse.Data.RuleGroups = append(ruleResponse.Data.RuleGroups, newGroup) - } - for _, rule := range alertRuleQuery.Result { - newGroup, ok := groupMap[rule.RuleGroup+"-"+rule.NamespaceUID] + groupKey := rule.RuleGroup + "-" + rule.NamespaceUID + newGroup, ok := groupMap[groupKey] if !ok { - continue + folder := namespaceMap[rule.NamespaceUID] + if folder == nil { + srv.log.Warn("query returned rules that belong to folder the user does not have access to. The rule will not be added to the response", "folder_uid", rule.NamespaceUID, "rule_uid", rule.UID) + continue + } + newGroup = &apimodels.RuleGroup{ + Name: rule.RuleGroup, + File: folder.Title, // file is what Prometheus uses for provisioning, we replace it with namespace. + } + groupMap[groupKey] = newGroup + ruleResponse.Data.RuleGroups = append(ruleResponse.Data.RuleGroups, newGroup) } + alertingRule := apimodels.AlertingRule{ State: "inactive", Name: rule.Title, @@ -222,7 +204,6 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *models.ReqContext) response.Res if alertState.LastEvaluationTime.After(newRule.LastEvaluation) { newRule.LastEvaluation = alertState.LastEvaluationTime - newGroup.LastEvaluation = alertState.LastEvaluationTime } newRule.EvaluationTime = alertState.EvaluationDuration.Seconds() @@ -252,7 +233,10 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *models.ReqContext) response.Res alertingRule.Rule = newRule newGroup.Rules = append(newGroup.Rules, alertingRule) newGroup.Interval = float64(rule.IntervalSeconds) + newGroup.EvaluationTime = newRule.EvaluationTime + newGroup.LastEvaluation = newRule.LastEvaluation } + return response.JSON(http.StatusOK, ruleResponse) } diff --git a/pkg/services/ngalert/api/api_prometheus_test.go b/pkg/services/ngalert/api/api_prometheus_test.go index 74ec3dc9809..af3a8f5cd48 100644 --- a/pkg/services/ngalert/api/api_prometheus_test.go +++ b/pkg/services/ngalert/api/api_prometheus_test.go @@ -270,16 +270,17 @@ func TestRouteGetRuleStatuses(t *testing.T) { t.Run("with a rule that only has one query", func(t *testing.T) { fakeStore, fakeAIM, api := setupAPI(t) generateRuleAndInstanceWithQuery(t, orgID, fakeAIM, fakeStore, withClassicConditionSingleQuery()) + folder := fakeStore.Folders[orgID][0] r := api.RouteGetRuleStatuses(c) require.Equal(t, http.StatusOK, r.Status()) - require.JSONEq(t, ` + require.JSONEq(t, fmt.Sprintf(` { "status": "success", "data": { "groups": [{ "name": "rule-group", - "file": "namespaceUID", + "file": "%s", "rules": [{ "state": "inactive", "name": "AlwaysFiring", @@ -306,16 +307,17 @@ func TestRouteGetRuleStatuses(t *testing.T) { }], "interval": 60, "lastEvaluation": "2022-03-10T14:01:00Z", - "evaluationTime": 0 + "evaluationTime": 60 }] } } -`, string(r.Body())) +`, folder.Title), string(r.Body())) }) t.Run("with the inclusion of internal Labels", func(t *testing.T) { fakeStore, fakeAIM, api := setupAPI(t) generateRuleAndInstanceWithQuery(t, orgID, fakeAIM, fakeStore, withClassicConditionSingleQuery()) + folder := fakeStore.Folders[orgID][0] req, err := http.NewRequest("GET", "/api/v1/rules?includeInternalLabels=true", nil) require.NoError(t, err) @@ -323,13 +325,13 @@ func TestRouteGetRuleStatuses(t *testing.T) { r := api.RouteGetRuleStatuses(c) require.Equal(t, http.StatusOK, r.Status()) - require.JSONEq(t, ` + require.JSONEq(t, fmt.Sprintf(` { "status": "success", "data": { "groups": [{ "name": "rule-group", - "file": "namespaceUID", + "file": "%s", "rules": [{ "state": "inactive", "name": "AlwaysFiring", @@ -359,26 +361,27 @@ func TestRouteGetRuleStatuses(t *testing.T) { }], "interval": 60, "lastEvaluation": "2022-03-10T14:01:00Z", - "evaluationTime": 0 + "evaluationTime": 60 }] } } -`, string(r.Body())) +`, folder.Title), string(r.Body())) }) t.Run("with a rule that has multiple queries", func(t *testing.T) { fakeStore, fakeAIM, api := setupAPI(t) generateRuleAndInstanceWithQuery(t, orgID, fakeAIM, fakeStore, withExpressionsMultiQuery()) + folder := fakeStore.Folders[orgID][0] r := api.RouteGetRuleStatuses(c) require.Equal(t, http.StatusOK, r.Status()) - require.JSONEq(t, ` + require.JSONEq(t, fmt.Sprintf(` { "status": "success", "data": { "groups": [{ "name": "rule-group", - "file": "namespaceUID", + "file": "%s", "rules": [{ "state": "inactive", "name": "AlwaysFiring", @@ -405,11 +408,11 @@ func TestRouteGetRuleStatuses(t *testing.T) { }], "interval": 60, "lastEvaluation": "2022-03-10T14:01:00Z", - "evaluationTime": 0 + "evaluationTime": 60 }] } } -`, string(r.Body())) +`, folder.Title), string(r.Body())) }) } diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index 06beea3a1d2..b4e3b5e8ed0 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -32,7 +32,7 @@ type UpsertRule struct { New ngmodels.AlertRule } -// Store is the interface for persisting alert rules and instances +// RuleStore is the interface for persisting alert rules and instances type RuleStore interface { DeleteAlertRulesByUID(ctx context.Context, orgID int64, ruleUID ...string) error DeleteAlertInstancesByRuleUID(ctx context.Context, orgID int64, ruleUID string) error @@ -43,7 +43,6 @@ type RuleStore interface { GetAlertRules(ctx context.Context, query *ngmodels.GetAlertRulesQuery) error GetUserVisibleNamespaces(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 UpsertAlertRules(ctx context.Context, rule []UpsertRule) error } @@ -405,50 +404,3 @@ func (st DBstore) validateAlertRule(alertRule ngmodels.AlertRule) error { return nil } - -func (st DBstore) GetOrgRuleGroups(ctx context.Context, query *ngmodels.ListOrgRuleGroupsQuery) error { - return st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { - var ruleGroups [][]string - q := ` -SELECT DISTINCT - rule_group, - namespace_uid, - ( - SELECT title - FROM dashboard - WHERE - org_id = alert_rule.org_id AND - uid = alert_rule.namespace_uid - ) AS namespace_title -FROM alert_rule -WHERE org_id = ?` - params := []interface{}{query.OrgID} - - if len(query.NamespaceUIDs) > 0 { - placeholders := make([]string, 0, len(query.NamespaceUIDs)) - for _, folderUID := range query.NamespaceUIDs { - params = append(params, folderUID) - placeholders = append(placeholders, "?") - } - q = fmt.Sprintf(" %s AND namespace_uid IN (%s)", q, strings.Join(placeholders, ",")) - } - - if query.DashboardUID != "" { - q = fmt.Sprintf("%s and dashboard_uid = ?", q) - params = append(params, query.DashboardUID) - if query.PanelID != 0 { - q = fmt.Sprintf("%s and panel_id = ?", q) - params = append(params, query.PanelID) - } - } - - q = fmt.Sprintf(" %s ORDER BY namespace_title", q) - - if err := sess.SQL(q, params...).Find(&ruleGroups); err != nil { - return err - } - - query.Result = ruleGroups - return nil - }) -} diff --git a/pkg/services/ngalert/store/testing.go b/pkg/services/ngalert/store/testing.go index 527626c3e98..783d06e3ee8 100644 --- a/pkg/services/ngalert/store/testing.go +++ b/pkg/services/ngalert/store/testing.go @@ -219,8 +219,8 @@ func (f *FakeRuleStore) GetUserVisibleNamespaces(_ context.Context, orgID int64, return namespacesMap, nil } - for _, rule := range f.Rules[orgID] { - namespacesMap[rule.NamespaceUID] = &models2.Folder{} + for _, folder := range f.Folders[orgID] { + namespacesMap[folder.Uid] = folder } return namespacesMap, nil } @@ -233,34 +233,6 @@ func (f *FakeRuleStore) GetNamespaceByTitle(_ context.Context, title string, org } return nil, fmt.Errorf("not found") } -func (f *FakeRuleStore) GetOrgRuleGroups(_ context.Context, q *models.ListOrgRuleGroupsQuery) error { - f.mtx.Lock() - defer f.mtx.Unlock() - f.RecordedOps = append(f.RecordedOps, *q) - if err := f.Hook(*q); err != nil { - return err - } - - // If we have namespaces, we want to try and retrieve the list of rules stored. - if len(q.NamespaceUIDs) != 0 { - rules, ok := f.Rules[q.OrgID] - if !ok { - return nil - } - - var ruleGroups [][]string - 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}) - } - } - } - - q.Result = ruleGroups - } - return nil -} func (f *FakeRuleStore) UpsertAlertRules(_ context.Context, q []UpsertRule) error { f.mtx.Lock()