From 19f62152b26341e806430bc5df47d53a740014ef Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Mon, 30 Nov 2020 13:02:11 -0500 Subject: [PATCH 1/3] fix index out of bound bug when comparing ZLabelSets Signed-off-by: Ben Ye --- pkg/store/labelpb/label.go | 5 +-- pkg/store/labelpb/label_test.go | 57 +++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/pkg/store/labelpb/label.go b/pkg/store/labelpb/label.go index 36c5bfc139..ea0a7fa49b 100644 --- a/pkg/store/labelpb/label.go +++ b/pkg/store/labelpb/label.go @@ -307,7 +307,8 @@ func (z ZLabelSets) Less(i, j int) bool { l := 0 r := 0 var result int - for l < z[i].Size() && r < z[j].Size() { + lenI, lenJ := len(z[i].Labels), len(z[j].Labels) + for l < lenI && r < lenJ { result = z[i].Labels[l].Compare(z[j].Labels[r]) if result == 0 { l++ @@ -317,5 +318,5 @@ func (z ZLabelSets) Less(i, j int) bool { return result < 0 } - return l == z[i].Size() + return l == lenI } diff --git a/pkg/store/labelpb/label_test.go b/pkg/store/labelpb/label_test.go index ab8543657b..d8d258e8e1 100644 --- a/pkg/store/labelpb/label_test.go +++ b/pkg/store/labelpb/label_test.go @@ -129,6 +129,34 @@ func TestSortZLabelSets(t *testing.T) { }), ), }, + { + Labels: ZLabelsFromPromLabels( + labels.FromMap(map[string]string{ + "__name__": "grpc_client_handled_total", + "cluster": "test", + "grpc_code": "OK", + "aa": "1", + "bb": "2", + "cc": "3", + "dd": "4", + "ee": "5", + }), + ), + }, + { + Labels: ZLabelsFromPromLabels( + labels.FromMap(map[string]string{ + "__name__": "grpc_client_handled_total", + "cluster": "test", + "grpc_code": "OK", + "aa": "1", + "bb": "2", + "cc": "3", + "dd": "4", + "ee": "5", + }), + ), + }, { Labels: ZLabelsFromPromLabels( labels.FromMap(map[string]string{ @@ -188,6 +216,35 @@ func TestSortZLabelSets(t *testing.T) { }), ), }, + { + Labels: ZLabelsFromPromLabels( + labels.FromMap(map[string]string{ + "__name__": "grpc_client_handled_total", + "cluster": "test", + "grpc_code": "OK", + "aa": "1", + "bb": "2", + "cc": "3", + "dd": "4", + "ee": "5", + }), + ), + }, + // This label set is the same as the previous one, which should correctly return 0 in Less() function. + { + Labels: ZLabelsFromPromLabels( + labels.FromMap(map[string]string{ + "cluster": "test", + "__name__": "grpc_client_handled_total", + "grpc_code": "OK", + "aa": "1", + "bb": "2", + "cc": "3", + "dd": "4", + "ee": "5", + }), + ), + }, } sort.Sort(list) From 7e33c655e65af61b4b2fe53f716505eda9ea2240 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Mon, 30 Nov 2020 16:01:19 -0500 Subject: [PATCH 2/3] fix param parsing error message Signed-off-by: Ben Ye --- pkg/queryfrontend/labels_codec.go | 25 +++++++++++++++++++++---- pkg/queryfrontend/queryrange_codec.go | 6 +++--- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/pkg/queryfrontend/labels_codec.go b/pkg/queryfrontend/labels_codec.go index b4caed1cd5..b31ce6cd96 100644 --- a/pkg/queryfrontend/labels_codec.go +++ b/pkg/queryfrontend/labels_codec.go @@ -87,8 +87,25 @@ func (c labelsCodec) MergeResponse(responses ...queryrange.Response) (queryrange if len(responses) == 1 { return responses[0], nil } - seriesData := make(labelpb.ZLabelSets, 0) + i := 0 + var resp queryrange.Response + for _, response := range responses { + if len(response.(*ThanosSeriesResponse).Data) > 0 { + i++ + resp = response + } + if i > 1 { + break + } + } + // Fast path for the case that only one response contains series data and other responses are empty. + // We can just return that response to avoid deduplication and sorting. + if i == 1 { + return resp, nil + } + + seriesData := make(labelpb.ZLabelSets, 0) uniqueSeries := make(map[string]struct{}) for _, res := range responses { for _, series := range res.(*ThanosSeriesResponse).Data { @@ -287,7 +304,7 @@ func (c labelsCodec) parseLabelsRequest(r *http.Request, op string) (queryrange. return nil, err } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam]) + result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam], queryv1.StoreMatcherParam) if err != nil { return nil, err } @@ -321,7 +338,7 @@ func (c labelsCodec) parseSeriesRequest(r *http.Request) (queryrange.Request, er return nil, err } - result.Matchers, err = parseMatchersParam(r.Form[queryv1.MatcherParam]) + result.Matchers, err = parseMatchersParam(r.Form[queryv1.MatcherParam], queryv1.MatcherParam) if err != nil { return nil, err } @@ -340,7 +357,7 @@ func (c labelsCodec) parseSeriesRequest(r *http.Request) (queryrange.Request, er result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam] } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam]) + result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam], queryv1.StoreMatcherParam) if err != nil { return nil, err } diff --git a/pkg/queryfrontend/queryrange_codec.go b/pkg/queryfrontend/queryrange_codec.go index 004fb522d5..c85fe43309 100644 --- a/pkg/queryfrontend/queryrange_codec.go +++ b/pkg/queryfrontend/queryrange_codec.go @@ -111,7 +111,7 @@ func (c queryRangeCodec) DecodeRequest(_ context.Context, r *http.Request) (quer result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam] } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam]) + result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam], queryv1.StoreMatcherParam) if err != nil { return nil, err } @@ -221,12 +221,12 @@ func parsePartialResponseParam(s string, defaultEnablePartialResponse bool) (boo return defaultEnablePartialResponse, nil } -func parseMatchersParam(ss []string) ([][]*labels.Matcher, error) { +func parseMatchersParam(ss []string, matcherParam string) ([][]*labels.Matcher, error) { matchers := make([][]*labels.Matcher, 0, len(ss)) for _, s := range ss { ms, err := parser.ParseMetricSelector(s) if err != nil { - return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, queryv1.StoreMatcherParam) + return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, matcherParam) } matchers = append(matchers, ms) } From 7f56e7480d00dc830e8dc1aecc641db9d15ded9a Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Thu, 3 Dec 2020 09:37:59 -0500 Subject: [PATCH 3/3] address comment feedbacks Signed-off-by: Ben Ye --- pkg/queryfrontend/labels_codec.go | 23 +++-------------------- pkg/queryfrontend/queryrange_codec.go | 8 ++++---- 2 files changed, 7 insertions(+), 24 deletions(-) diff --git a/pkg/queryfrontend/labels_codec.go b/pkg/queryfrontend/labels_codec.go index b31ce6cd96..ed52bb254e 100644 --- a/pkg/queryfrontend/labels_codec.go +++ b/pkg/queryfrontend/labels_codec.go @@ -88,23 +88,6 @@ func (c labelsCodec) MergeResponse(responses ...queryrange.Response) (queryrange return responses[0], nil } - i := 0 - var resp queryrange.Response - for _, response := range responses { - if len(response.(*ThanosSeriesResponse).Data) > 0 { - i++ - resp = response - } - if i > 1 { - break - } - } - // Fast path for the case that only one response contains series data and other responses are empty. - // We can just return that response to avoid deduplication and sorting. - if i == 1 { - return resp, nil - } - seriesData := make(labelpb.ZLabelSets, 0) uniqueSeries := make(map[string]struct{}) for _, res := range responses { @@ -304,7 +287,7 @@ func (c labelsCodec) parseLabelsRequest(r *http.Request, op string) (queryrange. return nil, err } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam], queryv1.StoreMatcherParam) + result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam) if err != nil { return nil, err } @@ -338,7 +321,7 @@ func (c labelsCodec) parseSeriesRequest(r *http.Request) (queryrange.Request, er return nil, err } - result.Matchers, err = parseMatchersParam(r.Form[queryv1.MatcherParam], queryv1.MatcherParam) + result.Matchers, err = parseMatchersParam(r.Form, queryv1.MatcherParam) if err != nil { return nil, err } @@ -357,7 +340,7 @@ func (c labelsCodec) parseSeriesRequest(r *http.Request) (queryrange.Request, er result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam] } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam], queryv1.StoreMatcherParam) + result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam) if err != nil { return nil, err } diff --git a/pkg/queryfrontend/queryrange_codec.go b/pkg/queryfrontend/queryrange_codec.go index c85fe43309..3206c28be8 100644 --- a/pkg/queryfrontend/queryrange_codec.go +++ b/pkg/queryfrontend/queryrange_codec.go @@ -111,7 +111,7 @@ func (c queryRangeCodec) DecodeRequest(_ context.Context, r *http.Request) (quer result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam] } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam], queryv1.StoreMatcherParam) + result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam) if err != nil { return nil, err } @@ -221,9 +221,9 @@ func parsePartialResponseParam(s string, defaultEnablePartialResponse bool) (boo return defaultEnablePartialResponse, nil } -func parseMatchersParam(ss []string, matcherParam string) ([][]*labels.Matcher, error) { - matchers := make([][]*labels.Matcher, 0, len(ss)) - for _, s := range ss { +func parseMatchersParam(ss url.Values, matcherParam string) ([][]*labels.Matcher, error) { + matchers := make([][]*labels.Matcher, 0, len(ss[matcherParam])) + for _, s := range ss[matcherParam] { ms, err := parser.ParseMetricSelector(s) if err != nil { return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, matcherParam)