From 1a27ab29b8a33592e50daef4c59f6fc7b9a5b1f9 Mon Sep 17 00:00:00 2001 From: Julien Duchesne Date: Mon, 6 Jan 2025 15:48:38 -0500 Subject: [PATCH] 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 * Store a slice of Rule instead Signed-off-by: Julien Duchesne * Add `BenchmarkRuleDependencyController_AnalyseRules` for future reference Signed-off-by: Julien Duchesne --------- Signed-off-by: Julien Duchesne --- rules/alerting.go | 53 ++++++++++++++++++++++++++++++++-------- rules/alerting_test.go | 20 ++++++++++----- rules/group.go | 26 +++++++++----------- rules/manager.go | 8 +++--- rules/manager_test.go | 35 +++++++++++++++++++++----- rules/origin_test.go | 14 ++++++----- rules/recording.go | 54 +++++++++++++++++++++++++++++++++-------- rules/recording_test.go | 20 ++++++++++----- rules/rule.go | 14 ++++++++--- 9 files changed, 178 insertions(+), 66 deletions(-) diff --git a/rules/alerting.go b/rules/alerting.go index e7f15baefe..77d53395e0 100644 --- 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) 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 diff --git a/rules/alerting_test.go b/rules/alerting_test.go index f0aa339cc7..f7bdf4a955 100644 --- 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 index cabb45abbb..724b926d4f 100644 --- a/rules/group.go +++ b/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. diff --git a/rules/manager.go b/rules/manager.go index 390742ce50..c4c0f8a1ef 100644 --- a/rules/manager.go +++ b/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)) } } diff --git a/rules/manager_test.go b/rules/manager_test.go index df6f5fd1b4..defa93a68c 100644 --- a/rules/manager_test.go +++ b/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) + } + } +} diff --git a/rules/origin_test.go b/rules/origin_test.go index 0bf428f3c1..b38f5d99b2 100644 --- 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) diff --git a/rules/recording.go b/rules/recording.go index 52c2a875ab..3b6db210af 100644 --- a/rules/recording.go +++ b/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 } diff --git a/rules/recording_test.go b/rules/recording_test.go index 72c0764f9b..3fbf11c435 100644 --- a/rules/recording_test.go +++ b/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()) } diff --git a/rules/rule.go b/rules/rule.go index 687c03d000..33f1755ac5 100644 --- a/rules/rule.go +++ b/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 }