Rules: Store dependencies instead of boolean (#15689)

* Rules: Store dependencies instead of boolean To improve prometheus/prometheus#15681 further, we'll need to store the dependencies and dependents of each Right now, if a rule has both (at least 1) dependents and dependencies, it is not possible to determine the order to run the rules and they must all run sequentially This PR only changes the dependents and dependencies attributes of rules, it does not implement a new topological sort algorithm Signed-off-by: Julien Duchesne <[email protected]> * Store a slice of Rule instead Signed-off-by: Julien Duchesne <[email protected]> * Add `BenchmarkRuleDependencyController_AnalyseRules` for future reference Signed-off-by: Julien Duchesne <[email protected]> --------- Signed-off-by: Julien Duchesne <[email protected]>
grafana · Jan 6, 2025 · 3db5cc9 · 3db5cc9
1 parent abe4acf
commit 3db5cc9
Show file tree

Hide file tree

Showing 9 changed files with 178 additions and 66 deletions.
diff --git a/rules/alerting.go b/rules/alerting.go
@@ -143,8 +143,9 @@ type AlertingRule struct {
 
 	logger *slog.Logger
 
-	noDependentRules  *atomic.Bool
-	noDependencyRules *atomic.Bool
+	dependenciesMutex sync.RWMutex
+	dependentRules    []Rule
+	dependencyRules   []Rule
 }
 
 // NewAlertingRule constructs a new AlertingRule.
@@ -171,8 +172,6 @@ func NewAlertingRule(
 		evaluationTimestamp: atomic.NewTime(time.Time{}),
 		evaluationDuration:  atomic.NewDuration(0),
 		lastError:           atomic.NewError(nil),
-		noDependentRules:    atomic.NewBool(false),
-		noDependencyRules:   atomic.NewBool(false),
 	}
 }
 
@@ -316,20 +315,54 @@ func (r *AlertingRule) Restored() bool {
 	return r.restored.Load()
 }
 
