Rules: Store dependencies instead of boolean (#15689)

* Rules: Store dependencies instead of boolean
To improve https://github.com/prometheus/prometheus/pull/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 <julien.duchesne@grafana.com>

* Store a slice of Rule instead

Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>

* Add `BenchmarkRuleDependencyController_AnalyseRules` for future reference

Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>

---------

Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>

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) SetNoDependencyRules(noDependencyRules bool) {
-	r.noDependencyRules.Store(noDependencyRules)
+func (r *AlertingRule) DependentRules() []Rule {
+	r.dependenciesMutex.RLock()
+	defer r.dependenciesMutex.RUnlock()
+	return r.dependentRules
+}
+
+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

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

rules/group.go

@@ -1034,27 +1034,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
@@ -1064,7 +1062,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.

rules/manager.go

@@ -444,8 +444,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)
 }
 
@@ -460,8 +460,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))
 	}
 }
 

rules/manager_test.go

@@ -1423,8 +1423,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{
@@ -1613,11 +1611,12 @@ 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))
@@ -1625,7 +1624,7 @@ func TestDependencyMap(t *testing.T) {
 	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))
 }
 
@@ -1958,7 +1957,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))
 	}
 }
@@ -2508,3 +2508,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)
+		}
+	}
+}

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)

rules/recording.go

@@ -18,6 +18,7 @@ import (
 	"errors"
 	"fmt"
 	"net/url"
+	"sync"
 	"time"
 
 	"go.uber.org/atomic"
@@ -43,8 +44,9 @@ type RecordingRule struct {
 	// Duration of how long it took to evaluate the recording rule.
 	evaluationDuration *atomic.Duration
 
-	noDependentRules  *atomic.Bool
-	noDependencyRules *atomic.Bool
+	dependenciesMutex sync.RWMutex
+	dependentRules    []Rule
+	dependencyRules   []Rule
 }
 
 // NewRecordingRule returns a new recording rule.
@@ -57,8 +59,6 @@ func NewRecordingRule(name string, vector parser.Expr, lset labels.Labels) *Reco
 		evaluationTimestamp: atomic.NewTime(time.Time{}),
 		evaluationDuration:  atomic.NewDuration(0),
 		lastError:           atomic.NewError(nil),
-		noDependentRules:    atomic.NewBool(false),
-		noDependencyRules:   atomic.NewBool(false),
 	}
 }
 
@@ -172,18 +172,52 @@ func (rule *RecordingRule) GetEvaluationTimestamp() time.Time {
 	return rule.evaluationTimestamp.Load()
 }
 
-func (rule *RecordingRule) SetNoDependentRules(noDependentRules bool) {
-	rule.noDependentRules.Store(noDependentRules)
+func (rule *RecordingRule) SetDependentRules(dependents []Rule) {
+	rule.dependenciesMutex.Lock()
+	defer rule.dependenciesMutex.Unlock()
+
+	rule.dependentRules = make([]Rule, len(dependents))
+	copy(rule.dependentRules, dependents)
 }
 
 func (rule *RecordingRule) NoDependentRules() bool {
-	return rule.noDependentRules.Load()
+	rule.dependenciesMutex.RLock()
+	defer rule.dependenciesMutex.RUnlock()
+
+	if rule.dependentRules == nil {
+		return false // We don't know if there are dependent rules.
+	}
+
+	return len(rule.dependentRules) == 0
 }
 
-func (rule *RecordingRule) SetNoDependencyRules(noDependencyRules bool) {
-	rule.noDependencyRules.Store(noDependencyRules)
+func (rule *RecordingRule) DependentRules() []Rule {
+	rule.dependenciesMutex.RLock()
+	defer rule.dependenciesMutex.RUnlock()
+	return rule.dependentRules
+}
+
+func (rule *RecordingRule) SetDependencyRules(dependencies []Rule) {
+	rule.dependenciesMutex.Lock()
+	defer rule.dependenciesMutex.Unlock()
+
+	rule.dependencyRules = make([]Rule, len(dependencies))
+	copy(rule.dependencyRules, dependencies)
 }
 
 func (rule *RecordingRule) NoDependencyRules() bool {
-	return rule.noDependencyRules.Load()
+	rule.dependenciesMutex.RLock()
+	defer rule.dependenciesMutex.RUnlock()
+
+	if rule.dependencyRules == nil {
+		return false // We don't know if there are dependency rules.
+	}
+
+	return len(rule.dependencyRules) == 0
+}
+
+func (rule *RecordingRule) DependencyRules() []Rule {
+	rule.dependenciesMutex.RLock()
+	defer rule.dependenciesMutex.RUnlock()
+	return rule.dependencyRules
 }

rules/recording_test.go

@@ -255,24 +255,32 @@ func TestRecordingEvalWithOrigin(t *testing.T) {
 	require.Equal(t, detail, NewRuleDetail(rule))
 }
 
-func TestRecordingRule_SetNoDependentRules(t *testing.T) {
+func TestRecordingRule_SetDependentRules(t *testing.T) {
+	dependentRule := NewRecordingRule("test1", nil, labels.EmptyLabels())
+
 	rule := NewRecordingRule("1", &parser.NumberLiteral{Val: 1}, labels.EmptyLabels())
 	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 TestRecordingRule_SetNoDependencyRules(t *testing.T) {
+func TestRecordingRule_SetDependencyRules(t *testing.T) {
+	dependencyRule := NewRecordingRule("test1", nil, labels.EmptyLabels())
+
 	rule := NewRecordingRule("1", &parser.NumberLiteral{Val: 1}, labels.EmptyLabels())
 	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())
 }

rules/rule.go

@@ -62,19 +62,25 @@ type Rule interface {
 	// NOTE: Used dynamically by rules.html template.
 	GetEvaluationTimestamp() time.Time
 
-	// SetNoDependentRules sets whether there's no other rule in the rule group that depends on this rule.
-	SetNoDependentRules(bool)
+	// SetDependentRules sets rules which depend on the output of this rule.
+	SetDependentRules(rules []Rule)
 
 	// NoDependentRules returns true if it's guaranteed that in the rule group there's no other rule
 	// which depends on this one. In case this function returns false there's no such guarantee, which
 	// means there may or may not be other rules depending on this one.
 	NoDependentRules() bool
 
-	// SetNoDependencyRules sets whether this rule doesn't depend on the output of any rule in the rule group.
-	SetNoDependencyRules(bool)
+	// DependentRules returns the rules which depend on the output of this rule.
+	DependentRules() []Rule
+
+	// SetDependencyRules sets rules on which this rule depends.
+	SetDependencyRules(rules []Rule)
 
 	// NoDependencyRules returns true if it's guaranteed that this rule doesn't depend on the output of
 	// any other rule in the group. In case this function returns false there's no such guarantee, which
 	// means the rule may or may not depend on other rules.
 	NoDependencyRules() bool
+
+	// DependencyRules returns the rules on which this rule depends.
+	DependencyRules() []Rule
 }