Add paginated feature to list rules api #14017

qinxx108 · 2024-04-30T17:44:00Z

This is to implement the proposal here: #12120
There is 3 parameters added:

MaxAlerts: Limit number of alerts returned for each rules, if there is more alerts for a rule, we will return hasMore = true
MaxRuleGroups: Limit number of rule groups returned, if there is more rule groups to return, we will return a NextToken in the response
NextToken: The paginate token

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

beorn7 · 2024-05-08T17:10:05Z

I tagged people that have discussed #12120 as reviewers.

beorn7 · 2024-05-22T10:49:34Z

@roidelapluie @codesome @gotjosh @juliusv could you have a look? Or at least let us know if you have noticed this PR, but you don't have time to get to it so that less qualified reviewers might give it a try?

codesome · 2024-05-31T17:16:30Z

I won't have time to review this PR and I don't feel qualified enough for this part of the code.

juliusv

Finally got around to taking a closer look at this one! Overall the approach looks pretty good to me, I left a number of questions and comments about the details though.

It's of course not perfect in the face of rule reloads happening while you paginate, but from reading #12120 I think that is understood.

Once the design + implementation settles, we'll also need https://github.com/prometheus/prometheus/blob/main/docs/querying/api.md#rules to be updated with documentation about the new functionality.

juliusv · 2024-06-01T09:52:29Z

web/api/v1/api.go

