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

@@ -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")
+	alertRuleQuery := ngmodels.ListAlertRulesQuery{
-	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{
 		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)
 }
 

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()))
 	})
 }
 

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
-	})
-}

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] {
+	for _, folder := range f.Folders[orgID] {
-		namespacesMap[rule.NamespaceUID] = &models2.Folder{}
+		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()