querier: allow to enforce series query request limit #10748
Conversation
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
narqo
force-pushed
the
vldmr/series-api-limit
branch
from
86c9af2 to
e78d660
Compare
|
💻 Deploy preview available: https://deploy-preview-mimir-10748-zb444pucvq-vp.a.run.app/docs/mimir/latest/ |
Signed-off-by: Vladimir Varankin <[email protected]>
| if sp.Limit == 0 && sp.Limit != limit { | ||
| defer func() { | ||
| var warning annotations.Annotations | ||
| warning.Add(errors.New("results may be truncated due to limit")) |
There was a problem hiding this comment.
can we add more details - what is the limit and what was the requested limit
There was a problem hiding this comment.
This only adds a warning when the request didn't send a limit. My thinking was that when I pass a ?limit= explicitly, Prometheus adds its own warning to the response (ref code). I will expand the message
There was a problem hiding this comment.
oh, i misread && as ||, so I assumed this will add a warning when we clamp the limit. Do you want to do that too?
There was a problem hiding this comment.
I'm not sure. We don't do anything like that when we clamp other request parameters. Should we do that for the limit only? So I thought we will warn the user when they didn't pass the limit, but the limit was enforced — that feels the most unexpected scenario for them.
There was a problem hiding this comment.
i agree that putting a limit when the requester intended to get unlimited results is most surprising.
But also getting N results when you limited to N+1 would imply there are only N results. That's also a lie and somewhat surprising.
I think it's not a bad idea to add a warning when we clamp the start/end times (I suppose that's what you meant when you said "other request parameter"). I can do that next week
| if sp.Limit == 0 && sp.Limit != limit { | ||
| defer func() { | ||
| var warning annotations.Annotations | ||
| warning.Add(errors.New("results may be truncated due to limit")) |
There was a problem hiding this comment.
Creating a new comment to not derail the conversation you're having with Dimitar. I suggest to be more verbose and clarify what we did, e.g. "the request had no limit, but a default limit of XXX has been enforced" (or something similar).
| # querier. If the requested limit is outside the allowed value, the request will | ||
| # not fail but will be manipulated to only query data up to allowed limit. 0 to | ||
| # disable. |
There was a problem hiding this comment.
| # querier. If the requested limit is outside the allowed value, the request will | |
| # not fail but will be manipulated to only query data up to allowed limit. 0 to | |
| # disable. | |
| # querier. If the requested limit is outside of the allowed value, the request doesn't | |
| # fail, but is manipulated to only query data up to the allowed limit. Set to `0` to | |
| # disable. |
| @@ -3635,6 +3635,13 @@ The `limits` block configures default and per-tenant limits imposed by component | |||
| # CLI flag: -store.max-labels-query-length | |||
| [max_labels_query_length: <duration> | default = 0s] | |||
|
|
|||
| # Maximum number of items, series queries. This limit is enforced in the | |||
There was a problem hiding this comment.
I'm a little confused by the phrasing here. Is this the maximum number of series queries permitted? If so, we can remove the items and just say Maximum number of series queries.
What this PR does
Following #10620 #10652
This PR adds a new config and its related per-tenant override (
max_series_query_limit), that allows to enforce the limit of a/seriesAPI endpoint.Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.