+		maxAlert, err := strconv.ParseInt(r.URL.Query().Get("maxAlerts"), 10, 32)
+		if err != nil || maxAlert < 0 {
+			return nil, &parsePaginationError{
+				err:       fmt.Errorf("maxAlerts need to be a valid number and larger than 0: %w", err),


Here it says that maxAlerts needs to be larger than 0, but the check above only checks that it's at least 0. Which one is intended?

If you want to allow 0, then maybe strconv.ParseUInt() would be better. Then you can omit the explicit maxAlert < 0 check completely as that's covered by the function.

juliusv · 2024-06-01T09:58:23Z

web/api/v1/api.go

+
+	if r.URL.Query().Get("maxRuleGroups") != "" {
+		maxRuleGroups, err := strconv.ParseInt(r.URL.Query().Get("maxRuleGroups"), 10, 32)
+		if err != nil || maxRuleGroups < 0 {


juliusv · 2024-06-01T09:58:52Z

web/api/v1/api.go

@@ -1515,6 +1607,77 @@ func parseExcludeAlerts(r *http.Request) (bool, error) {
 	return excludeAlerts, nil
 }

+func parseListRulesPaginationRequest(r *http.Request) (*listRulesPaginationRequest, *parsePaginationError) {
+	var (
+		returnMaxAlert      = int32(-1)


For consistency:

Suggested change

returnMaxAlert = int32(-1)

returnMaxAlerts = int32(-1)

juliusv · 2024-06-01T10:03:44Z

web/api/v1/api.go

+func parseListRulesPaginationRequest(r *http.Request) (*listRulesPaginationRequest, *parsePaginationError) {
+	var (
+		returnMaxAlert      = int32(-1)
+		returnMaxRuleGroups = int32(-1)


I'd probably just use uint64 for these, even if we never expect to exceed an int32. Just feels more natural nowadays, avoids parsing issues when a user sets a ridiculously high limit, and you need to have one less cast (if changing to strconv.ParseUInt() below.

juliusv · 2024-06-01T10:08:16Z

web/api/v1/api.go

+				parameter: "maxAlerts",
+			}
+		}
+		returnMaxAlert = int32(maxAlert)


I don't think we need two variable names for each parameter (e.g. maxAlert and returnMaxAlerts), at least after making the variable type adjustments I mentioned above. I'd just omit the ones with the return prefix, set the three variables to their default values at the top, and then overwrite them directly from the parsed parameters. That's a bit simpler to read as well I think.

juliusv · 2024-06-01T11:31:58Z

web/api/v1/api.go

+}
+
+func TruncateGroupInfos(groupInfos []*RuleGroup, maxRuleGroups int) ([]*RuleGroup, string) {
+	resultNumber := 0


For simplicity I'd probably omit this extra tracking variable and just use len(returnGroupDescs) instead in both places where it's needed.

juliusv · 2024-06-01T11:33:24Z

web/api/v1/api.go

+			continue
+		}
+
+		// Return the next token if there is more aggregation group


Suggested change

// Return the next token if there is more aggregation group

// Return the next token if there are more aggregation groups

juliusv · 2024-06-01T11:50:00Z

web/api/v1/api.go

+	return nil, nil
+}
+
+func TruncateGroupInfos(groupInfos []*RuleGroup, maxRuleGroups int) ([]*RuleGroup, string) {


Now reading this function again after adding the other comments about it, I think it'd make sense to write it in a different way that would make it simpler and more efficient:

Do a special-case early exit for the maxRuleGroups < 0 || len(groupInfos) <= maxRuleGroups case (just return groupInfos as is and "" for the token).

Then: Instead of looping over all groups and appending them individually, just return groupInfos[:maxRuleGroups] for the rule groups and the token for groupInfos[maxRuleGroups-1].

the issue is that we need to truncate the result of filtering the rule groups based on the file names and group names. There is a small edge case that blocks the optmized version: a list of rule groups for a page can be the last one or not depending if the subsequent rule groups are filtered out or not.

E.g.:
/rules?rule_groups[]=a1,b1,c1&maxAlerts=1

in the rule groups we have

a1

b1

c1

d1

When we are iterating over the rule groups, there is no way to know if c1 is going to produce a page with nextToke="" before we evaluate d1.

Ah ok, thanks for the explanation!

web/api/v1/api.go

juliusv · 2024-06-01T11:58:00Z

web/api/v1/api.go

@@ -168,6 +169,17 @@ type apiFuncResult struct {

 type apiFunc func(r *http.Request) apiFuncResult

+type listRulesPaginationRequest struct {
+	MaxAlerts     int32


I think I'd just stick to either int or int64 for these fields here, less conversions / casts needed later on and no need to artificially limit the value range.

* Reduce number of variables * Reduce type convesion Signed-off-by: Raphael Silva <rapphil@gmail.com>

Addressing comments for pull request

* Remove maxAlerts parameter. * Reuse existing API responses by using omitempty in some fields Signed-off-by: Raphael Silva <rapphil@gmail.com>

* Eliminate the need to sort the rule groups. Signed-off-by: Raphael Silva <rapphil@gmail.com>

…ted-list-rules

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil · 2024-07-04T23:41:05Z

Ok, to simplify this PR I made the following changes:

Remove MaxAlerts parameter -> we already have the exclude_alerts parameter. This can be added in a later PR if necessary, but right now I don't think the user experience with maxAlerts is great: different output format, no way to paginate. So I think it is better to remove it.
Do not sort the rule groups before paginating and Instead just rely on the name of the rule groups to find the next token marker and return results in the same order that they are created by users.
Add optional field NextToken in the RuleDiscovery that will be present in case the response to list rules has more pages.

Things that I'm not happy right now about this PR and would like input:

Parameter naming conventions. What is the convention for naming API parameters? I can see that some APIs use camel case (e.g.: scrapePool) for query parameters while other uses snake case (e.g.: rule_group[]).
What should be the behaviour in case user pass a request with next token, but no MaxRules?
- Should I return everything starting from the nextToken -> this makes sense because we can simple say that by default the API will return all results.
- Use a different default, say 10.

rapphil · 2024-07-04T23:44:53Z

I added a bit more unit tests.

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil · 2024-07-15T05:45:54Z

Hi @bwplotka 👋🏽

I saw that you reviewed a similar PR recently: #10194

Would you be able to help reviewing this one as well?

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil · 2024-07-15T14:39:56Z

I ended updating the new parameters so that they are consistent with the existing ones where we use snake case instead of camel case.

bwplotka · 2024-07-17T16:07:55Z

Thanks, I am on PTO but in between cocktails I will review (joking on cocktails, non sober merging is not a great practice 😜).

bwplotka · 2024-08-07T09:01:42Z

Parameter naming conventions. What is the convention for naming API parameters? I can see that some APIs use camel case (e.g.: scrapePool) for query parameters while other uses snake case (e.g.: rule_group[]).

Nice, at least rules API is snake case, so let's keep that? https://prometheus.io/docs/prometheus/latest/querying/api/#rules

What should be the behaviour in case user pass a request with next token, but no MaxRules?
Should I return everything starting from the nextToken -> this makes sense because we can simple say that by default the API will return all results.
Use a different default, say 10.

Interesting, I think what you suggested make sense (return all from the token)

Signed-off-by: Raphael Philipe Mendes da Silva <rapphil@gmail.com>

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil · 2024-10-08T16:27:12Z

@bwplotka Thanks for your review. The code looks a lot cleaner now and all the corners cases are being handled.

Signed-off-by: Raphael Silva <rapphil@gmail.com>

bwplotka · 2024-10-17T07:49:39Z

docs/querying/api.md

@@ -764,6 +764,8 @@ URL query parameters:
 - `file[]=<string>`: only return rules with the given filepath. If the parameter is repeated, rules with any of the provided filepaths are returned. When the parameter is absent or empty, no filtering is done.
 - `exclude_alerts=<bool>`: only return rules, do not return active alerts.
 - `match[]=<label_selector>`: only return rules that have configured labels that satisfy the label selectors. If the parameter is repeated, rules that match any of the sets of label selectors are returned. Note that matching is on the labels in the definition of each rule, not on the values after template expansion (for alerting rules). Optional.
+- `max_groups=<number>`: The `max_groups` parameter allows you to specify the maximum number of rule groups to return in a single response. If the total number of rule groups exceeds the specified `max_groups` value, the response will include a `nextToken` property. You can use the value of this `nextToken` property in subsequent requests in the `next_token` parameter to paginate over the remaining rule groups. The `nextToken` property will not be present in the final response, indicating that you have retrieved all the available rule groups. Please note that there are no guarantees regarding the consistency of the response if the rule groups are being modified during the pagination process. If a rule group is removed while you are paginating over the rule groups, an error might be raised if the removed rule group coincides with the next token.
+- `next_token`: the pagination token that was returned in previous request when the `max_groups` property is set. The pagination token is used to iteratively paginate over a large number of rule groups. To use the `next_token` parameter, the `max_groups` parameter also need to be present.


What will happen if such token will no longer exists or will be wrong? (400 will be returned, looking on code, so should we document that?)

bwplotka · 2024-10-17T07:50:25Z

web/api/v1/api.go

@@ -1,5 +1,5 @@
 // Copyright 2016 The Prometheus Authors
-// Licensed under the Apache License, Version 2.0 (the "License");


Is this an incidental change?

argh. fixing it.

bwplotka

Generally LGTM, thanks for improvements and sorry for delays.

I want to double check if we want to rename those params to:

group_limit
group_next_token

This is for consistency with limit params on other APIs. The group prefix is to be clear we paginate per group not rule, and to be future proof if we ever want to add per rule pagination. WDYT? 🤗

bwplotka · 2024-10-17T11:12:24Z

web/api/v1/api.go

+	if paginationRequest != nil && paginationRequest.NextToken != "" && !foundToken {
+		err := fmt.Errorf("invalid nextToken '%v'. were rule groups changed?", paginationRequest.NextToken)
+		return invalidParamError(err, "next_token")
+	}


Suggested change

if paginationRequest != nil && paginationRequest.NextToken != "" && !foundToken {

err := fmt.Errorf("invalid nextToken '%v'. were rule groups changed?", paginationRequest.NextToken)

return invalidParamError(err, "next_token")

}

if paginationRequest != nil && paginationRequest.NextToken != "" && !foundToken {

return invalidParamError(fmt.Errorf("invalid nextToken %; did rule groups change?", paginationRequest.NextToken), "next_token")

}

web/api/v1/api.go

bwplotka · 2024-10-17T11:18:06Z

web/api/v1/api.go

+		parsedMaxGroups int64 = -1
+		err             error
+	)
+	maxGroups := r.URL.Query().Get("max_groups")


What about group_limit naming instead? This might be consistent with other limit parameters in our API.

I guess one counter argument is that limit never supported tokens/pagination, but maybe it does not matter and limit would be better?

Consistency for the win :) . Actually can we say the APIs that have limit can support pagination in the future?

bwplotka · 2024-10-17T11:21:27Z

web/api/v1/api.go

+		err             error
+	)
+	maxGroups := r.URL.Query().Get("max_groups")
+	nextToken := r.URL.Query().Get("next_token")


next_token is great if we will never paginate over rules (: Are we sure about this? Otherwise maybe group_next_token and group_limit would be a nice, explicit pair?

bwplotka · 2024-10-17T11:23:50Z

web/api/v1/api.go

 	for _, grp := range ruleGroups {
+		if paginationRequest != nil && paginationRequest.NextToken != "" && !foundToken {


FYI: Code might actually simpler if we don't have a separate struct but groupNextToken and groupLimit variables 🙈 parse can return two vars or so (:

will try to remove it.

done. it saved a few lines :)

* Eliminate need for extra structs to store pagination parameters Signed-off-by: Raphael Silva <rapphil@gmail.com>

bwplotka

LGTM! Thanks!

web/api/v1/api.go

bwplotka · 2024-10-18T07:38:38Z

Before merging I wanted to have one "last" discussion to double check the hardest thing in programming: naming (:

https://cloud-native.slack.com/archives/C01AUBA4PFE/p1729235985519149

juliusv

👍 Looks good now besides the last comment!

docs/querying/api.md

Co-authored-by: Julius Volz <julius.volz@gmail.com> Signed-off-by: Raphael Philipe Mendes da Silva <rapphil@gmail.com>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Raphael Philipe Mendes da Silva <rapphil@gmail.com>

Signed-off-by: Raphael Silva <rapphil@gmail.com>

bwplotka

Thanks for prompt review changes after feedback; LGTM!

* master: (667 commits) NHCB scrape: refactor state handling and speed up scrape test (prometheus#15193) feat(tools): add debug printouts to rules unit testing (prometheus#15196) docs: add keep_firing_for in alerting rules api: Add rule group pagination to list rules api (prometheus#14017) Update scrape/scrape.go benchmark, rename parser omtext_with_nhcb goimports run Better docstring on test Remove omcounterdata.txt as redundant Fix failing benchmarks Add unit test to show that current wrapper is sub-optimal Rename convert_classic_histograms to convert_classic_histograms_to_nhcb More followup to prometheus#15164 Follow up prometheus#15178 Followup to prometheus#15164 test(cmd/prometheus): speed up test execution by t.Parallel() when possible feat: normalize "le" and "quantile" labels values upon ingestion scrape: provide a fallback format (prometheus#15136) Disallowing configure AM with the v1 api (prometheus#13883) feat: ProtobufParse.formatOpenMetricsFloat: improve float formatting by using strconv.AppendFloat instead of fmt.Sprint ... # Conflicts: # go.mod # go.sum

qinxx108 force-pushed the paginated-list-rules branch 2 times, most recently from 8617a47 to 9dfdb47 Compare April 30, 2024 21:45

Add paginated feature to list rules api

b674217

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

qinxx108 force-pushed the paginated-list-rules branch from 9dfdb47 to b674217 Compare April 30, 2024 22:26

beorn7 requested review from roidelapluie, codesome, gotjosh and juliusv May 8, 2024 17:09

codesome removed their request for review May 31, 2024 17:17

juliusv reviewed Jun 1, 2024

View reviewed changes

Refactor to simplify code:

bbd2f1c

* Reduce number of variables * Reduce type convesion Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil mentioned this pull request Jun 11, 2024

Addressing comments for pull request qinxx108/prometheus#228

Merged

qinxx108 and others added 5 commits June 11, 2024 12:15

Merge pull request #228 from rapphil/rapphil-address-comment-list-rules

5545b53

Addressing comments for pull request

Simplify paginated implementation

5759216

* Remove maxAlerts parameter. * Reuse existing API responses by using omitempty in some fields Signed-off-by: Raphael Silva <rapphil@gmail.com>

Simplify pagination implementation

71c9dfa

* Eliminate the need to sort the rule groups. Signed-off-by: Raphael Silva <rapphil@gmail.com>

Merge remote-tracking branch 'yijei/paginated-list-rules' into pagina…

2cf34e4

…ted-list-rules

Fix linting error

ee70c9d

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil added 3 commits July 8, 2024 04:45

Add more unit tests

5fe8baf

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Merge remote-tracking branch 'origin/main' into paginated-list-rules

7ceddec

Update pagination parameters to be consistent with existing parameters

b9c0c84

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil force-pushed the paginated-list-rules branch from 45b39d7 to b9c0c84 Compare July 15, 2024 05:43

Rename max_rule_groups to max_groups

11c20b8

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil added 2 commits October 4, 2024 17:06

Merge branch 'main' into paginated-list-rules

5825efd

Signed-off-by: Raphael Philipe Mendes da Silva <rapphil@gmail.com>

Refactor to simplify code

b028620

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil force-pushed the paginated-list-rules branch from badfccf to 3936f2a Compare October 8, 2024 04:40

rapphil added 4 commits October 8, 2024 16:26

Refactor to simplify the calculation of next token

aac07c6

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Handle corner case in pagination request

2ef6532

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Handle corner cases for pagination of list rules

dc47970

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Update documentation for list rules parameters

cd75574

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil force-pushed the paginated-list-rules branch from de715ac to cd75574 Compare October 8, 2024 16:26

Refactor comments

dfce254

Signed-off-by: Raphael Silva <rapphil@gmail.com>

bwplotka reviewed Oct 17, 2024

View reviewed changes

bwplotka previously approved these changes Oct 17, 2024

View reviewed changes

Simplify pagination implementation

e4b3aca

* Eliminate need for extra structs to store pagination parameters Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil dismissed bwplotka’s stale review via e4b3aca October 17, 2024 19:18

bwplotka previously approved these changes Oct 18, 2024

View reviewed changes

web/api/v1/api.go Outdated Show resolved Hide resolved

juliusv reviewed Oct 21, 2024

View reviewed changes

docs/querying/api.md Outdated Show resolved Hide resolved

Update docs/querying/api.md

8b028d5

Co-authored-by: Julius Volz <julius.volz@gmail.com> Signed-off-by: Raphael Philipe Mendes da Silva <rapphil@gmail.com>

rapphil dismissed bwplotka’s stale review via 8b028d5 October 21, 2024 21:15

rapphil and others added 2 commits October 21, 2024 15:18

Update web/api/v1/api.go

585a60f

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Raphael Philipe Mendes da Silva <rapphil@gmail.com>

Update comment describing the need for next token

d313df6

Signed-off-by: Raphael Silva <rapphil@gmail.com>

bwplotka approved these changes Oct 21, 2024

View reviewed changes

bwplotka merged commit d2802c6 into prometheus:main Oct 21, 2024
26 checks passed

fayzal-g mentioned this pull request Oct 24, 2024

Ruler: Add pagination for api/v1/rules grafana/mimir#9563

Merged

rajagopalanand mentioned this pull request Nov 3, 2024

Pagination support for ListRules cortexproject/cortex#6299

Merged

3 tasks

beorn7 mentioned this pull request Nov 12, 2024

[Feature Request] Pagination support for rule listing APIs #12120

Closed

julienduchesne pushed a commit to julienduchesne/prometheus that referenced this pull request Dec 13, 2024

api: Add rule group pagination to list rules api (prometheus#14017)

f449671

	// Return the next token if there is more aggregation group
	// Return the next token if there are more aggregation groups

		@@ -1,5 +1,5 @@
		// Copyright 2016 The Prometheus Authors
		// Licensed under the Apache License, Version 2.0 (the "License");

		for _, grp := range ruleGroups {
		if paginationRequest != nil && paginationRequest.NextToken != "" && !foundToken {

Add paginated feature to list rules api #14017

Add paginated feature to list rules api #14017

Uh oh!

Conversation

qinxx108 commented Apr 30, 2024

Uh oh!

beorn7 commented May 8, 2024

Uh oh!

beorn7 commented May 22, 2024

Uh oh!

codesome commented May 31, 2024

Uh oh!

juliusv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rapphil commented Jul 4, 2024

Uh oh!

rapphil commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rapphil commented Jul 15, 2024

Uh oh!

rapphil commented Jul 15, 2024

Uh oh!

bwplotka commented Jul 17, 2024

Uh oh!

bwplotka commented Aug 7, 2024

Uh oh!

rapphil commented Oct 8, 2024

Uh oh!

bwplotka Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rapphil commented Jul 4, 2024 •

edited

Loading

bwplotka Oct 17, 2024 •

edited

Loading

bwplotka Oct 17, 2024 •

edited

Loading