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.

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.

storage/merge_test.go

storage/interface.go

web/api/v1/api.go

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.

storage/merge.go

web/api/v1/api.go

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.

storage/interface.go

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.

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)

machine424

lgtm so far, just some nits.

web/api/v1/api.go

web/api/v1/api_test.go

web/api/v1/api.go

machine424

thanks again!
looking forward to the implementation PR.
Let's see if it's ok for Bryan as well.

harry671003 · 2024-06-09T21:43:42Z

I took a stab at implementing the limits inside merge.go
I included those changes in the same PR as changes seemed minimal.
Could you take another look @machine424 and @bboreham ?

machine424 · 2024-06-19T06:36:05Z

I took a stab at implementing the limits inside merge.go I included those changes in the same PR as changes seemed minimal. Could you take another look @machine424 and @bboreham ?

I'd proceed as we agreed: merge the PR with the interface changes only (I already approved those) if no one has an objection, and then add the implementation in another PR. This will also keep reviewing easy.

harry671003 · 2024-06-19T17:06:21Z

I'd proceed as we agreed: merge the PR with the interface changes only (I already approved those) if no one has an objection, and then add the implementation in another PR.

I reverted back to the original. I can create a separate PR for the implementation. :)

web/api/v1/api.go

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

machine424

lgtm, Let's see if it's ok for Bryan.

web/api/v1/api.go

harry671003 · 2024-07-04T18:00:15Z

Could this PR be merged if there are no objections?

bboreham

Thanks!

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

storage/merge_test.go Outdated Show resolved Hide resolved

storage/interface.go Outdated Show resolved Hide resolved

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

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

storage/merge.go Show resolved Hide resolved

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

bboreham reviewed May 27, 2024

View reviewed changes

storage/interface.go Outdated Show resolved Hide resolved

storage/interface.go Show resolved Hide resolved

harry671003 force-pushed the pass_limit_to_querier branch 2 times, most recently 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

Closed

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

machine424 reviewed Jun 4, 2024

View reviewed changes

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

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

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

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

harry671003 force-pushed the pass_limit_to_querier branch from 0057a07 to 00e966a Compare June 5, 2024 17:58

machine424 reviewed Jun 6, 2024

View reviewed changes

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

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

harry671003 force-pushed the pass_limit_to_querier branch from 00e966a to 403214b Compare June 6, 2024 16:20

machine424 previously approved these changes Jun 6, 2024

View reviewed changes

harry671003 force-pushed the pass_limit_to_querier branch 3 times, most recently from 9b9d037 to edc8d0e Compare June 9, 2024 21:20

harry671003 force-pushed the pass_limit_to_querier branch from edc8d0e to 4ca2212 Compare June 9, 2024 21:26

harry671003 dismissed machine424’s stale review via 4ca2212 June 14, 2024 15:21

harry671003 force-pushed the pass_limit_to_querier branch from 14b8811 to 4a46dcf Compare June 19, 2024 17:03

harry671003 requested a review from machine424 June 19, 2024 17:08

bboreham reviewed Jun 20, 2024

View reviewed changes

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

Pass limit param as hint to storage.Querier

d5f6887

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

harry671003 force-pushed the pass_limit_to_querier branch from 4a46dcf to d5f6887 Compare June 20, 2024 16:47

harry671003 requested a review from bboreham June 21, 2024 14:43

machine424 previously approved these changes Jun 24, 2024

View reviewed changes

bboreham reviewed Jun 25, 2024

View reviewed changes

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

harry671003 dismissed machine424’s stale review via f251e35 June 25, 2024 15:28

harry671003 requested a review from bboreham June 25, 2024 15:28

harry671003 force-pushed the pass_limit_to_querier branch from f251e35 to d5f6887 Compare June 26, 2024 00:04

harry671003 requested a review from machine424 July 8, 2024 17:59

bboreham approved these changes Jul 12, 2024

View reviewed changes

bboreham merged commit d116bf7 into prometheus:main Jul 12, 2024

charleskorn mentioned this pull request Jul 17, 2024

Merge upstream at a28d197 grafana/mimir-prometheus#661

Merged

harry671003 mentioned this pull request Jul 22, 2024

Chore: Update prometheus, thanos and promql-engine cortexproject/cortex#6105

Merged

3 tasks

harry671003 deleted the pass_limit_to_querier branch July 22, 2024 21:37

This was referenced Jul 23, 2024

Upgrade Prometheus grafana/mimir#8780

Merged

Make use of LabelHints.Limit for label names and values requests grafana/mimir#8805

Closed

This was referenced Jul 30, 2024

Support Metadata API limit param in storage layer cortexproject/cortex#6127

Closed

Implement metadata API limit param thanos-io/thanos#7575

Open

harry671003 mentioned this pull request Aug 7, 2024

API: Add limit param in metadata APIs thanos-io/thanos#7609

Merged

2 tasks

krajorama mentioned this pull request Mar 11, 2025

[PERF] TSDB: Pass down label value limit into implementation #16158

Merged

storage: pass limit param as hint in querier #14109

storage: pass limit param as hint in querier #14109

Uh oh!

Conversation

harry671003 commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

Description

Uh oh!

machine424 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

machine424 commented May 17, 2024

Uh oh!

harry671003 commented May 21, 2024

Uh oh!

harry671003 commented May 21, 2024

Uh oh!

machine424 commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

harry671003 commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

machine424 commented May 28, 2024

Uh oh!

machine424 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

machine424 left a comment

Choose a reason for hiding this comment

Uh oh!

harry671003 commented Jun 9, 2024

Uh oh!

machine424 commented Jun 19, 2024

Uh oh!

harry671003 commented Jun 19, 2024

Uh oh!

Uh oh!

machine424 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harry671003 commented Jul 4, 2024

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harry671003 commented May 15, 2024 •

edited

Loading

machine424 left a comment •

edited

Loading

machine424 commented May 23, 2024 •

edited

Loading

harry671003 commented May 27, 2024 •

edited

Loading