Add support for exclude_alerts flag in ListRules API #6011

rajagopalanand · 2024-06-12T20:03:22Z

Which issue(s) this PR fixes:
Fixes #5989

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

rapphil · 2024-06-12T20:39:40Z

no unit tests?
The title of the PR is not helpful. It should indicate what the PR is doing instead of a link to a issue. I can find the link to the issue in the body of the PR if I needed.

yeya24

+1 to @rapphil.
And let's add a changelog entry.

friedrichg

@rajagopalanand still needs the unit test update right?

rajagopalanand · 2024-06-13T12:38:09Z

Will add tests

yeya24 · 2024-06-18T21:44:34Z

pkg/ruler/api.go

@@ -135,11 +135,18 @@ func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) {
 		return
 	}

+	excludeAlerts, err := parseExcludeAlerts(req)
+	if err != nil {


Do we need to include the error in the response? Can we align with Prometheus API response?

yeya24

Thanks!

yeya24 · 2024-06-18T23:18:07Z

I think the test is flaky. Can we fix them?

--- FAIL: TestGetRules (10.23s)
    --- FAIL: TestGetRules/No_Sharding_with_Rule_Type_Filter (0.24s)
        ruler_test.go:362: 
            	Error Trace:	/__w/cortex/cortex/pkg/ruler/ruler_test.go:362
            	            				/__w/cortex/cortex/pkg/ruler/ruler_test.go:922
            	            				/__w/cortex/cortex/pkg/ruler/ruler_test.go:872
            	            				/__w/cortex/cortex/pkg/ruler/ruler_test.go:904
            	Error:      	"0" is not greater than or equal to "1"
            	Test:       	TestGetRules/No_Sharding_with_Rule_Type_Filter

rajagopalanand · 2024-06-19T13:59:08Z

Previously the Ruler tests did not look for alerts firing. With this new filter, the tests look for active alerts and this requires extra time for the Ruler to load the rules and evaluate the rule groups at least twice. In unit tests, there is not an obvious way to know if a Ruler has started evaluating rule groups. Integration tests call the metrics endpoint to know if a particular rule group has been evaluated. I could cast the manager interface to the concrete DefaultMultiTenantManager and access user registries. For now, I added some extra sleep time to allow Rulers to evaluate the rule groups

rajagopalanand · 2024-06-19T16:20:42Z

In addition, TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy is failing every once in a while. Here is an example of a test failure. I ran the tests with some print statements locally and with enough # of runs, the error occurred on my local machine

instance-64 zone-1 zone-2 instance-44
    --- FAIL: TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy/num_instances_=_60,_num_zones_=_3,_stable_sharding_=_false (0.03s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x1bd7f83]

goroutine 69 [running]:
testing.tRunner.func1.2({0x1d9bfa0, 0x2d156f0})
	/usr/local/Cellar/go/1.21.3/libexec/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
	/usr/local/Cellar/go/1.21.3/libexec/src/testing/testing.go:1548 +0x397
panic({0x1d9bfa0?, 0x2d156f0?})
	/usr/local/Cellar/go/1.21.3/libexec/src/runtime/panic.go:914 +0x21f
github.com/cortexproject/cortex/pkg/ring.(*MinimizeSpreadTokenGenerator).GenerateTokens(0xc00059ca90, 0xc0001ef160, {0xc0004f4090, 0xb}, {0xc0004f40a0, 0x6}, 0x80, 0x1)
	/Users/anrajag/workplace/rajagopalanand-cortex/pkg/ring/token_generator.go:134 +0x883
github.com/cortexproject/cortex/pkg/ring.TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy.func1(0xc000503040)
	/Users/anrajag/workplace/rajagopalanand-cortex/pkg/ring/ring_test.go:2497 +0xce6
testing.tRunner(0xc000503040, 0xc00070ec60)
	/usr/local/Cellar/go/1.21.3/libexec/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 11
	/usr/local/Cellar/go/1.21.3/libexec/src/testing/testing.go:1648 +0x3ad

The above seg fault lines #s do not match because I added some log statements. Not sure if this a problem with the test or if there is an edge case bug in MinimizeSpreadTokenGenerator.GenerateTokens

rapphil · 2024-06-20T20:20:39Z

pkg/ruler/ruler_test.go

+					ruler := rulerAddrMap[rID]
+					if tc.sharding {
+						subRing := ruler.ring.ShuffleShard(user, tc.shuffleShardSize)
+						owns, err := instanceOwnsRuleGroup(subRing, rgDesc, validation.DisabledRuleGroups{}, ruler.lifecycler.GetInstanceAddr(), false)


it feels that during error the waitUntil should also error instead of continuing the test. There are no disabled rule groups so you can only get a real error.

rapphil · 2024-06-20T20:22:05Z

pkg/ruler/ruler_test.go

+				}
+				if len(m) == 0 {
+					close(doneCh)
+					return


this should be an exceptional case to have m without elements no? should this error instead of just continue silently?

rapphil · 2024-06-20T20:25:24Z

pkg/ruler/ruler_test.go

+					for metric := range scrapeCh {
+						collected := &dto.Metric{}
+						_ = metric.Write(collected)
+						userLabel := ""
+						ruleGroupName := ""
+						for _, lp := range collected.GetLabel() {
+							if lp.GetName() == "user" && lp.GetValue() == user {
+								userLabel = lp.GetValue()
+							}
+							if lp.GetName() == "rule_group" {
+								ruleGroupName = lp.GetValue()
+							}
+						}
+						if userLabel != "" && ruleGroupName != "" {
+							for k := range m {
+								if strings.Contains(ruleGroupName, k) {
+									m[k] = m[k] + collected.Counter.GetValue()
+								}
+							}
+						}
+						done := true
+						for k := range m {
+							if m[k] < 3 {
+								done = false
+								break
+							}
+						}
+						if done {
+							return
+						}
+					}


you can use labels to simplify the code a bit.

Suggested change

for metric := range scrapeCh {

collected := &dto.Metric{}

_ = metric.Write(collected)

userLabel := ""

ruleGroupName := ""

for _, lp := range collected.GetLabel() {

if lp.GetName() == "user" && lp.GetValue() == user {

userLabel = lp.GetValue()

}

if lp.GetName() == "rule_group" {

ruleGroupName = lp.GetValue()

}

}

if userLabel != "" && ruleGroupName != "" {

for k := range m {

if strings.Contains(ruleGroupName, k) {

m[k] = m[k] + collected.Counter.GetValue()

}

}

}

done := true

for k := range m {

if m[k] < 3 {

done = false

break

}

}

if done {

return

}

}

outerLoop:

for metric := range scrapeCh {

collected := &dto.Metric{}

_ = metric.Write(collected)

userLabel := ""

ruleGroupName := ""

for _, lp := range collected.GetLabel() {

if lp.GetName() == "user" && lp.GetValue() == user {

userLabel = lp.GetValue()

}

if lp.GetName() == "rule_group" {

ruleGroupName = lp.GetValue()

}

}

if userLabel != "" && ruleGroupName != "" {

for k := range m {

if strings.Contains(ruleGroupName, k) {

m[k] = m[k] + collected.Counter.GetValue()

}

}

}

for k := range m {

if m[k] < 3 {

continue outerLoop

}

}

}

pkg/ruler/ruler_test.go

yeya24 · 2024-06-24T16:47:22Z

pkg/ruler/ruler_test.go

@@ -844,9 +899,90 @@ func TestGetRules(t *testing.T) {
 				time.Sleep(100 * time.Millisecond)
 			}

+			scrape := func(rID string, scrapeCh chan prometheus.Metric, doneCh chan struct{}) {


If unit test is hard to add let's just add it to the integration test?

Yes, I will try and add an integration test

friedrichg

It seems the testing is generating the most comments. I second the idea of having an integration test, that should settle the feature IMHO

friedrichg · 2024-06-27T10:02:59Z

Makefile

@@ -236,7 +236,7 @@ shell:
 	bash

 configs-integration-test:
-	/bin/bash -c "go test -v -tags 'netgo integration' -timeout 30s ./pkg/configs/... ./pkg/ruler/..."
+	/bin/bash -c "go test -v -tags 'netgo integration' -timeout 60s ./pkg/configs/... ./pkg/ruler/..."


Never a good sign... but ok

Like I mentioned in the earlier comment, the tests did not check for active alerts and so the tests can immediately call GetRules. Rules need to be evaluated for alerts to be present and it takes extra time to evaluate the rules. configs-integration-test are also calling the ruler tests which takes extra time to evaluate the rules. Basically it is a cascading effect. Even if we remove the sleep, which I did in the recent version, some of the tests are going to take longer time for alerts to be populated

I will look at this again later in the week and see if there is a way to better organize the test or create a new test but even in this case, the tests would need to run longer for alerts to be populated

yeya24 · 2024-07-16T17:35:58Z

It would be great to revisit this PR and get it merged

rajagopalanand · 2024-07-16T19:08:35Z

Yes, will address it this week

rajagopalanand · 2024-07-30T16:22:33Z

Added integ tests and removed unit tests

rapphil · 2024-07-30T16:51:57Z

Makefile

@@ -10,7 +10,7 @@ VERSION=$(shell cat "./VERSION" 2> /dev/null)
 GOPROXY_VALUE=$(shell go env GOPROXY)

 # ARCHS
-ARCHS       = amd64 arm64
+ARCHS       = amd64 #arm64


do we need to put this back?

Signed-off-by: Anand Rajagopal <anrajag@amazon.com>

alanprot · 2024-07-30T17:49:13Z

LGTM!

pull-request-size bot added the size/M label Jun 12, 2024

rajagopalanand force-pushed the list-rules-bug-fix branch from 909ccb9 to d4dc41a Compare June 12, 2024 20:13

yeya24 approved these changes Jun 12, 2024

View reviewed changes

rajagopalanand changed the title ~~Fix for https://github.com/cortexproject/cortex/issues/5989~~ Add support for exclude_alerts flag in ListRules API Jun 12, 2024

yeya24 self-requested a review June 13, 2024 00:03

friedrichg reviewed Jun 13, 2024

View reviewed changes

rajagopalanand force-pushed the list-rules-bug-fix branch from d4dc41a to 81601e9 Compare June 18, 2024 20:57

pull-request-size bot added size/L and removed size/M labels Jun 18, 2024

rajagopalanand force-pushed the list-rules-bug-fix branch 3 times, most recently from 84bd9f0 to add416e Compare June 18, 2024 21:51

yeya24 reviewed Jun 18, 2024

View reviewed changes

rajagopalanand force-pushed the list-rules-bug-fix branch from add416e to cbc6585 Compare June 18, 2024 22:53

yeya24 approved these changes Jun 18, 2024

View reviewed changes

rajagopalanand force-pushed the list-rules-bug-fix branch 3 times, most recently from ab03cc6 to 8a65ab1 Compare June 19, 2024 13:52

rajagopalanand force-pushed the list-rules-bug-fix branch 7 times, most recently from 9103987 to 0c61cfb Compare June 20, 2024 03:31

rajagopalanand force-pushed the list-rules-bug-fix branch 3 times, most recently from 519ba00 to ecf2d95 Compare June 20, 2024 18:54

rapphil reviewed Jun 20, 2024

View reviewed changes

rajagopalanand force-pushed the list-rules-bug-fix branch from ecf2d95 to 16b5c0d Compare June 20, 2024 21:59

emanlodovice reviewed Jun 20, 2024

View reviewed changes

pkg/ruler/ruler_test.go Outdated Show resolved Hide resolved

yeya24 reviewed Jun 24, 2024

View reviewed changes

alanprot requested a review from rapphil June 26, 2024 23:07

friedrichg reviewed Jun 27, 2024

View reviewed changes

rajagopalanand force-pushed the list-rules-bug-fix branch 2 times, most recently from 713b06b to 3771a51 Compare July 30, 2024 15:51

rajagopalanand force-pushed the list-rules-bug-fix branch from 3771a51 to 172eea5 Compare July 30, 2024 16:27

pull-request-size bot added size/M and removed size/L labels Jul 30, 2024

rapphil reviewed Jul 30, 2024

View reviewed changes

fix for cortexproject#5989

0cd075a

Signed-off-by: Anand Rajagopal <anrajag@amazon.com>

rajagopalanand force-pushed the list-rules-bug-fix branch from 172eea5 to 0cd075a Compare July 30, 2024 16:57

rapphil approved these changes Jul 30, 2024

View reviewed changes

alanprot approved these changes Jul 30, 2024

View reviewed changes

yeya24 merged commit 8ce3642 into cortexproject:master Jul 30, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for exclude_alerts flag in ListRules API #6011

Add support for exclude_alerts flag in ListRules API #6011

rajagopalanand commented Jun 12, 2024 •

edited

Loading

rapphil commented Jun 12, 2024

yeya24 left a comment

friedrichg left a comment

rajagopalanand commented Jun 13, 2024 •

edited

Loading

yeya24 Jun 18, 2024

yeya24 left a comment

yeya24 commented Jun 18, 2024

rajagopalanand commented Jun 19, 2024 •

edited

Loading

rajagopalanand commented Jun 19, 2024 •

edited

Loading

rapphil Jun 20, 2024

rapphil Jun 20, 2024

rapphil Jun 20, 2024

yeya24 Jun 24, 2024

rajagopalanand Jun 27, 2024 •

edited

Loading

friedrichg left a comment

friedrichg Jun 27, 2024

rajagopalanand Jun 27, 2024

rajagopalanand Jun 27, 2024

yeya24 commented Jul 16, 2024

rajagopalanand commented Jul 16, 2024

rajagopalanand commented Jul 30, 2024

rapphil Jul 30, 2024

rajagopalanand Jul 30, 2024

alanprot commented Jul 30, 2024

Add support for exclude_alerts flag in ListRules API #6011

Add support for exclude_alerts flag in ListRules API #6011

Conversation

rajagopalanand commented Jun 12, 2024 • edited Loading

rapphil commented Jun 12, 2024

yeya24 left a comment

Choose a reason for hiding this comment

friedrichg left a comment

Choose a reason for hiding this comment

rajagopalanand commented Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

yeya24 commented Jun 18, 2024

rajagopalanand commented Jun 19, 2024 • edited Loading

rajagopalanand commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajagopalanand Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

friedrichg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 commented Jul 16, 2024

rajagopalanand commented Jul 16, 2024

rajagopalanand commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanprot commented Jul 30, 2024

rajagopalanand commented Jun 12, 2024 •

edited

Loading

rajagopalanand commented Jun 13, 2024 •

edited

Loading

rajagopalanand commented Jun 19, 2024 •

edited

Loading

rajagopalanand commented Jun 19, 2024 •

edited

Loading

rajagopalanand Jun 27, 2024 •

edited

Loading