diff --git a/go.mod b/go.mod index 555bbc18a0..6b9d0ecf16 100644 --- a/go.mod +++ b/go.mod @@ -285,7 +285,7 @@ require ( ) // Using a fork of Prometheus with Mimir-specific changes. -replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241210170917-0a0a41616520 +replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241217171657-e97bb2a6aa36 // Replace memberlist with our fork which includes some fixes that haven't been // merged upstream yet: diff --git a/go.sum b/go.sum index 44c2383392..d2e63ab2d1 100644 --- a/go.sum +++ b/go.sum @@ -1279,8 +1279,8 @@ github.com/grafana/gomemcache v0.0.0-20241016125027-0a5bcc5aef40 h1:1TeKhyS+pvzO github.com/grafana/gomemcache v0.0.0-20241016125027-0a5bcc5aef40/go.mod h1:IGRj8oOoxwJbHBYl1+OhS9UjQR0dv6SQOep7HqmtyFU= github.com/grafana/memberlist v0.3.1-0.20220714140823-09ffed8adbbe h1:yIXAAbLswn7VNWBIvM71O2QsgfgW9fRXZNR0DXe6pDU= github.com/grafana/memberlist v0.3.1-0.20220714140823-09ffed8adbbe/go.mod h1:MS2lj3INKhZjWNqd3N0m3J+Jxf3DAOnAH9VT3Sh9MUE= -github.com/grafana/mimir-prometheus v0.0.0-20241210170917-0a0a41616520 h1:FADazl5oVYBARbfVMtLkPQ9IfIwhiE9lrPrKNPOHBV4= -github.com/grafana/mimir-prometheus v0.0.0-20241210170917-0a0a41616520/go.mod h1:NpYc1U0eC7m6xUh3t3Pq565KxaIc08Oaquiu71dEMi8= +github.com/grafana/mimir-prometheus v0.0.0-20241217171657-e97bb2a6aa36 h1:6eBPqfxqNlhRn/PQED6F/1qn2b4oWl2nItGPhIdYaeM= +github.com/grafana/mimir-prometheus v0.0.0-20241217171657-e97bb2a6aa36/go.mod h1:NpYc1U0eC7m6xUh3t3Pq565KxaIc08Oaquiu71dEMi8= github.com/grafana/opentracing-contrib-go-stdlib v0.0.0-20230509071955-f410e79da956 h1:em1oddjXL8c1tL0iFdtVtPloq2hRPen2MJQKoAWpxu0= github.com/grafana/opentracing-contrib-go-stdlib v0.0.0-20230509071955-f410e79da956/go.mod h1:qtI1ogk+2JhVPIXVc6q+NHziSmy2W5GbdQZFUHADCBU= github.com/grafana/prometheus-alertmanager v0.25.1-0.20240930132144-b5e64e81e8d3 h1:6D2gGAwyQBElSrp3E+9lSr7k8gLuP3Aiy20rweLWeBw= diff --git a/pkg/ruler/manager_metrics.go b/pkg/ruler/manager_metrics.go index 9f5a3953fe..784b07340f 100644 --- a/pkg/ruler/manager_metrics.go +++ b/pkg/ruler/manager_metrics.go @@ -25,6 +25,7 @@ type ManagerMetrics struct { GroupInterval *prometheus.Desc GroupLastEvalTime *prometheus.Desc GroupLastDuration *prometheus.Desc + GroupLastRuleDurationSum *prometheus.Desc GroupLastRestoreDuration *prometheus.Desc GroupRules *prometheus.Desc GroupLastEvalSamples *prometheus.Desc @@ -89,6 +90,12 @@ func NewManagerMetrics(logger log.Logger) *ManagerMetrics { []string{"user", "rule_group"}, nil, ), + GroupLastRuleDurationSum: prometheus.NewDesc( + "cortex_prometheus_rule_group_last_rule_duration_sum_seconds", + "The sum of time in seconds it took to evaluate each rule in the group regardless of concurrency. This should be higher than the group duration if rules are evaluated concurrently.", + []string{"user", "rule_group"}, + nil, + ), GroupLastRestoreDuration: prometheus.NewDesc( "cortex_prometheus_rule_group_last_restore_duration_seconds", "The duration of the last alert rules alerts restoration using the `ALERTS_FOR_STATE` series across all rule groups.", @@ -131,6 +138,7 @@ func (m *ManagerMetrics) Describe(out chan<- *prometheus.Desc) { out <- m.GroupInterval out <- m.GroupLastEvalTime out <- m.GroupLastDuration + out <- m.GroupLastRuleDurationSum out <- m.GroupLastRestoreDuration out <- m.GroupRules out <- m.GroupLastEvalSamples @@ -156,6 +164,7 @@ func (m *ManagerMetrics) Collect(out chan<- prometheus.Metric) { data.SendSumOfGaugesPerTenant(out, m.GroupInterval, "prometheus_rule_group_interval_seconds", dskit_metrics.WithLabels("rule_group")) data.SendSumOfGaugesPerTenant(out, m.GroupLastEvalTime, "prometheus_rule_group_last_evaluation_timestamp_seconds", dskit_metrics.WithLabels("rule_group")) data.SendSumOfGaugesPerTenant(out, m.GroupLastDuration, "prometheus_rule_group_last_duration_seconds", dskit_metrics.WithLabels("rule_group")) + data.SendSumOfGaugesPerTenant(out, m.GroupLastRuleDurationSum, "prometheus_rule_group_last_rule_duration_sum_seconds", dskit_metrics.WithLabels("rule_group")) data.SendSumOfGaugesPerTenant(out, m.GroupRules, "prometheus_rule_group_rules", dskit_metrics.WithLabels("rule_group")) data.SendSumOfGaugesPerTenant(out, m.GroupLastEvalSamples, "prometheus_rule_group_last_evaluation_samples", dskit_metrics.WithLabels("rule_group")) } diff --git a/pkg/ruler/rule_concurrency.go b/pkg/ruler/rule_concurrency.go index 964ea9a326..ba4a506c2a 100644 --- a/pkg/ruler/rule_concurrency.go +++ b/pkg/ruler/rule_concurrency.go @@ -190,9 +190,19 @@ func (c *TenantConcurrencyController) Allow(_ context.Context, group *rules.Grou // isGroupAtRisk checks if the rule group's last evaluation time is within the risk threshold. func (c *TenantConcurrencyController) isGroupAtRisk(group *rules.Group) bool { interval := group.Interval().Seconds() - lastEvaluation := group.GetEvaluationTime().Seconds() + runtimeThreshold := interval * c.thresholdRuleConcurrency / 100 - return lastEvaluation >= interval*c.thresholdRuleConcurrency/100 + // If the group evaluation time is greater than the threshold, the group is at risk. + if group.GetEvaluationTime().Seconds() >= runtimeThreshold { + return true + } + + // If the total rule evaluation time is greater than the threshold, the group is at risk. + if group.GetRuleEvaluationTimeSum().Seconds() >= runtimeThreshold { + return true + } + + return false } // isRuleIndependent checks if the rule is independent of other rules. diff --git a/pkg/ruler/rule_concurrency_test.go b/pkg/ruler/rule_concurrency_test.go index ac953d2c4a..64be33715c 100644 --- a/pkg/ruler/rule_concurrency_test.go +++ b/pkg/ruler/rule_concurrency_test.go @@ -5,6 +5,7 @@ package ruler import ( "bytes" "context" + "fmt" "testing" "time" @@ -12,8 +13,10 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/testutil" "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/promql" "github.com/prometheus/prometheus/promql/parser" "github.com/prometheus/prometheus/rules" + "github.com/prometheus/prometheus/util/teststorage" "github.com/stretchr/testify/require" "go.uber.org/atomic" "golang.org/x/sync/semaphore" @@ -264,11 +267,51 @@ func TestIsRuleIndependent(t *testing.T) { } func TestGroupAtRisk(t *testing.T) { - exp, err := parser.ParseExpr("vector(1)") - require.NoError(t, err) - rule1 := rules.NewRecordingRule("test", exp, labels.Labels{}) - rule1.SetNoDependencyRules(true) - rule1.SetNoDependentRules(true) + createAndEvalTestGroup := func(interval time.Duration, evalConcurrently bool) *rules.Group { + st := teststorage.New(t) + defer st.Close() + + // Create 100 rules that all take 1ms to evaluate. + var createdRules []rules.Rule + ruleCt := 100 + ruleWaitTime := 1 * time.Millisecond + for i := 0; i < ruleCt; i++ { + q, err := parser.ParseExpr("vector(1)") + require.NoError(t, err) + rule := rules.NewRecordingRule(fmt.Sprintf("test_rule%d", i), q, labels.Labels{}) + rule.SetNoDependencyRules(true) + rule.SetNoDependentRules(true) + createdRules = append(createdRules, rule) + } + + // Create the group and evaluate it + opts := rules.GroupOptions{ + Interval: interval, + Opts: &rules.ManagerOptions{ + Appendable: st, + QueryFunc: func(_ context.Context, _ string, _ time.Time) (promql.Vector, error) { + time.Sleep(ruleWaitTime) + return promql.Vector{}, nil + }, + }, + Rules: createdRules, + } + if evalConcurrently { + opts.Opts.RuleConcurrencyController = &allowAllConcurrencyController{} + } + g := rules.NewGroup(opts) + rules.DefaultEvalIterationFunc(context.Background(), g, time.Now()) + + // Sanity check that we're actually running the rules concurrently. + // The group should take less time than the sum of all rules if we're running them concurrently, more otherwise. + if evalConcurrently { + require.Less(t, g.GetEvaluationTime(), time.Duration(ruleCt)*ruleWaitTime) + } else { + require.Greater(t, g.GetEvaluationTime(), time.Duration(ruleCt)*ruleWaitTime) + } + + return g + } m := newMultiTenantConcurrencyControllerMetrics(prometheus.NewPedanticRegistry()) controller := &TenantConcurrencyController{ @@ -284,44 +327,48 @@ func TestGroupAtRisk(t *testing.T) { } tc := map[string]struct { - group *rules.Group - expected bool + groupInterval time.Duration + evalConcurrently bool + expected bool }{ "group last evaluation greater than interval": { - group: func() *rules.Group { - g := rules.NewGroup(rules.GroupOptions{ - Interval: -1 * time.Minute, - Opts: &rules.ManagerOptions{}, - }) - return g - }(), - expected: true, + // Total runtime: 100x1ms ~ 100ms (run sequentially), > 1ms -> Not at risk + groupInterval: 1 * time.Millisecond, + evalConcurrently: false, + expected: true, }, "group last evaluation less than interval": { - group: func() *rules.Group { - g := rules.NewGroup(rules.GroupOptions{ - Interval: 1 * time.Minute, - Opts: &rules.ManagerOptions{}, - }) - return g - }(), - expected: false, + // Total runtime: 100x1ms ~ 100ms (run sequentially), < 1s -> Not at risk + groupInterval: 1 * time.Second, + evalConcurrently: false, + expected: false, }, - "group last evaluation exactly at concurrency trigger threshold": { - group: func() *rules.Group { - g := rules.NewGroup(rules.GroupOptions{ - Interval: 0 * time.Minute, - Opts: &rules.ManagerOptions{}, - }) - return g - }(), - expected: true, + "group total rule evaluation duration of last evaluation greater than threshold": { + // Total runtime: 100x1ms ~ 100ms, > 50ms -> Group isn't at risk for its runtime, but it is for the sum of all rules. + groupInterval: 50 * time.Millisecond, + evalConcurrently: true, + expected: true, + }, + "group total rule evaluation duration of last evaluation less than threshold": { + // Total runtime: 100x1ms ~ 100ms, < 1s -> Not at risk + groupInterval: 1 * time.Second, + evalConcurrently: true, + expected: false, }, } for name, tt := range tc { t.Run(name, func(t *testing.T) { - require.Equal(t, tt.expected, controller.isGroupAtRisk(tt.group)) + group := createAndEvalTestGroup(tt.groupInterval, tt.evalConcurrently) + require.Equal(t, tt.expected, controller.isGroupAtRisk(group)) }) } } + +type allowAllConcurrencyController struct{} + +func (a *allowAllConcurrencyController) Allow(_ context.Context, _ *rules.Group, _ rules.Rule) bool { + return true +} + +func (a *allowAllConcurrencyController) Done(_ context.Context) {} diff --git a/vendor/github.com/prometheus/prometheus/rules/group.go b/vendor/github.com/prometheus/prometheus/rules/group.go index b6feb6f962..8ad8958f8d 100644 --- a/vendor/github.com/prometheus/prometheus/rules/group.go +++ b/vendor/github.com/prometheus/prometheus/rules/group.go @@ -44,20 +44,21 @@ import ( // Group is a set of rules that have a logical relation. type Group struct { - name string - file string - interval time.Duration - queryOffset *time.Duration - limit int - rules []Rule - sourceTenants []string - seriesInPreviousEval []map[string]labels.Labels // One per Rule. - staleSeries []labels.Labels - opts *ManagerOptions - mtx sync.Mutex - evaluationTime time.Duration - lastEvaluation time.Time // Wall-clock time of most recent evaluation. - lastEvalTimestamp time.Time // Time slot used for most recent evaluation. + name string + file string + interval time.Duration + queryOffset *time.Duration + limit int + rules []Rule + sourceTenants []string + seriesInPreviousEval []map[string]labels.Labels // One per Rule. + staleSeries []labels.Labels + opts *ManagerOptions + mtx sync.Mutex + evaluationTime time.Duration // Time it took to evaluate the group. + evaluationRuleTimeSum time.Duration // Sum of time it took to evaluate each rule in the group. + lastEvaluation time.Time // Wall-clock time of most recent evaluation. + lastEvalTimestamp time.Time // Time slot used for most recent evaluation. shouldRestore bool @@ -119,6 +120,7 @@ func NewGroup(o GroupOptions) *Group { metrics.EvalFailures.WithLabelValues(key) metrics.GroupLastEvalTime.WithLabelValues(key) metrics.GroupLastDuration.WithLabelValues(key) + metrics.GroupLastRuleDurationSum.WithLabelValues(key) metrics.GroupRules.WithLabelValues(key).Set(float64(len(o.Rules))) metrics.GroupSamples.WithLabelValues(key) metrics.GroupInterval.WithLabelValues(key).Set(o.Interval.Seconds()) @@ -380,6 +382,28 @@ func (g *Group) setEvaluationTime(dur time.Duration) { g.evaluationTime = dur } +// GetRuleEvaluationTimeSum returns the sum of the time it took to evaluate each rule in the group irrespective of concurrency. +func (g *Group) GetRuleEvaluationTimeSum() time.Duration { + g.mtx.Lock() + defer g.mtx.Unlock() + return g.evaluationRuleTimeSum +} + +// updateRuleEvaluationTimeSum updates evaluationRuleTimeSum which is the sum of the time it took to evaluate each rule in the group irrespective of concurrency. +// It collects the times from the rules themselves. +func (g *Group) updateRuleEvaluationTimeSum() { + var sum time.Duration + for _, rule := range g.rules { + sum += rule.GetEvaluationDuration() + } + + g.metrics.GroupLastRuleDurationSum.WithLabelValues(GroupKey(g.file, g.name)).Set(sum.Seconds()) + + g.mtx.Lock() + defer g.mtx.Unlock() + g.evaluationRuleTimeSum = sum +} + // GetLastEvaluation returns the time the last evaluation of the rule group took place. func (g *Group) GetLastEvaluation() time.Time { g.mtx.Lock() @@ -916,6 +940,7 @@ type Metrics struct { GroupInterval *prometheus.GaugeVec GroupLastEvalTime *prometheus.GaugeVec GroupLastDuration *prometheus.GaugeVec + GroupLastRuleDurationSum *prometheus.GaugeVec GroupLastRestoreDuration *prometheus.GaugeVec GroupRules *prometheus.GaugeVec GroupSamples *prometheus.GaugeVec @@ -994,6 +1019,14 @@ func NewGroupMetrics(reg prometheus.Registerer) *Metrics { }, []string{"rule_group"}, ), + GroupLastRuleDurationSum: prometheus.NewGaugeVec( + prometheus.GaugeOpts{ + Namespace: namespace, + Name: "rule_group_last_rule_duration_sum_seconds", + Help: "The sum of time in seconds it took to evaluate each rule in the group regardless of concurrency. This should be higher than the group duration if rules are evaluated concurrently.", + }, + []string{"rule_group"}, + ), GroupLastRestoreDuration: prometheus.NewGaugeVec( prometheus.GaugeOpts{ Namespace: namespace, @@ -1031,6 +1064,7 @@ func NewGroupMetrics(reg prometheus.Registerer) *Metrics { m.GroupInterval, m.GroupLastEvalTime, m.GroupLastDuration, + m.GroupLastRuleDurationSum, m.GroupLastRestoreDuration, m.GroupRules, m.GroupSamples, diff --git a/vendor/github.com/prometheus/prometheus/rules/manager.go b/vendor/github.com/prometheus/prometheus/rules/manager.go index b5bb015116..97fe4ec75e 100644 --- a/vendor/github.com/prometheus/prometheus/rules/manager.go +++ b/vendor/github.com/prometheus/prometheus/rules/manager.go @@ -82,6 +82,7 @@ func DefaultEvalIterationFunc(ctx context.Context, g *Group, evalTimestamp time. timeSinceStart := time.Since(start) g.metrics.IterationDuration.Observe(timeSinceStart.Seconds()) + g.updateRuleEvaluationTimeSum() g.setEvaluationTime(timeSinceStart) g.setLastEvaluation(start) g.setLastEvalTimestamp(evalTimestamp) diff --git a/vendor/github.com/prometheus/prometheus/tsdb/querier.go b/vendor/github.com/prometheus/prometheus/tsdb/querier.go index 7f4c4317f2..0d2f1ddcd8 100644 --- a/vendor/github.com/prometheus/prometheus/tsdb/querier.go +++ b/vendor/github.com/prometheus/prometheus/tsdb/querier.go @@ -270,7 +270,7 @@ func PostingsForMatchers(ctx context.Context, ix IndexPostingsReader, ms ...*lab its = append(its, it) case m.Type == labels.MatchNotRegexp && m.Value == ".+": // .+ regexp matches any non-empty string: get postings for all label values and remove them. - its = append(notIts, ix.PostingsForAllLabelValues(ctx, m.Name)) + notIts = append(notIts, ix.PostingsForAllLabelValues(ctx, m.Name)) case labelMustBeSet[m.Name]: // If this matcher must be non-empty, we can be smarter. diff --git a/vendor/modules.txt b/vendor/modules.txt index 20a6a3279f..c7096737ce 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1017,7 +1017,7 @@ github.com/prometheus/exporter-toolkit/web github.com/prometheus/procfs github.com/prometheus/procfs/internal/fs github.com/prometheus/procfs/internal/util -# github.com/prometheus/prometheus v1.99.0 => github.com/grafana/mimir-prometheus v0.0.0-20241210170917-0a0a41616520 +# github.com/prometheus/prometheus v1.99.0 => github.com/grafana/mimir-prometheus v0.0.0-20241217171657-e97bb2a6aa36 ## explicit; go 1.22.0 github.com/prometheus/prometheus/config github.com/prometheus/prometheus/discovery @@ -1688,7 +1688,7 @@ sigs.k8s.io/kustomize/kyaml/yaml/walk sigs.k8s.io/yaml sigs.k8s.io/yaml/goyaml.v2 sigs.k8s.io/yaml/goyaml.v3 -# github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241210170917-0a0a41616520 +# github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241217171657-e97bb2a6aa36 # github.com/hashicorp/memberlist => github.com/grafana/memberlist v0.3.1-0.20220714140823-09ffed8adbbe # gopkg.in/yaml.v3 => github.com/colega/go-yaml-yaml v0.0.0-20220720105220-255a8d16d094 # github.com/grafana/regexp => github.com/grafana/regexp v0.0.0-20240531075221-3685f1377d7b