Adding support for list rules API filter parameters #5417

rajagopalanand · 2023-06-20T13:41:28Z

What this PR does:

This PR is introduces ability to filter rules in alignment with Prometheus

Checklist

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

docs/contributing/how-integration-tests-work.md

qinxx108 · 2023-06-20T15:10:57Z

pkg/ruler/api.go

@@ -119,6 +125,26 @@ func respondError(logger log.Logger, w http.ResponseWriter, msg string) {
 	}
 }

+func respondClientError(logger log.Logger, w http.ResponseWriter, msg string) {


what's the different of this function with respondError?

That function returns 400 vs respondError returns 500. I added the function to be consistent with respondAccepted()

respondAccepted() has more differences in terms of the response message. respondClientError() only difference is the status code being returned.

What about adding a status code parameter to respondError() ?

I don't think it's a good idea to move towards having dedicated methods for each response code. It's adding even more code duplication for little benefit.

Changed this to return 400 and changed to return bad request/data.

alvinlin123 · 2023-06-20T15:59:59Z

CHANGELOG.md

@@ -30,6 +30,7 @@
 * [ENHANCEMENT] Store Gateway: Add new metrics `cortex_bucket_store_sent_chunk_size_bytes`, `cortex_bucket_store_postings_size_bytes` and `cortex_bucket_store_empty_postings_total`. #5397
 * [ENHANCEMENT] Add jitter to lifecycler heartbeat. #5404
 * [ENHANCEMENT] Store Gateway: Add config `estimated_max_series_size_bytes` and `estimated_max_chunk_size_bytes` to address data overfetch. #5401
+* [ENHANCEMENT] Ruler: Support for filtering rules. #5417


We should be more precise and mention it's for API

alvinlin123 · 2023-06-20T16:02:50Z

pkg/ruler/api.go

+func respondClientError(logger log.Logger, w http.ResponseWriter, msg string) {
+	b, err := json.Marshal(&response{
+		Status:    "error",
+		ErrorType: v1.ErrServer,


If this function returns 4xx, then shouldn't the ErrorType be v1.ErrClient?

Changed this to v1.ErrBadData

pkg/ruler/api.go

alvinlin123 · 2023-06-20T16:08:44Z

pkg/ruler/api.go

@@ -145,8 +171,27 @@ func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) {
 		return
 	}

+	if err := req.ParseForm(); err != nil {
+		level.Error(logger).Log("msg", "error parsing form/query params", "err", err)
+		respondClientError(logger, w, err.Error())


Are you sure req.ParseForm()'s error translate to client error? From quick look of the req.ParseForm()'s implementation, it doesn't look like client error is the right choice here.

Changed this to respondBadRequest() as that is more appropriate

pkg/ruler/api.go

docs/contributing/how-integration-tests-work.md

soonping-amzn · 2023-06-20T16:46:00Z

pkg/ruler/api.go

@@ -119,6 +125,26 @@ func respondError(logger log.Logger, w http.ResponseWriter, msg string) {
 	}
 }

+func respondClientError(logger log.Logger, w http.ResponseWriter, msg string) {


respondAccepted() has more differences in terms of the response message. respondClientError() only difference is the status code being returned.

What about adding a status code parameter to respondError() ?

I don't think it's a good idea to move towards having dedicated methods for each response code. It's adding even more code duplication for little benefit.

soonping-amzn · 2023-06-20T16:52:49Z

pkg/ruler/api.go

+
+	typ := strings.ToLower(req.URL.Query().Get("type"))
+	if typ != "" && typ != AlertingRuleFilter && typ != RecordingRuleFilter {
+		respondClientError(logger, w, fmt.Sprintf("not supported value %q", typ))


"value" can be a bit vague, especially if the user provides additional parameters.

How about "unsupported rule type: %q" ?

qinxx108 · 2023-06-20T19:03:40Z

pkg/ruler/api.go

@@ -99,6 +100,11 @@ type recordingRule struct {
 	EvaluationTime float64       `json:"evaluationTime"`
 }

+const (
+	AlertingRuleFilter  string = "alert"


maybe move this into ruler.go? as ruler need to use this const as well

CHANGELOG.md

pkg/ruler/ruler.go

yeya24 · 2023-06-21T20:09:41Z

pkg/ruler/ruler.proto

+  repeated string ruleGroupNames = 2;
+  repeated string files = 3;
+  string type = 4;
+}


Will this cause compatibility issue? During rollout, protocol changes and new/old ruler cannot communicate with each other?

If the clients do not send any filters, then the results are consistent during deployment. However, if the clients start sending filters during deployment, the results are inconsistent until the deployment is complete. This is because old ruler does not filter and the new one will

docs/contributing/how-integration-tests-work.md

pkg/ruler/ruler_test.go

yeya24

Thanks!

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

pull-request-size bot added the size/L label Jun 20, 2023

rajagopalanand force-pushed the list-rules-filter branch from 8c27b9e to 6f570e0 Compare June 20, 2023 14:20

qinxx108 reviewed Jun 20, 2023

View reviewed changes

docs/contributing/how-integration-tests-work.md Show resolved Hide resolved

qinxx108 reviewed Jun 20, 2023

View reviewed changes

alvinlin123 requested review from friedrichg, alanprot and yeya24 June 20, 2023 15:58

alvinlin123 suggested changes Jun 20, 2023

View reviewed changes

soonping-amzn reviewed Jun 20, 2023

View reviewed changes

qinxx108 reviewed Jun 20, 2023

View reviewed changes

rajagopalanand force-pushed the list-rules-filter branch from 4db84b5 to b420fb3 Compare June 21, 2023 00:13

alvinlin123 changed the title ~~List rules filter~~ Adding support for list rules API filter parameters Jun 21, 2023

yeya24 reviewed Jun 21, 2023

View reviewed changes

rajagopalanand force-pushed the list-rules-filter branch 5 times, most recently from 1c0ceb2 to 209982f Compare June 26, 2023 00:02

pull-request-size bot added size/XL and removed size/L labels Jun 26, 2023

rajagopalanand force-pushed the list-rules-filter branch 5 times, most recently from 2ecf6e5 to bbd2e23 Compare June 26, 2023 14:41

yeya24 reviewed Jun 26, 2023

View reviewed changes

rajagopalanand force-pushed the list-rules-filter branch 4 times, most recently from 9423eaf to c91ffeb Compare June 27, 2023 01:16

rajagopalanand requested a review from yeya24 June 27, 2023 02:39

yeya24 approved these changes Jun 27, 2023

View reviewed changes

rajagopalanand force-pushed the list-rules-filter branch 2 times, most recently from c68e8fa to 7a23683 Compare June 28, 2023 20:12

rajagopalanand added 4 commits June 28, 2023 20:14

filter rules

392d29a

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

filter rules

b16fceb

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

Adding support to filter rules

03ce53f

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

Adding support to filter rules

0e39bd9

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

rajagopalanand force-pushed the list-rules-filter branch from 7a23683 to 0e39bd9 Compare June 28, 2023 20:19

alvinlin123 approved these changes Jun 28, 2023

View reviewed changes

alvinlin123 merged commit 9e783e7 into cortexproject:master Jun 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for list rules API filter parameters #5417

Adding support for list rules API filter parameters #5417

rajagopalanand commented Jun 20, 2023

qinxx108 Jun 20, 2023

rajagopalanand Jun 20, 2023

soonping-amzn Jun 20, 2023

rajagopalanand Jun 21, 2023

alvinlin123 Jun 20, 2023

rajagopalanand Jun 28, 2023

alvinlin123 Jun 20, 2023

rajagopalanand Jun 21, 2023

alvinlin123 Jun 20, 2023

rajagopalanand Jun 21, 2023

soonping-amzn Jun 20, 2023

soonping-amzn Jun 20, 2023

qinxx108 Jun 20, 2023

rajagopalanand Jun 22, 2023

yeya24 Jun 21, 2023

rajagopalanand Jun 22, 2023

yeya24 left a comment

Adding support for list rules API filter parameters #5417

Adding support for list rules API filter parameters #5417

Conversation

rajagopalanand commented Jun 20, 2023

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

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

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

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 left a comment

Choose a reason for hiding this comment