-func (r *AlertingRule) SetNoDependentRules(noDependentRules bool) {
-	r.noDependentRules.Store(noDependentRules)
+func (r *AlertingRule) SetDependentRules(dependents []Rule) {
+	r.dependenciesMutex.Lock()
+	defer r.dependenciesMutex.Unlock()
+
+	r.dependentRules = make([]Rule, len(dependents))
+	copy(r.dependentRules, dependents)
 }
 
 func (r *AlertingRule) NoDependentRules() bool {
-	return r.noDependentRules.Load()
+	r.dependenciesMutex.RLock()
+	defer r.dependenciesMutex.RUnlock()
+
+	if r.dependentRules == nil {
+		return false // We don't know if there are dependent rules.
+	}
+
+	return len(r.dependentRules) == 0
+}
+
+func (r *AlertingRule) DependentRules() []Rule {
+	r.dependenciesMutex.RLock()
+	defer r.dependenciesMutex.RUnlock()
+	return r.dependentRules
 }
 
-func (r *AlertingRule) SetNoDependencyRules(noDependencyRules bool) {
-	r.noDependencyRules.Store(noDependencyRules)
+func (r *AlertingRule) SetDependencyRules(dependencies []Rule) {
+	r.dependenciesMutex.Lock()
+	defer r.dependenciesMutex.Unlock()
+
+	r.dependencyRules = make([]Rule, len(dependencies))
+	copy(r.dependencyRules, dependencies)
 }
 
 func (r *AlertingRule) NoDependencyRules() bool {
-	return r.noDependencyRules.Load()
+	r.dependenciesMutex.RLock()
+	defer r.dependenciesMutex.RUnlock()
+
+	if r.dependencyRules == nil {
+		return false // We don't know if there are dependency rules.
+	}
+
+	return len(r.dependencyRules) == 0
+}
+
+func (r *AlertingRule) DependencyRules() []Rule {
+	r.dependenciesMutex.RLock()
+	defer r.dependenciesMutex.RUnlock()
+	return r.dependencyRules
 }
 
 // resolvedRetention is the duration for which a resolved alert instance

diff --git a/rules/alerting_test.go b/rules/alerting_test.go
@@ -998,7 +998,9 @@ func TestAlertingEvalWithOrigin(t *testing.T) {
 	require.Equal(t, detail, NewRuleDetail(rule))
 }
 
-func TestAlertingRule_SetNoDependentRules(t *testing.T) {
+func TestAlertingRule_SetDependentRules(t *testing.T) {
+	dependentRule := NewRecordingRule("test1", nil, labels.EmptyLabels())
+
 	rule := NewAlertingRule(
 		"test",
 		&parser.NumberLiteral{Val: 1},
@@ -1012,14 +1014,18 @@ func TestAlertingRule_SetNoDependentRules(t *testing.T) {
 	)
 	require.False(t, rule.NoDependentRules())
 
-	rule.SetNoDependentRules(false)
+	rule.SetDependentRules([]Rule{dependentRule})
 	require.False(t, rule.NoDependentRules())
+	require.Equal(t, []Rule{dependentRule}, rule.DependentRules())
 
-	rule.SetNoDependentRules(true)
+	rule.SetDependentRules([]Rule{})
 	require.True(t, rule.NoDependentRules())
+	require.Empty(t, rule.DependentRules())
 }
 
-func TestAlertingRule_SetNoDependencyRules(t *testing.T) {
+func TestAlertingRule_SetDependencyRules(t *testing.T) {
+	dependencyRule := NewRecordingRule("test1", nil, labels.EmptyLabels())
+
 	rule := NewAlertingRule(
 		"test",
 		&parser.NumberLiteral{Val: 1},
@@ -1033,11 +1039,13 @@ func TestAlertingRule_SetNoDependencyRules(t *testing.T) {
 	)
 	require.False(t, rule.NoDependencyRules())
 
-	rule.SetNoDependencyRules(false)
+	rule.SetDependencyRules([]Rule{dependencyRule})
 	require.False(t, rule.NoDependencyRules())
+	require.Equal(t, []Rule{dependencyRule}, rule.DependencyRules())
 
-	rule.SetNoDependencyRules(true)
+	rule.SetDependencyRules([]Rule{})
 	require.True(t, rule.NoDependencyRules())
+	require.Empty(t, rule.DependencyRules())
 }
 
 func TestAlertingRule_ActiveAlertsCount(t *testing.T) {

diff --git a/rules/group.go b/rules/group.go
@@ -1073,27 +1073,25 @@ func NewGroupMetrics(reg prometheus.Registerer) *Metrics {
 // output metric produced by another rule in its expression (i.e. as its "input").
 type dependencyMap map[Rule][]Rule
 
-// dependents returns the count of rules which use the output of the given rule as one of their inputs.
-func (m dependencyMap) dependents(r Rule) int {
-	return len(m[r])
+// dependents returns the rules which use the output of the given rule as one of their inputs.
+func (m dependencyMap) dependents(r Rule) []Rule {
+	return m[r]
 }
 
-// dependencies returns the count of rules on which the given rule is dependent for input.
-func (m dependencyMap) dependencies(r Rule) int {
+// dependencies returns the rules on which the given rule is dependent for input.
+func (m dependencyMap) dependencies(r Rule) []Rule {
 	if len(m) == 0 {
-		return 0
+		return []Rule{}
 	}
 
-	var count int
-	for _, children := range m {
-		for _, child := range children {
-			if child == r {
-				count++
-			}
+	var dependencies []Rule
+	for rule, dependents := range m {
+		if slices.Contains(dependents, r) {
+			dependencies = append(dependencies, rule)
 		}
 	}
 
-	return count
+	return dependencies
 }
 
 // isIndependent determines whether the given rule is not dependent on another rule for its input, nor is any other rule
@@ -1103,7 +1101,7 @@ func (m dependencyMap) isIndependent(r Rule) bool {
 		return false
 	}
 
-	return m.dependents(r)+m.dependencies(r) == 0
+	return len(m.dependents(r)) == 0 && len(m.dependencies(r)) == 0
 }
 
 // buildDependencyMap builds a data-structure which contains the relationships between rules within a group.

diff --git a/rules/manager.go b/rules/manager.go
@@ -473,8 +473,8 @@ func SendAlerts(s Sender, externalURL string) NotifyFunc {
 // RuleDependencyController controls whether a set of rules have dependencies between each other.
 type RuleDependencyController interface {
 	// AnalyseRules analyses dependencies between the input rules. For each rule that it's guaranteed
-	// not having any dependants and/or dependency, this function should call Rule.SetNoDependentRules(true)
-	// and/or Rule.SetNoDependencyRules(true).
+	// not having any dependants and/or dependency, this function should call Rule.SetDependentRules(...)
+	// and/or Rule.SetDependencyRules(...).
 	AnalyseRules(rules []Rule)
 }
 
@@ -489,8 +489,8 @@ func (c ruleDependencyController) AnalyseRules(rules []Rule) {
 	}
 
 	for _, r := range rules {
-		r.SetNoDependentRules(depMap.dependents(r) == 0)
-		r.SetNoDependencyRules(depMap.dependencies(r) == 0)
+		r.SetDependentRules(depMap.dependents(r))
+		r.SetDependencyRules(depMap.dependencies(r))
 	}
 }
 

diff --git a/rules/manager_test.go b/rules/manager_test.go
@@ -1660,8 +1660,6 @@ func TestRuleGroupEvalIterationFunc(t *testing.T) {
 			evaluationTimestamp: atomic.NewTime(time.Time{}),
 			evaluationDuration:  atomic.NewDuration(0),
 			lastError:           atomic.NewError(nil),
-			noDependentRules:    atomic.NewBool(false),
-			noDependencyRules:   atomic.NewBool(false),
 		}
 
 		group := NewGroup(GroupOptions{
@@ -1850,19 +1848,20 @@ func TestDependencyMap(t *testing.T) {
 	depMap := buildDependencyMap(group.rules)
 
 	require.Zero(t, depMap.dependencies(rule))
-	require.Equal(t, 2, depMap.dependents(rule))
+	require.Equal(t, []Rule{rule2, rule4}, depMap.dependents(rule))
+	require.Len(t, depMap.dependents(rule), 2)
 	require.False(t, depMap.isIndependent(rule))
 
 	require.Zero(t, depMap.dependents(rule2))
-	require.Equal(t, 1, depMap.dependencies(rule2))
+	require.Equal(t, []Rule{rule}, depMap.dependencies(rule2))
 	require.False(t, depMap.isIndependent(rule2))
 
 	require.Zero(t, depMap.dependents(rule3))
 	require.Zero(t, depMap.dependencies(rule3))
 	require.True(t, depMap.isIndependent(rule3))
 
 	require.Zero(t, depMap.dependents(rule4))
-	require.Equal(t, 1, depMap.dependencies(rule4))
+	require.Equal(t, []Rule{rule}, depMap.dependencies(rule4))
 	require.False(t, depMap.isIndependent(rule4))
 }
 
@@ -2195,7 +2194,8 @@ func TestDependencyMapUpdatesOnGroupUpdate(t *testing.T) {
 		require.NotEqual(t, orig[h], depMap)
 		// We expect there to be some dependencies since the new rule group contains a dependency.
 		require.NotEmpty(t, depMap)
-		require.Equal(t, 1, depMap.dependents(rr))
+		require.Len(t, depMap.dependents(rr), 1)
+		require.Equal(t, "HighRequestRate", depMap.dependents(rr)[0].Name())
 		require.Zero(t, depMap.dependencies(rr))
 	}
 }
@@ -2780,3 +2780,26 @@ func TestRuleDependencyController_AnalyseRules(t *testing.T) {
 		})
 	}
 }
+
+func BenchmarkRuleDependencyController_AnalyseRules(b *testing.B) {
+	storage := teststorage.New(b)
+	b.Cleanup(func() { storage.Close() })
+
+	ruleManager := NewManager(&ManagerOptions{
+		Context:    context.Background(),
+		Logger:     promslog.NewNopLogger(),
+		Appendable: storage,
+		QueryFunc:  func(ctx context.Context, q string, ts time.Time) (promql.Vector, error) { return nil, nil },
+	})
+
+	groups, errs := ruleManager.LoadGroups(time.Second, labels.EmptyLabels(), "", nil, "fixtures/rules_multiple.yaml")
+	require.Empty(b, errs)
+	require.Len(b, groups, 1)
+
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for _, g := range groups {
+			ruleManager.opts.RuleDependencyController.AnalyseRules(g.rules)
+		}
+	}
+}
diff --git a/rules/origin_test.go b/rules/origin_test.go
@@ -45,10 +45,12 @@ func (u unknownRule) SetEvaluationDuration(time.Duration)  {}
 func (u unknownRule) GetEvaluationDuration() time.Duration { return 0 }
 func (u unknownRule) SetEvaluationTimestamp(time.Time)     {}
 func (u unknownRule) GetEvaluationTimestamp() time.Time    { return time.Time{} }
-func (u unknownRule) SetNoDependentRules(bool)             {}
+func (u unknownRule) SetDependentRules([]Rule)             {}
 func (u unknownRule) NoDependentRules() bool               { return false }
-func (u unknownRule) SetNoDependencyRules(bool)            {}
+func (u unknownRule) DependentRules() []Rule               { return nil }
+func (u unknownRule) SetDependencyRules([]Rule)            {}
 func (u unknownRule) NoDependencyRules() bool              { return false }
+func (u unknownRule) DependencyRules() []Rule              { return nil }
 
 func TestNewRuleDetailPanics(t *testing.T) {
 	require.PanicsWithValue(t, `unknown rule type "rules.unknownRule"`, func() {
@@ -76,12 +78,12 @@ func TestNewRuleDetail(t *testing.T) {
 		require.False(t, detail.NoDependentRules)
 		require.False(t, detail.NoDependencyRules)
 
-		rule.SetNoDependentRules(true)
+		rule.SetDependentRules([]Rule{})
 		detail = NewRuleDetail(rule)
 		require.True(t, detail.NoDependentRules)
 		require.False(t, detail.NoDependencyRules)
 
-		rule.SetNoDependencyRules(true)
+		rule.SetDependencyRules([]Rule{})
 		detail = NewRuleDetail(rule)
 		require.True(t, detail.NoDependentRules)
 		require.True(t, detail.NoDependencyRules)
@@ -104,12 +106,12 @@ func TestNewRuleDetail(t *testing.T) {
 		require.False(t, detail.NoDependentRules)
 		require.False(t, detail.NoDependencyRules)
 
-		rule.SetNoDependentRules(true)
+		rule.SetDependentRules([]Rule{})
 		detail = NewRuleDetail(rule)
 		require.True(t, detail.NoDependentRules)
 		require.False(t, detail.NoDependencyRules)
 
-		rule.SetNoDependencyRules(true)
+		rule.SetDependencyRules([]Rule{})
 		detail = NewRuleDetail(rule)
 		require.True(t, detail.NoDependentRules)
 		require.True(t, detail.NoDependencyRules)