Pagination support for ListRules #6299

rajagopalanand · 2024-11-01T02:29:59Z

What this PR does:

This PR adds pagination support for List Rules API similar to Prometheus pagination feature for List Rules

getShardedRules() calls getLocalRules()
getLocalRules() sorts the results (rules on the local ruler) by token
getLocalRules() then compares the input token to the calculated token for each rule group to generate the next page by calling generatePage
Finally getShardedRules() merges results from all the getLocalRules(), deduplicates, sorts using token as the key, and generates the next page

Checklist

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

Signed-off-by: Anand Rajagopal <[email protected]>

yeya24

Thanks this looks good.

yeya24 · 2024-11-04T21:41:30Z

integration/e2ecortex/client.go

+	}
+	if filter.NextToken != "" {
+		addQueryParams(urlValues, "group_next_token", filter.NextToken)
+	}


Nit: If we just add group_next_token="" what will happen? It should still be a non paginated request?
Maybe we can just remove the condition check here

To keep consistency with upstream, we should fail if group_next_token was provided but group_limit was not.

https://github.com/prometheus/prometheus/blob/main/web/api/v1/api.go#L1609

Never mind, I see the validation in the API.

yeya24 · 2024-11-04T21:48:06Z

pkg/ruler/ruler.go

 }

-func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeBackups bool) ([]*GroupStateDesc, error) {
+func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeBackups bool) (RulesResponse, error) {


Nit: Why we don't return pointer here? since pointer is what is asked by GetRules.

rapphil · 2024-11-05T00:15:31Z

integration/ruler_test.go

+				MaxRuleGroup: 20,
+			},
+			resultCheckFn: func(t assert.TestingT, resultGroups []*ruler.RuleGroup, token string, iteration int) {
+				assert.Len(t, resultGroups, 20, "Expected %d rules but got %d", 20, len(resultGroups))


nit: doesn't the error message from the assertion tell us what was expected vs the actual?

rapphil · 2024-11-05T00:15:35Z

integration/ruler_test.go

+	}{
+		"List Rule Groups - Equal number of rule groups per page": {
+			filter: e2ecortex.RuleFilter{
+				MaxRuleGroup: 20,


can we add a test that use the next token?

rapphil · 2024-11-05T00:46:27Z

pkg/ruler/merger.go

+
+	if maxRuleGroups > 0 {
+		//Need to sort here before we truncate
+		sort.Sort(GroupStateDescs(groups))


I wonder if we should always sort the result.

We are already sorting the results in the prometheus list rules:

cortex/pkg/ruler/api.go

Line 243 in c25b18d

sort.Slice(groups, func(i, j int) bool {

So we could sort just once here instead for all the cases. This will reduce modality and simplify the code a bit. WDYT @yeya24 ?

Also, where is the next token from the previous request being used here to generate the response starting from the rule group that we truncated?

I wonder if we should always sort the result.

We are already sorting the results in the prometheus list rules:

cortex/pkg/ruler/api.go

Line 243 in c25b18d

sort.Slice(groups, func(i, j int) bool {

So we could sort just once here instead for all the cases. This will reduce modality and simplify the code a bit. WDYT @yeya24 ?

Without pagination, the size of the list can be pretty substantial and we would sort it twice. This was the reason I did not want to sort for non-paginated requests

Also, where is the next token from the previous request being used here to generate the response starting from the rule group that we truncated?

Each response from getLocalRules has been filtered using the next token

Each response from getLocalRules has been filtered using the next token

but what about the case you are using sharding?

My understanding is that in this case the ruler that receives the request will look into the ring for the other rulers that contain the rules and fire requests to each to collect from them. This ruler will then consolidate the list of rules in a single list before returning the response. I was expecting to see the pagination happen at this point because we need the complete list of rules to paginate properly.

My understanding is that it is not possible to create a distributed pagination algorithm because the way how we spread the rulers in rulers. This specially important if there are ring reorganization during requests.

Each response from getLocalRules has been filtered using the next token

but what about the case you are using sharding?

getShardedRules() calls getLocalRules(). So the results from each ruler is already filtered using next token

Ok, we discussed offline, and there was a misunderstanding from my part. I stand corrected.

Since we are truncating on a sorted list, where the sorting happens by the sha(namespace + group), it is safe to use the >= as filter when we spread the requests to get the rules from other rulers. This is like a distributed filter operation.

Having said that, It would not hurt to add a bit of description to the PR to detail how the implementation was done.

rapphil · 2024-11-05T00:47:54Z

pkg/ruler/ruler_pagination.go

+func GetRuleGroupNextToken(namespace string, group string) string {
+	h := sha1.New()
+	h.Write([]byte(namespace + ";" + group))
+	return hex.EncodeToString(h.Sum(nil))
+}
+
+func TruncateGroups(groups []*GroupStateDesc, maxRuleGroups int) ([]*GroupStateDesc, string) {


do these functions need to be public?

I'm talking only about TruncateGroups and GetRuleGroupNextToken

TruncateGroups does not need to be public. I will fix it. GetRuleGroupNextToken needs to be public since it is used in integration testing

I would actually move all the content of this file to close where they are used, since they are only used in one place. I'm not particularly strong about this but I think it would be simpler to understand.

Another suggestion, instead of truncateGroups we could name this function generatePage, createPage, makePage or similar to make it clear that this function is used to generate the page response with token etc. I think this function deserves some comments describing how the pagination works and that is supposed to receive a sorted list of groups. just my two cents.

rapphil · 2024-11-05T06:41:10Z

pkg/ruler/ruler.go

+	resultingGroupDescs := make([]*GroupStateDesc, 0, len(combinedRuleStateDescs))
+	for _, group := range combinedRuleStateDescs {
+		groupID := GetRuleGroupNextToken(group.Group.Namespace, group.Group.Name)
+		if len(rulesRequest.NextToken) > 0 && rulesRequest.NextToken >= groupID {


there is no meaning in comparing two hashes sha1() using >=. We should use == only.

discussed offline, this is ok because are assuming that combinedRulesStateDescs is sorted by sha.

please add a small comment here detailing why it is ok to compare groupIDs.

rapphil · 2024-11-05T18:48:02Z

pkg/ruler/ruler_pagination.go

+	"encoding/hex"
+)
+
+type GroupStateDescs []*GroupStateDesc


Should this be called PaginedGroupStates or something like that since this is only used in the pagination context?

Also I don't think this type need to be public right?

It will now need to be public because it is used in integ tests

Signed-off-by: Anand Rajagopal <[email protected]>

rapphil · 2024-11-06T07:03:45Z

pkg/ruler/ruler_pagination.go

@@ -5,21 +5,24 @@ import (
 	"encoding/hex"
 )

-type GroupStateDescs []*GroupStateDesc
+type PaginedGroupStates []*GroupStateDesc


typo? PaginedGroupStates -> PaginatedGroupStates

Signed-off-by: Anand Rajagopal <[email protected]>

yeya24

Thanks

pull-request-size bot added the size/XL label Nov 1, 2024

rajagopalanand force-pushed the paginated-list-rules branch 5 times, most recently from f51bdef to e710de2 Compare November 3, 2024 20:32

Pagination support for ListRules

4c1d435

Signed-off-by: Anand Rajagopal <[email protected]>

rajagopalanand force-pushed the paginated-list-rules branch from e710de2 to 4c1d435 Compare November 3, 2024 21:04

rajagopalanand marked this pull request as ready for review November 3, 2024 21:11

dosubot bot added the component/rules Bits & bobs todo with rules and alerts: the ruler, config service etc. label Nov 3, 2024

yeya24 approved these changes Nov 4, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 4, 2024

rapphil reviewed Nov 5, 2024

View reviewed changes

Addressing comments

46b004e

Signed-off-by: Anand Rajagopal <[email protected]>

rajagopalanand requested a review from rapphil November 6, 2024 03:17

rapphil reviewed Nov 6, 2024

View reviewed changes

rapphil approved these changes Nov 6, 2024

View reviewed changes

Fixed a typo

2e53b88

Signed-off-by: Anand Rajagopal <[email protected]>

yeya24 approved these changes Nov 6, 2024

View reviewed changes

yeya24 merged commit 9acd3f7 into cortexproject:master Nov 6, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pagination support for ListRules #6299

Pagination support for ListRules #6299

rajagopalanand commented Nov 1, 2024 •

edited

Loading

yeya24 left a comment

yeya24 Nov 4, 2024

rapphil Nov 4, 2024 •

edited

Loading

rapphil Nov 5, 2024

yeya24 Nov 4, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rajagopalanand Nov 5, 2024

rajagopalanand Nov 5, 2024

rapphil Nov 5, 2024 •

edited

Loading

rajagopalanand Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rajagopalanand Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024

rapphil Nov 5, 2024 •

edited

Loading

rajagopalanand Nov 6, 2024

rapphil Nov 6, 2024

yeya24 left a comment

Pagination support for ListRules #6299

Pagination support for ListRules #6299

Conversation

rajagopalanand commented Nov 1, 2024 • edited Loading

yeya24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rapphil Nov 4, 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

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

rapphil Nov 5, 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

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

rapphil Nov 5, 2024 • edited Loading

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

rajagopalanand commented Nov 1, 2024 •

edited

Loading

rapphil Nov 4, 2024 •

edited

Loading

rapphil Nov 5, 2024 •

edited

Loading

rapphil Nov 5, 2024 •

edited

Loading