web/api add limit param #13396

kartikaysaxena · 2024-01-14T06:47:45Z

In continuation to the PR #12823

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena · 2024-01-14T07:18:37Z

Hey @GiedriusS! Can you please take a look at this pull request?

beorn7 · 2024-01-17T11:49:24Z

Requesting review from @bboreham as the author of the original proposal #12795 .

GiedriusS · 2024-01-17T12:20:49Z

web/api/v1/api.go

+
+func parseLimitParam(limitStr string) (limit int, err error) {
+	limit = math.MaxInt
+	if limitStr != "" {


Probably better to return early:

if limitStr == "" { return limit, nil }

So now you don't need to check != "". This is a Go style point: the code moves further to the left (is less indented) so is a little easier to read.

bboreham

Thanks for this PR. The code is nice and simple.

As an enhancement we could pass in the limit to the code that computes results, so we can stop early rather than creating the full result then truncating it. However I think it would be fine to merge this and then make it more efficient.

web/api/v1/api.go

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

bboreham · 2024-01-19T06:56:10Z

It just occurred to me: the caller might like to know if the results were truncated.

yeya24 · 2024-02-08T23:11:05Z

Any plan to move this forward?

kartikaysaxena · 2024-02-09T03:10:45Z

It just occurred to me: the caller might like to know if the results were truncated.

You mean should I add a warning to the response if the results were truncated because of the limit?

bboreham · 2024-02-09T08:06:27Z

You mean should I add a warning to the response if the results were truncated because of the limit?

Sounds reasonable.

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena · 2024-02-09T21:19:16Z

curl -g 'http://localhost:9090/api/v1/series?match[]=up&match[]=go_goroutines{job="example-app"}'

{"status":"success","data":[{"__name__":"go_goroutines","instance":"localhost:9090","job":"example-app"},{"__name__":"up","instance":"localhost:8080","job":"example-app"},{"__name__":"up","instance":"localhost:9090","job":"example-app"}]}%

curl -g 'http://localhost:9090/api/v1/series?match[]=up&match[]=go_goroutines{job="example-app"}&limit=1'

{"status":"success","data":[{"__name__":"go_goroutines","instance":"localhost:9090","job":"example-app"}],"warnings":["results truncated due to limit"]}%

It now shows the warnings in the response when the results are truncated

yeya24 · 2024-02-09T21:23:18Z

web/api/v1/api.go

@@ -783,6 +792,14 @@ func (api *API) labelValues(r *http.Request) (result apiFuncResult) {

 	slices.Sort(vals)

+	limit, err := parseLimitParam(r.FormValue("limit"))


Can we move this before we actually fetch data? So that we don't throw a 400 error after we finish fetching label values

yeya24 · 2024-02-09T21:23:28Z

web/api/v1/api.go

@@ -860,8 +877,19 @@ func (api *API) series(r *http.Request) (result apiFuncResult) {
 	}

 	metrics := []labels.Labels{}
+
+	limit, err := parseLimitParam(r.FormValue("limit"))


Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

web/api/v1/api.go

yeya24 · 2024-02-09T23:12:38Z

It might be out of scope of this pr.
Now the limit applies at the API layer, which is after fetching all requested data. Should we consider adding limit as part of the storage interface so that we can return less data from underlying TSDB?

For Series we can probably do it via SelectHints, but I am unsure about label names and label values API.

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

darshanime · 2024-02-10T05:41:27Z

the limit applies at the API layer, which is after fetching all requested data

Since we check for limits within the Next() loop, we don't really fetch all the series data? We do fetch all the labels though, but those are cheap enough due to an index read.
@bboreham wdyt?

yeya24 · 2024-02-10T07:23:25Z

@darshanime I think you are correct. However, that requires the underlying implementation to support streaming.
If the implementation doesn't support streaming, for example the remote read querier, or other implementations in downstream projects like Cortex or Thanos, then we overfetch data.

Having limit as part of the hints might help such usecases.

bboreham · 2024-02-10T09:48:41Z

Since we check for limits within the Next() loop, we don't really fetch all the series data? We do fetch all the labels though, but those are cheap enough due to an index read.

In some code paths the results are sorted before that loop. Sorting is O(N log N).

And that is not the only place where the limit could be applied.

kartikaysaxena · 2024-02-10T22:25:18Z

I think we may move this forward and then further iterate on it, at least for a start.

bboreham

Thanks, the code seems nearly done.

Since there was a mistake in the handling of warnings, you should add a test for that case.
Also, ideally, a test for an invalid limit parameter.

docs/querying/api.md

bboreham · 2024-02-12T10:14:00Z

web/api/v1/api.go

+
+func parseLimitParam(limitStr string) (limit int, err error) {
+	limit = math.MaxInt
+	if limitStr != "" {


So now you don't need to check != "". This is a Go style point: the code moves further to the left (is less indented) so is a little easier to read.

bboreham · 2024-02-12T10:17:09Z

web/api/v1/api.go

+	if len(vals) >= limit {
+		vals = vals[:limit]
+		warnings = annotations.New().Add(errors.New("results truncated due to limit"))
+		return apiFuncResult{vals, nil, warnings, closer}


This line seems unnecessary.

bboreham · 2024-02-12T10:19:35Z

web/api/v1/api.go

+	if len(names) >= limit {
+		names = names[:limit]
+		warnings := annotations.New().Add(errors.New("results truncated due to limit"))
+		return apiFuncResult{names, nil, warnings, nil}


This line seems unnecessary, since it is identical to the next line.

web/api/v1/api.go

yeya24 · 2024-02-14T05:06:27Z

I think we may move this forward and then further iterate on it, at least for a start.

Thanks. Yeah I think it is a good start.

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena · 2024-02-15T22:35:43Z

Hey @bboreham, sorry for the delay. I was busy with some academic engagements at my university, so I have made some changes; can you please take a look at them? Thanks for your guidance.

bboreham

I think this is very close now. Couple of comments remain from my previous review.

web/api/v1/api.go

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena · 2024-02-28T18:47:38Z

Was busy due to university minors, suggestions are implemented now, PTAL @bboreham

bboreham

Thanks, I think we can merge this now and iterate based on experience.

bboreham · 2024-02-29T15:32:21Z

BTW I added @rexagod as co-author in sign-off, since the original code came from #12823

dimitarvdimitrov · 2024-03-25T17:54:18Z

web/api/v1/api.go

@@ -703,6 +708,11 @@ func (api *API) labelNames(r *http.Request) apiFuncResult {
 	if names == nil {
 		names = []string{}
 	}
+
+	if len(names) >= limit {


shouldn't this check be len(names) == limit instead of >=? In this case the returned elements can be equal to the limit and also contain all elements in the database, but the response will still contain a warning. The same is used on the other two endpoints

you mean len(names) > limit?
I opened #14116 to fix that.
Thanks and don't hesitate to open an issue for such "issues":)

Add warnings count check to TestEndpoints The limit param was added in prometheus#13396 Signed-off-by: machine424 <ayoubmrini424@gmail.com>

for the the series, label names and label values APIs Add warnings count check to TestEndpoints The limit param was added in prometheus#13396 Signed-off-by: machine424 <ayoubmrini424@gmail.com>

for the the series, label names and label values APIs Add warnings count check to TestEndpoints The limit param was added in #13396 Signed-off-by: machine424 <ayoubmrini424@gmail.com>

kartikaysaxena added 2 commits January 14, 2024 12:15

web/api add limit param

7a021bd

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

lint fix

cda3049

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

beorn7 mentioned this pull request Jan 15, 2024

web/api: add limit parameter #12823

Closed

beorn7 requested a review from bboreham January 17, 2024 11:48

GiedriusS reviewed Jan 17, 2024

View reviewed changes

bboreham reviewed Jan 17, 2024

View reviewed changes

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

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

suggestions implemented

071af9d

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena requested review from GiedriusS and bboreham January 18, 2024 03:54

kartikaysaxena added 2 commits February 10, 2024 02:24

added warnings when results are truncated

8ea623d

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

lint

66ab452

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

yeya24 reviewed Feb 9, 2024

View reviewed changes

move logic upwards

641d665

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena requested a review from yeya24 February 9, 2024 22:06

yeya24 reviewed Feb 9, 2024

View reviewed changes

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

added warnings in label names and values API

a5f9b52

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena requested a review from yeya24 February 9, 2024 23:18

bboreham reviewed Feb 12, 2024

View reviewed changes

kartikaysaxena force-pushed the api_limit branch 2 times, most recently from eefa8f2 to c16f2d5 Compare February 15, 2024 22:28

suggestions implemented

140a6cb

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

kartikaysaxena force-pushed the api_limit branch from c16f2d5 to 140a6cb Compare February 15, 2024 22:33

bboreham reviewed Feb 19, 2024

View reviewed changes

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

suggestions implemented

2c88e61

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>

bboreham approved these changes Feb 29, 2024

View reviewed changes

bboreham merged commit 8736772 into prometheus:main Feb 29, 2024
24 checks passed

yeya24 mentioned this pull request Mar 1, 2024

Chore: Update Prometheus cortexproject/cortex#5795

Merged

3 tasks

bboreham mentioned this pull request Mar 6, 2024

API tests: fix flaky TestEndpoints #13714

Merged

dimitarvdimitrov reviewed Mar 25, 2024

View reviewed changes

NWRichmond mentioned this pull request Apr 1, 2024

Prometheus: Use limit parameter to manage metric names payloads grafana/grafana#85444

Open

This was referenced May 14, 2024

AutoComplete/Template Variables: Prometheus auto-complete and template variables can be very slow to load on large clusters grafana/grafana#87871

Open

storage: pass limit param as hint in querier #14109

Open

machine424 mentioned this pull request May 17, 2024

fix(api): Send warnings only if the limit is really exceeded #14116

Merged

harry671003 mentioned this pull request May 29, 2024

mergeGenericQuerier should exit early when the limit is hit #14161

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

web/api add limit param #13396

web/api add limit param #13396

kartikaysaxena commented Jan 14, 2024

kartikaysaxena commented Jan 14, 2024

beorn7 commented Jan 17, 2024

GiedriusS Jan 17, 2024

kartikaysaxena Jan 18, 2024

bboreham Feb 12, 2024

bboreham left a comment

bboreham commented Jan 19, 2024

yeya24 commented Feb 8, 2024

kartikaysaxena commented Feb 9, 2024

bboreham commented Feb 9, 2024

kartikaysaxena commented Feb 9, 2024 •

edited

Loading

yeya24 Feb 9, 2024

yeya24 Feb 9, 2024

yeya24 commented Feb 9, 2024

darshanime commented Feb 10, 2024

yeya24 commented Feb 10, 2024 •

edited

Loading

bboreham commented Feb 10, 2024

kartikaysaxena commented Feb 10, 2024

bboreham left a comment

bboreham Feb 12, 2024

bboreham Feb 12, 2024

bboreham Feb 12, 2024

yeya24 commented Feb 14, 2024

kartikaysaxena commented Feb 15, 2024

bboreham left a comment

kartikaysaxena commented Feb 28, 2024

bboreham left a comment

bboreham commented Feb 29, 2024

dimitarvdimitrov Mar 25, 2024

machine424 May 17, 2024

		@@ -783,6 +792,14 @@ func (api API) labelValues(r http.Request) (result apiFuncResult) {

		slices.Sort(vals)

		limit, err := parseLimitParam(r.FormValue("limit"))

web/api add limit param #13396

web/api add limit param #13396

Conversation

kartikaysaxena commented Jan 14, 2024

kartikaysaxena commented Jan 14, 2024

beorn7 commented Jan 17, 2024

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

bboreham commented Jan 19, 2024

yeya24 commented Feb 8, 2024

kartikaysaxena commented Feb 9, 2024

bboreham commented Feb 9, 2024

kartikaysaxena commented Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 commented Feb 9, 2024

darshanime commented Feb 10, 2024

yeya24 commented Feb 10, 2024 • edited Loading

bboreham commented Feb 10, 2024

kartikaysaxena commented Feb 10, 2024

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

Choose a reason for hiding this comment

yeya24 commented Feb 14, 2024

kartikaysaxena commented Feb 15, 2024

bboreham left a comment

Choose a reason for hiding this comment

kartikaysaxena commented Feb 28, 2024

bboreham left a comment

Choose a reason for hiding this comment

bboreham commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikaysaxena commented Feb 9, 2024 •

edited

Loading

yeya24 commented Feb 10, 2024 •

edited

Loading