Add paginated feature to list rules api #14017
Conversation
qinxx108
force-pushed
the
paginated-list-rules
branch
2 times, most recently
from
8617a47
to
9dfdb47
Compare
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
qinxx108
force-pushed
the
paginated-list-rules
branch
from
9dfdb47
to
b674217
Compare
|
I tagged people that have discussed #12120 as reviewers. |
|
@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? |
|
I won't have time to review this PR and I don't feel qualified enough for this part of the code. |
There was a problem hiding this comment.
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.
| 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), |
There was a problem hiding this comment.
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.
|
|
||
| if r.URL.Query().Get("maxRuleGroups") != "" { | ||
| maxRuleGroups, err := strconv.ParseInt(r.URL.Query().Get("maxRuleGroups"), 10, 32) | ||
| if err != nil || maxRuleGroups < 0 { |
| @@ -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) | |||
There was a problem hiding this comment.
For consistency:
| returnMaxAlert = int32(-1) | |
| returnMaxAlerts = int32(-1) |
| func parseListRulesPaginationRequest(r *http.Request) (*listRulesPaginationRequest, *parsePaginationError) { | ||
| var ( | ||
| returnMaxAlert = int32(-1) | ||
| returnMaxRuleGroups = int32(-1) |
There was a problem hiding this comment.
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.
| parameter: "maxAlerts", | ||
| } | ||
| } | ||
| returnMaxAlert = int32(maxAlert) |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| func TruncateGroupInfos(groupInfos []*RuleGroup, maxRuleGroups int) ([]*RuleGroup, string) { | ||
| resultNumber := 0 |
There was a problem hiding this comment.
For simplicity I'd probably omit this extra tracking variable and just use len(returnGroupDescs) instead in both places where it's needed.
| continue | ||
| } | ||
|
|
||
| // Return the next token if there is more aggregation group |
There was a problem hiding this comment.
| // Return the next token if there is more aggregation group | |
| // Return the next token if there are more aggregation groups |
| return nil, nil | ||
| } | ||
|
|
||
| func TruncateGroupInfos(groupInfos []*RuleGroup, maxRuleGroups int) ([]*RuleGroup, string) { |
There was a problem hiding this comment.
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) <= maxRuleGroupscase (just returngroupInfosas 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 forgroupInfos[maxRuleGroups-1].
| } | ||
|
|
||
| func getRuleGroupNextToken(namespace, group string) string { | ||
| h := sha1.New() |
There was a problem hiding this comment.
Sorting by the hash effectively scrambles the rule group ordering in comparison to the normal / expected order that is returned without pagination and that the rule groups were written in by the user. Is there a good reason why we actually need the token to look like a hash value vs. just using the unhashed file + group name concatenation as a token that would preserve the original ordering?
| @@ -168,6 +169,17 @@ type apiFuncResult struct { | |||
|
|
|||
| type apiFunc func(r *http.Request) apiFuncResult | |||
|
|
|||
| type listRulesPaginationRequest struct { | |||
| MaxAlerts int32 | |||
There was a problem hiding this comment.
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.
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