From 663121cb394ca9ba79a906589e3a2bfef7218b65 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Wed, 3 Apr 2024 17:26:00 +0200 Subject: [PATCH] Make limit setting more explicit Signed-off-by: Dimitar Dimitrov --- pkg/frontend/querymiddleware/codec.go | 15 ++++-- pkg/frontend/querymiddleware/codec_test.go | 59 ++++++++++++++++++++-- pkg/frontend/querymiddleware/model.pb.go | 4 +- pkg/frontend/querymiddleware/model.proto | 4 +- 4 files changed, 70 insertions(+), 12 deletions(-) diff --git a/pkg/frontend/querymiddleware/codec.go b/pkg/frontend/querymiddleware/codec.go index 6f6bc3dabd3..4407aae85aa 100644 --- a/pkg/frontend/querymiddleware/codec.go +++ b/pkg/frontend/querymiddleware/codec.go @@ -313,9 +313,12 @@ func (prometheusCodec) DecodeLabelsQueryRequest(_ context.Context, r *http.Reque labelMatcherSets := reqValues["match[]"] - limit, err := strconv.ParseUint(reqValues.Get("limit"), 10, 64) - if reqValues.Has("limit") && err != nil { - return nil, apierror.New(apierror.TypeBadData, "limit parameter: "+err.Error()) + limit := uint64(0) // 0 means unlimited + if limitStr := reqValues.Get("limit"); limitStr != "" { + limit, err = strconv.ParseUint(limitStr, 10, 64) + if err != nil || limit == 0 { + return nil, apierror.New(apierror.TypeBadData, fmt.Sprintf("limit parameter must be a positive number: %s", limitStr)) + } } if IsLabelNamesQuery(r.URL.Path) { @@ -535,6 +538,9 @@ func (c prometheusCodec) EncodeLabelsQueryRequest(ctx context.Context, req Label if len(req.GetLabelMatcherSets()) > 0 { urlValues["match[]"] = req.GetLabelMatcherSets() } + if req.GetLimit() > 0 { + urlValues["limit"] = []string{strconv.FormatUint(req.GetLimit(), 10)} + } u = &url.URL{ Path: req.Path, RawQuery: urlValues.Encode(), @@ -552,6 +558,9 @@ func (c prometheusCodec) EncodeLabelsQueryRequest(ctx context.Context, req Label if len(req.GetLabelMatcherSets()) > 0 { urlValues["match[]"] = req.GetLabelMatcherSets() } + if req.GetLimit() > 0 { + urlValues["limit"] = []string{strconv.FormatUint(req.GetLimit(), 10)} + } u = &url.URL{ Path: req.Path, // path still contains label name RawQuery: urlValues.Encode(), diff --git a/pkg/frontend/querymiddleware/codec_test.go b/pkg/frontend/querymiddleware/codec_test.go index ca99d08ecd1..79f041e2bbd 100644 --- a/pkg/frontend/querymiddleware/codec_test.go +++ b/pkg/frontend/querymiddleware/codec_test.go @@ -121,7 +121,8 @@ func TestLabelsQueryRequest(t *testing.T) { expectedGetLabelName string expectedGetStartOrDefault int64 expectedGetEndOrDefault int64 - expectedErr error + expectedErr string + expectedLimit uint64 }{ { name: "label names with start and end timestamps, no matcher sets", @@ -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", url: "/api/v1/labels?end=1708588800&match%5B%5D=go_goroutines%7Bcontainer%3D~%22quer.%2A%22%7D&match%5B%5D=go_goroutines%7Bcontainer%21%3D%22query-scheduler%22%7D&start=1708502400", expectedStruct: &PrometheusLabelNamesQueryRequest{ Path: "/api/v1/labels", @@ -221,7 +222,7 @@ func TestLabelsQueryRequest(t *testing.T) { expectedGetEndOrDefault: 1708588800 * 1e3, }, { - name: "label values with start timestamp, no end timestamp, multiple matcher sets", + name: "label values with start and end timestamp, multiple matcher sets", url: "/api/v1/label/job/values?end=1708588800&match%5B%5D=go_goroutines%7Bcontainer%3D~%22quer.%2A%22%7D&match%5B%5D=go_goroutines%7Bcontainer%21%3D%22query-scheduler%22%7D&start=1708502400", expectedStruct: &PrometheusLabelValuesQueryRequest{ Path: "/api/v1/label/job/values", @@ -237,6 +238,53 @@ func TestLabelsQueryRequest(t *testing.T) { expectedGetStartOrDefault: 1708502400 * 1e3, expectedGetEndOrDefault: 1708588800 * 1e3, }, + { + name: "label names with start and end timestamp, multiple matcher sets, limit", + url: "/api/v1/labels?end=1708588800&limit=10&match%5B%5D=go_goroutines%7Bcontainer%3D~%22quer.%2A%22%7D&match%5B%5D=go_goroutines%7Bcontainer%21%3D%22query-scheduler%22%7D&start=1708502400", + expectedStruct: &PrometheusLabelNamesQueryRequest{ + Path: "/api/v1/labels", + Start: 1708502400 * 1e3, + End: 1708588800 * 1e3, + Limit: 10, + LabelMatcherSets: []string{ + "go_goroutines{container=~\"quer.*\"}", + "go_goroutines{container!=\"query-scheduler\"}", + }, + }, + expectedGetLabelName: "", + expectedLimit: 10, + expectedGetStartOrDefault: 1708502400 * 1e3, + expectedGetEndOrDefault: 1708588800 * 1e3, + }, + { + name: "label values with start and end timestamp, multiple matcher sets, limit", + url: "/api/v1/label/job/values?end=1708588800&limit=10&match%5B%5D=go_goroutines%7Bcontainer%3D~%22quer.%2A%22%7D&match%5B%5D=go_goroutines%7Bcontainer%21%3D%22query-scheduler%22%7D&start=1708502400", + expectedStruct: &PrometheusLabelValuesQueryRequest{ + Path: "/api/v1/label/job/values", + LabelName: "job", + Start: 1708502400 * 1e3, + End: 1708588800 * 1e3, + Limit: 10, + LabelMatcherSets: []string{ + "go_goroutines{container=~\"quer.*\"}", + "go_goroutines{container!=\"query-scheduler\"}", + }, + }, + expectedGetLabelName: "job", + expectedLimit: 10, + expectedGetStartOrDefault: 1708502400 * 1e3, + expectedGetEndOrDefault: 1708588800 * 1e3, + }, + { + name: "zero limit is not allowed", + url: "/api/v1/label/job/values?limit=0", + expectedErr: "limit parameter must be a positive number: 0", + }, + { + name: "negative limit is not allowed", + url: "/api/v1/label/job/values?limit=-1", + expectedErr: "limit parameter must be a positive number: -1", + }, } { t.Run(testCase.name, func(t *testing.T) { for _, reqMethod := range []string{http.MethodGet, http.MethodPost} { @@ -261,13 +309,14 @@ func TestLabelsQueryRequest(t *testing.T) { r = r.WithContext(ctx) reqDecoded, err := codec.DecodeLabelsQueryRequest(ctx, r) - if err != nil || testCase.expectedErr != nil { - require.EqualValues(t, testCase.expectedErr, err) + if err != nil || testCase.expectedErr != "" { + require.EqualError(t, err, testCase.expectedErr) return } require.EqualValues(t, testCase.expectedStruct, reqDecoded) require.EqualValues(t, testCase.expectedGetStartOrDefault, reqDecoded.GetStartOrDefault()) require.EqualValues(t, testCase.expectedGetEndOrDefault, reqDecoded.GetEndOrDefault()) + require.EqualValues(t, testCase.expectedLimit, reqDecoded.GetLimit()) reqEncoded, err := codec.EncodeLabelsQueryRequest(context.Background(), reqDecoded) require.NoError(t, err) diff --git a/pkg/frontend/querymiddleware/model.pb.go b/pkg/frontend/querymiddleware/model.pb.go index d9bdbeeb9f1..1789ca3cf36 100644 --- a/pkg/frontend/querymiddleware/model.pb.go +++ b/pkg/frontend/querymiddleware/model.pb.go @@ -372,7 +372,7 @@ type PrometheusLabelNamesQueryRequest struct { LabelMatcherSets []string `protobuf:"bytes,4,rep,name=labelMatcherSets,proto3" json:"labelMatcherSets,omitempty"` // ID of the request used to correlate downstream requests and responses. Id int64 `protobuf:"varint,5,opt,name=id,proto3" json:"id,omitempty"` - // Limit the number of label names returned. + // Limit the number of label names returned. A value of 0 means no limit. Limit uint64 `protobuf:"varint,6,opt,name=limit,proto3" json:"limit,omitempty"` } @@ -462,7 +462,7 @@ type PrometheusLabelValuesQueryRequest struct { LabelMatcherSets []string `protobuf:"bytes,5,rep,name=labelMatcherSets,proto3" json:"labelMatcherSets,omitempty"` // ID of the request used to correlate downstream requests and responses. Id int64 `protobuf:"varint,6,opt,name=id,proto3" json:"id,omitempty"` - // Limit the number of label values returned. + // Limit the number of label values returned. A value of 0 means no limit. Limit uint64 `protobuf:"varint,7,opt,name=limit,proto3" json:"limit,omitempty"` } diff --git a/pkg/frontend/querymiddleware/model.proto b/pkg/frontend/querymiddleware/model.proto index 74d1d624919..9ea1f42e84b 100644 --- a/pkg/frontend/querymiddleware/model.proto +++ b/pkg/frontend/querymiddleware/model.proto @@ -81,7 +81,7 @@ message PrometheusLabelNamesQueryRequest { // ID of the request used to correlate downstream requests and responses. int64 id = 5; - // Limit the number of label names returned. + // Limit the number of label names returned. A value of 0 means no limit. uint64 limit = 6; } @@ -102,7 +102,7 @@ message PrometheusLabelValuesQueryRequest { // ID of the request used to correlate downstream requests and responses. int64 id = 6; - // Limit the number of label values returned. + // Limit the number of label values returned. A value of 0 means no limit. uint64 limit = 7; }