query-frontend: pass on `limit` parameter for label names and values requests #7722

dimitarvdimitrov · 2024-03-25T17:51:11Z

What this PR does

This PR passes on the limit parameter from requests using the new prometheus codec types. The new codec is only used for caching logic at present, so limit wasn't being ignored on requests.

However, previously cache keys wouldn't include the parameter and there would be cache pollution. This PR includes limit in cache keys to fix that bug.

Which issue(s) this PR fixes or relates to

Fixes #7721

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

56quarters

LGTM with some non-blocking comments.

56quarters · 2024-03-28T16:21:31Z

pkg/frontend/querymiddleware/codec.go

@@ -311,12 +313,18 @@ func (prometheusCodec) DecodeLabelsQueryRequest(_ context.Context, r *http.Reque

 	labelMatcherSets := reqValues["match[]"]

+	limit, err := strconv.ParseInt(reqValues.Get("limit"), 10, 64)


Any reason to not use ParseUint() here since a limit of less than 0 doesn't really make sense? Does Prometheus accept negative values?

Any reason to not use ParseUint() here since a limit of less than 0 doesn't really make sense?

no reason. I changed the unit of the limit to uint64 throughout

Does Prometheus accept negative values?

no it doesn't

mimir/vendor/github.com/prometheus/prometheus/web/api/v1/api.go

Lines 1898 to 1913 in 7408575

func parseLimitParam(limitStr string) (limit int, err error) {

limit = math.MaxInt

if limitStr == "" {

return limit, nil

}

limit, err = strconv.Atoi(limitStr)

if err != nil {

return limit, err

}

if limit <= 0 {

return limit, errors.New("limit must be positive")

}

return limit, nil

}

pkg/frontend/querymiddleware/codec.go

56quarters · 2024-03-28T16:25:23Z

pkg/frontend/querymiddleware/codec.go

 	if IsLabelNamesQuery(r.URL.Path) {
 		return &PrometheusLabelNamesQueryRequest{
 			Path:             r.URL.Path,
 			Start:            start,
 			End:              end,
 			LabelMatcherSets: labelMatcherSets,
+			Limit:            limit,


IIUC, passing the limit value no matter what means we rely on whatever ParseInt() returns in the error case when there's no limit. Would it be better (more obvious) to set a default value above and conditionally call ParseInt()?

Makes sense.

this made me realize that I had missed setting the limit parameter on the HTTP request which the query-frontend creates (after processing the protobuf model). This wasn't a problem because the protobufs were only being used in caching logic (so no encoding into HTTP happens). Fixed in the last commit 663121c and added some more tests to make sure this happens properly

…requests This PR passes on the `limit` parameter from requests. It also includes it in cache keys if it's present Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

dimitarvdimitrov · 2024-04-03T15:27:54Z

thanks for the review @56quarters, do you want to take another look or can I merge on green?

dimitarvdimitrov · 2024-04-03T15:28:48Z

pkg/frontend/querymiddleware/codec_test.go

@@ -205,7 +206,7 @@ func TestLabelsQueryRequest(t *testing.T) {
 			expectedGetEndOrDefault:   1708588800 * 1e3,
 		},
 		{
-			name: "label names with start timestamp, no end timestamp, multiple matcher sets",
+			name: "label names with start and end timestamp, multiple matcher sets",


noticed that the test description here and below doesn't match the actual test case. Confirmed with @francoposa that it's the description that's wrong not the implementation

…on-labels-requests

56quarters

LGTM, sorry for the delay

…-parameter-on-labels-requests # Conflicts: # pkg/frontend/querymiddleware/model.pb.go # pkg/frontend/querymiddleware/model.proto

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

dimitarvdimitrov requested a review from a team as a code owner March 25, 2024 17:51

56quarters approved these changes Mar 28, 2024

View reviewed changes

dimitarvdimitrov added 4 commits April 3, 2024 15:55

query-frontend: pass on limit parameter for label names and values …

53f5f4f

…requests This PR passes on the `limit` parameter from requests. It also includes it in cache keys if it's present Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Add CHANGELOG.md entry

d750bb4

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Use uint64 for limit

81b3329

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Make limit setting more explicit

663121c

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

dimitarvdimitrov force-pushed the dimitar/query-frontend/pass-limit-parameter-on-labels-requests branch from 9a99a24 to 663121c Compare April 3, 2024 15:26

dimitarvdimitrov commented Apr 3, 2024

View reviewed changes

Merge branch 'main' into dimitar/query-frontend/pass-limit-parameter-…

67982cb

…on-labels-requests

dimitarvdimitrov enabled auto-merge (squash) April 5, 2024 10:29

56quarters approved these changes Apr 10, 2024

View reviewed changes

dimitarvdimitrov added 2 commits April 12, 2024 14:56

Merge branch 'refs/heads/main' into dimitar/query-frontend/pass-limit…

f566012

…-parameter-on-labels-requests # Conflicts: # pkg/frontend/querymiddleware/model.pb.go # pkg/frontend/querymiddleware/model.proto

Add Limit to structs

dbd10c2

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

dimitarvdimitrov merged commit 24cfe31 into main Apr 12, 2024
29 checks passed

dimitarvdimitrov deleted the dimitar/query-frontend/pass-limit-parameter-on-labels-requests branch April 12, 2024 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

query-frontend: pass on `limit` parameter for label names and values requests #7722

query-frontend: pass on `limit` parameter for label names and values requests #7722

dimitarvdimitrov commented Mar 25, 2024 •

edited

Loading

56quarters left a comment

56quarters Mar 28, 2024

dimitarvdimitrov Apr 3, 2024

56quarters Mar 28, 2024

dimitarvdimitrov Apr 3, 2024 •

edited

Loading

dimitarvdimitrov commented Apr 3, 2024

dimitarvdimitrov Apr 3, 2024

56quarters left a comment

		@@ -311,12 +313,18 @@ func (prometheusCodec) DecodeLabelsQueryRequest(_ context.Context, r *http.Reque

		labelMatcherSets := reqValues["match[]"]

		limit, err := strconv.ParseInt(reqValues.Get("limit"), 10, 64)

	func parseLimitParam(limitStr string) (limit int, err error) {
	limit = math.MaxInt
	if limitStr == "" {
	return limit, nil
	}

	limit, err = strconv.Atoi(limitStr)
	if err != nil {
	return limit, err
	}
	if limit <= 0 {
	return limit, errors.New("limit must be positive")
	}

	return limit, nil
	}

query-frontend: pass on limit parameter for label names and values requests #7722

query-frontend: pass on limit parameter for label names and values requests #7722

Conversation

dimitarvdimitrov commented Mar 25, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

56quarters left a comment

Choose a reason for hiding this comment

56quarters Mar 28, 2024

Choose a reason for hiding this comment

dimitarvdimitrov Apr 3, 2024

Choose a reason for hiding this comment

56quarters Mar 28, 2024

Choose a reason for hiding this comment

dimitarvdimitrov Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

dimitarvdimitrov commented Apr 3, 2024

dimitarvdimitrov Apr 3, 2024

Choose a reason for hiding this comment

56quarters left a comment

Choose a reason for hiding this comment

query-frontend: pass on `limit` parameter for label names and values requests #7722

query-frontend: pass on `limit` parameter for label names and values requests #7722

dimitarvdimitrov commented Mar 25, 2024 •

edited

Loading

dimitarvdimitrov Apr 3, 2024 •

edited

Loading