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 }