storage: pass limit param as hint in querier #14109

harry671003 · 2024-05-15T20:09:10Z

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

machine424

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.

machine424 · 2024-05-17T20:21:14Z

storage/merge_test.go

@@ -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}


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.

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.

storage/interface.go

machine424 · 2024-05-17T20:21:53Z

web/api/v1/api.go

@@ -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...)


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).

Good catch. I think requesting for limit+1 makes sense to keep the changes minimal.

machine424 · 2024-05-17T20:49:45Z

After this is in place, we could also make use of that limit hint for storage.remote.read-sample-limit https://github.com/prometheus/prometheus/pull/4532/files

harry671003 · 2024-05-21T17:26:38Z

After this is in place, we could also make use of that limit hint for storage.remote.read-sample-limit https://github.com/prometheus/prometheus/pull/4532/files

@machine424 - Sorry, I didn't understand how the sample limit is relevant here. Could you elaborate more?

harry671003 · 2024-05-21T17:29:12Z

Hey @bboreham, could you please take a look at this PR?

machine424 · 2024-05-23T15:52:45Z

After this is in place, we could also make use of that limit hint for storage.remote.read-sample-limit https://github.com/prometheus/prometheus/pull/4532/files

@machine424 - Sorry, I didn't understand how the sample limit is relevant here. Could you elaborate more?

We can, later, set the limit hint here

prometheus/storage/remote/read_handler.go

Lines 147 to 161 in 1081e33

    
           var hints *storage.SelectHints 
        
           if query.Hints != nil { 
        
           	hints = &storage.SelectHints{ 
        
           		Start:    query.Hints.StartMs, 
        
           		End:      query.Hints.EndMs, 
        
           		Step:     query.Hints.StepMs, 
        
           		Func:     query.Hints.Func, 
        
           		Grouping: query.Hints.Grouping, 
        
           		Range:    query.Hints.RangeMs, 
        
           		By:       query.Hints.By, 
        
           	} 
        
           } 
        
           var ws annotations.Annotations 
        
           resp.Results[i], ws, err = ToQueryResult(querier.Select(ctx, false, hints, filteredMatchers...), h.remoteReadSampleLimit)

in accordance with h.remoteReadSampleLimit if it makes sense.

You can rebase as #14116 was merged.

machine424 · 2024-05-24T19:57:34Z

storage/merge.go

 	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) {


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.

machine424 · 2024-05-24T20:32:37Z

web/api/v1/api.go

@@ -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.


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.

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 :)

bboreham

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.

bboreham · 2024-05-27T15:40:11Z

storage/interface.go

+	// Maximum number of results returned.
+	// Use a value of 0 to disable.


Please format this comment consistently with other members of the struct.

bboreham · 2024-05-27T15:41:04Z

storage/interface.go

@@ -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)


Changing this interface seems like quite a big step; it forces a change on all downstream implementations.

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.

eg: Add matchers to LabelValues() call #8400

Is this a blocker? Does it need more discussions with the maintainers?

harry671003 · 2024-05-27T17:10:23Z

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.

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.

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>

machine424 · 2024-05-28T08:50:59Z

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.

Yes, we discussed that for mergeGenericQuerier here #14109 (review), the labels related ones would be more needed as that implementation's Select is an iteration. Did we miss any other implementation?
(If we agree on that, let's mention that in the PR description, open en issue and link it there as well)

harry671003 requested review from roidelapluie, jesusvazquez, cstyan, bwplotka and tomwilkie as code owners May 15, 2024 20:09

machine424 requested changes May 17, 2024

View reviewed changes

harry671003 force-pushed the pass_limit_to_querier branch from 09dc94f to d3f5bae Compare May 21, 2024 17:19

harry671003 force-pushed the pass_limit_to_querier branch from d3f5bae to 1edec91 Compare May 23, 2024 17:33

harry671003 requested a review from machine424 May 23, 2024 17:34

machine424 reviewed May 24, 2024

View reviewed changes

bboreham reviewed May 27, 2024

View reviewed changes

harry671003 force-pushed the pass_limit_to_querier branch from 1edec91 to 9b7dd3e Compare May 27, 2024 17:44

Pass limit param as hint to storage.Querier

0057a07

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>

harry671003 force-pushed the pass_limit_to_querier branch from 9b7dd3e to 0057a07 Compare May 27, 2024 19:01

harry671003 mentioned this pull request May 29, 2024

mergeGenericQuerier should exit early when the limit is hit #14161

Open

harry671003 requested a review from bboreham May 29, 2024 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: pass limit param as hint in querier #14109

storage: pass limit param as hint in querier #14109

harry671003 commented May 15, 2024 •

edited

machine424 left a comment •

edited

machine424 May 17, 2024

harry671003 May 21, 2024

machine424 May 17, 2024

harry671003 May 21, 2024

machine424 commented May 17, 2024

harry671003 commented May 21, 2024

harry671003 commented May 21, 2024

machine424 commented May 23, 2024 •

edited

machine424 May 24, 2024

machine424 May 24, 2024

machine424 May 24, 2024

bboreham left a comment

bboreham May 27, 2024

bboreham May 27, 2024

harry671003 May 27, 2024 •

edited

harry671003 commented May 27, 2024 •

edited

machine424 commented May 28, 2024

		// Maximum number of results returned.
		// Use a value of 0 to disable.

storage: pass limit param as hint in querier #14109

Are you sure you want to change the base?

storage: pass limit param as hint in querier #14109

Conversation

harry671003 commented May 15, 2024 • edited

Related Issues

Description

Next Steps

machine424 left a comment • edited

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

machine424 commented May 17, 2024

harry671003 commented May 21, 2024

harry671003 commented May 21, 2024

machine424 commented May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harry671003 May 27, 2024 • edited

Choose a reason for hiding this comment

harry671003 commented May 27, 2024 • edited

machine424 commented May 28, 2024

harry671003 commented May 15, 2024 •

edited

machine424 left a comment •

edited

machine424 commented May 23, 2024 •

edited

harry671003 May 27, 2024 •

edited

harry671003 commented May 27, 2024 •

edited