-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage: pass limit param as hint in querier #14109
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
AFAIU, the limit hint isn't taken into account by the mergeGenericQuerier
implementation e.g.
For the Select
it's not that important as it streams, but we'll need that for the labels fcts.
We could create an issue for that and address it in another PR.
storage/merge_test.go
Outdated
@@ -1560,7 +1560,8 @@ func TestMergeGenericQuerierWithSecondaries_ErrorHandling(t *testing.T) { | |||
} | |||
}) | |||
t.Run("LabelNames", func(t *testing.T) { | |||
res, w, err := q.LabelNames(ctx) | |||
hints := &LabelHints{Limit: math.MaxInt} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only changes we should make to those tests is set the new arg hints=nil
and everything should keep working as before.
To test the limit hint, I suggest you add new tests/cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense. I would add more tests when I implement the limiting logic inside the mergeGenericQuerier
. I will create a follow up PR for that soon after this is merged.
@@ -692,7 +696,7 @@ func (api *API) labelNames(r *http.Request) apiFuncResult { | |||
if len(matcherSets) == 1 { | |||
matchers = matcherSets[0] | |||
} | |||
names, warnings, err = q.LabelNames(r.Context(), matchers...) | |||
names, warnings, err = q.LabelNames(r.Context(), hints, matchers...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the warnings logic, the if len(names) >= limit {
block in 709 is broken, #14116 is to fix that.
If that PR is merged before, those new blocks will become unreachable (if the implementations respect the limit, which is expected).
We'll need to think about how to handle that, maybe ask for limit+1
...
(same applies to labelsValues and series I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I think requesting for limit+1 makes sense to keep the changes minimal.
After this is in place, we could also make use of that limit hint for |
09dc94f
to
d3f5bae
Compare
@machine424 - Sorry, I didn't understand how the sample limit is relevant here. Could you elaborate more? |
Hey @bboreham, could you please take a look at this PR? |
We can, later, set the limit hint here prometheus/storage/remote/read_handler.go Lines 147 to 161 in 1081e33
h.remoteReadSampleLimit if it makes sense.
You can rebase as #14116 was merged. |
d3f5bae
to
1edec91
Compare
if err != nil { | ||
return nil, nil, fmt.Errorf("LabelValues() from merge generic querier for label %s: %w", name, err) | ||
} | ||
return res, ws, nil | ||
} | ||
|
||
// lvals performs merge sort for LabelValues from multiple queriers. | ||
func (q *mergeGenericQuerier) lvals(ctx context.Context, lq labelGenericQueriers, n string, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) { | ||
func (q *mergeGenericQuerier) lvals(ctx context.Context, lq labelGenericQueriers, n string, hints *LabelHints, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should only know about the limit int
as we do for the Select
's utils (we do not pass the whole SelectHints
to them).
Let's see what the others think about it.
web/api/v1/api.go
Outdated
@@ -658,6 +658,10 @@ func (api *API) labelNames(r *http.Request) apiFuncResult { | |||
return apiFuncResult{nil, &apiError{errorBadData, err}, nil, nil} | |||
} | |||
|
|||
hints := &storage.LabelHints{ | |||
Limit: limit + 1, // One more than the limit to ensure that warnings work correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually if no limit is set, limit
will be math.MaxInt
and this will overflow.
That case should be handled separately.
Maybe we can add a limitParamToLimitHint
util or sth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can align the meaning of "no limit" between them, it'd be even better, we can make parseLimitParam
consider 0 as the no limit value (as we do for the hint) + add some doc comments to state that, we'll need to turn the if len(...) > limit
checks into if limit >0 && len(...) > limit
(we can have an inline util for that), but at least in this way there is no confusion and no conversion to be done and the "no limit" can return more than 9*10^18
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read this correctly you didn't implement early exit when the limit was hit; just added the interface so that someone else could do that.
This seems wrong to me; if we're going to add this complexity we should make Prometheus better too.
storage/interface.go
Outdated
// Maximum number of results returned. | ||
// Use a value of 0 to disable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format this comment consistently with other members of the struct.
@@ -161,12 +161,12 @@ type LabelQuerier interface { | |||
// It is not safe to use the strings beyond the lifetime of the querier. | |||
// If matchers are specified the returned result set is reduced | |||
// to label values of metrics matching the matchers. | |||
LabelValues(ctx context.Context, name string, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) | |||
LabelValues(ctx context.Context, name string, hints *LabelHints, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this interface seems like quite a big step; it forces a change on all downstream implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing. I was also hesitant to change the interface initially. But, the only other way I could think of passing the limit to storage was through the context
.
I also noticed that the interface was changed in the past to add the matchers to the API.
Is this a blocker? Does it need more discussions with the maintainers?
I was afraid that including this change in this PR would make it big. I will create another PR soon after this to implement those changes. |
1edec91
to
9b7dd3e
Compare
Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
9b7dd3e
to
0057a07
Compare
Yes, we discussed that for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm so far, just some nits.
@@ -704,7 +708,7 @@ func (api *API) labelNames(r *http.Request) apiFuncResult { | |||
names = []string{} | |||
} | |||
|
|||
if len(names) > limit { | |||
if limit > 0 && len(names) > limit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you went with the first approach, I don't think we need these limit > 0
checks.
|
||
func getStorageLimitFromLimitParam(limit int) int { | ||
if limit < math.MaxInt { | ||
return limit + 1 // One more than the limit to ensure that warnings work correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return limit + 1 // One more than the limit to ensure that warnings work correctly. | |
return limit + 1 // One more than the limit to ensure that warnings are emitted when needed. |
{ | ||
name: "non empty label matcher with limit", | ||
matchers: []string{`{foo=~".+"}`}, | ||
expected: []string{"__name__", "abc"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that they'll always be the same?
u, err := url.Parse("http://example.com") | ||
require.NoError(t, err) | ||
q := u.Query() | ||
for _, matcher := range matchers { | ||
q.Add("match[]", matcher) | ||
} | ||
q.Add("limit", limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a limit!=""
check before this so we don't change the previous behavior. (before we weren't setting the limit param, now we're setting it to the empty string, they should be handled the same way, but it's better no to change existing tests)
Related Issues
Description
In #13396 a new limit param for the series, label names and label values APIs was introduced. This limit is currently only applied in the api.go. The storage implementations that do not support streaming such as Thanos, Cortex etc, will not be able to receive the limit param and would have to return all the results.
This change would pass the limit param as hints to storage so that the implementations can use that to return only the required number of results.
Next Steps
This PR is mainly changing the querier interface to allow passing the limit as a hint to the storage layer. As a next step, the limit hint must be taken into account by the
mergeGenericQuerier
implementation so that it can exit early.#14161