From 15f4951888ec026ed958f95263009c987172cbfc Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Tue, 8 Sep 2020 17:05:22 -0400 Subject: [PATCH] address review comments Signed-off-by: Ben Ye --- CHANGELOG.md | 2 +- pkg/api/query/v1.go | 30 +++++++++++------------ pkg/api/query/v1_test.go | 28 +++++++++++---------- pkg/queryfrontend/codec.go | 43 +++++++++++++++++---------------- pkg/queryfrontend/codec_test.go | 12 +++++---- pkg/store/proxy.go | 2 +- 6 files changed, 61 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e274dafd1ce..d96553846b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re ### Added -- [#3133](https://github.com/thanos-io/thanos/pull/3133) Query: Allow passing a `storeMatch[]` to Labels APIs. Also supports time range metadata based store filtering. +- [#3133](https://github.com/thanos-io/thanos/pull/3133) Query: Allow passing a `storeMatch[]` to Labels APIs. Also time range metadata based store filtering is supported on Labels APIs. ## [v0.15.0](https://github.com/thanos-io/thanos/releases) - 2020.09.07 diff --git a/pkg/api/query/v1.go b/pkg/api/query/v1.go index f3f7412f9d1..dbd76e067b7 100644 --- a/pkg/api/query/v1.go +++ b/pkg/api/query/v1.go @@ -53,11 +53,11 @@ import ( ) const ( - dedupParam = "dedup" - partialResponseParam = "partial_response" - maxSourceResolutionParam = "max_source_resolution" - replicaLabelsParam = "replicaLabels[]" - storeMatcherParam = "storeMatch[]" + DedupParam = "dedup" + PartialResponseParam = "partial_response" + MaxSourceResolutionParam = "max_source_resolution" + ReplicaLabelsParam = "replicaLabels[]" + StoreMatcherParam = "storeMatch[]" ) // QueryAPI is an API used by Thanos Query. @@ -149,11 +149,11 @@ type queryData struct { func (qapi *QueryAPI) parseEnableDedupParam(r *http.Request) (enableDeduplication bool, _ *api.ApiError) { enableDeduplication = true - if val := r.FormValue(dedupParam); val != "" { + if val := r.FormValue(DedupParam); val != "" { var err error enableDeduplication, err = strconv.ParseBool(val) if err != nil { - return false, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Wrapf(err, "'%s' parameter", dedupParam)} + return false, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Wrapf(err, "'%s' parameter", DedupParam)} } } return enableDeduplication, nil @@ -166,8 +166,8 @@ func (qapi *QueryAPI) parseReplicaLabelsParam(r *http.Request) (replicaLabels [] replicaLabels = qapi.replicaLabels // Overwrite the cli flag when provided as a query parameter. - if len(r.Form[replicaLabelsParam]) > 0 { - replicaLabels = r.Form[replicaLabelsParam] + if len(r.Form[ReplicaLabelsParam]) > 0 { + replicaLabels = r.Form[ReplicaLabelsParam] } return replicaLabels, nil @@ -178,7 +178,7 @@ func (qapi *QueryAPI) parseStoreMatchersParam(r *http.Request) (storeMatchers [] return nil, &api.ApiError{Typ: api.ErrorInternal, Err: errors.Wrap(err, "parse form")} } - for _, s := range r.Form[storeMatcherParam] { + for _, s := range r.Form[StoreMatcherParam] { matchers, err := parser.ParseMetricSelector(s) if err != nil { return nil, &api.ApiError{Typ: api.ErrorBadData, Err: err} @@ -196,7 +196,7 @@ func (qapi *QueryAPI) parseStoreMatchersParam(r *http.Request) (storeMatchers [] func (qapi *QueryAPI) parseDownsamplingParamMillis(r *http.Request, defaultVal time.Duration) (maxResolutionMillis int64, _ *api.ApiError) { maxSourceResolution := 0 * time.Second - val := r.FormValue(maxSourceResolutionParam) + val := r.FormValue(MaxSourceResolutionParam) if qapi.enableAutodownsampling || (val == "auto") { maxSourceResolution = defaultVal } @@ -204,12 +204,12 @@ func (qapi *QueryAPI) parseDownsamplingParamMillis(r *http.Request, defaultVal t var err error maxSourceResolution, err = parseDuration(val) if err != nil { - return 0, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Wrapf(err, "'%s' parameter", maxSourceResolutionParam)} + return 0, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Wrapf(err, "'%s' parameter", MaxSourceResolutionParam)} } } if maxSourceResolution < 0 { - return 0, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Errorf("negative '%s' is not accepted. Try a positive integer", maxSourceResolutionParam)} + return 0, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Errorf("negative '%s' is not accepted. Try a positive integer", MaxSourceResolutionParam)} } return int64(maxSourceResolution / time.Millisecond), nil @@ -217,11 +217,11 @@ func (qapi *QueryAPI) parseDownsamplingParamMillis(r *http.Request, defaultVal t func (qapi *QueryAPI) parsePartialResponseParam(r *http.Request, defaultEnablePartialResponse bool) (enablePartialResponse bool, _ *api.ApiError) { // Overwrite the cli flag when provided as a query parameter. - if val := r.FormValue(partialResponseParam); val != "" { + if val := r.FormValue(PartialResponseParam); val != "" { var err error defaultEnablePartialResponse, err = strconv.ParseBool(val) if err != nil { - return false, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Wrapf(err, "'%s' parameter", partialResponseParam)} + return false, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Wrapf(err, "'%s' parameter", PartialResponseParam)} } } return defaultEnablePartialResponse, nil diff --git a/pkg/api/query/v1_test.go b/pkg/api/query/v1_test.go index 96f50126a0f..8c19fcf71ed 100644 --- a/pkg/api/query/v1_test.go +++ b/pkg/api/query/v1_test.go @@ -1056,7 +1056,7 @@ func TestParseDownsamplingParamMillis(t *testing.T) { gate: gate.NewKeeper(nil).NewGate(4), } v := url.Values{} - v.Set(maxSourceResolutionParam, test.maxSourceResolutionParam) + v.Set(MaxSourceResolutionParam, test.maxSourceResolutionParam) r := http.Request{PostForm: v} // If no max_source_resolution is specified fit at least 5 samples between steps. @@ -1099,19 +1099,21 @@ func TestParseStoreMatchersParam(t *testing.T) { }}, }, } { - api := QueryAPI{ - gate: gate.NewKeeper(nil).NewGate(4), - } - v := url.Values{} - v.Set(storeMatcherParam, tc.storeMatchers) - r := &http.Request{PostForm: v} + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + api := QueryAPI{ + gate: gate.NewKeeper(nil).NewGate(4), + } + v := url.Values{} + v.Set(StoreMatcherParam, tc.storeMatchers) + r := &http.Request{PostForm: v} - storeMatchers, err := api.parseStoreMatchersParam(r) - if !tc.fail { - testutil.Assert(t, reflect.DeepEqual(storeMatchers, tc.result), "case %v: expected %v to be equal to %v", i, storeMatchers, tc.result) - } else { - testutil.NotOk(t, err) - } + storeMatchers, err := api.parseStoreMatchersParam(r) + if !tc.fail { + testutil.Equals(t, tc.result, storeMatchers) + } else { + testutil.NotOk(t, err) + } + }) } } diff --git a/pkg/queryfrontend/codec.go b/pkg/queryfrontend/codec.go index f3f1d200184..769ce0b1bcd 100644 --- a/pkg/queryfrontend/codec.go +++ b/pkg/queryfrontend/codec.go @@ -18,6 +18,7 @@ import ( "github.com/prometheus/prometheus/promql/parser" "github.com/weaveworks/common/httpgrpc" + queryv1 "github.com/thanos-io/thanos/pkg/api/query" "github.com/thanos-io/thanos/pkg/promclient" "github.com/thanos-io/thanos/pkg/store/storepb" ) @@ -83,7 +84,7 @@ func (c codec) DecodeRequest(_ context.Context, r *http.Request) (queryrange.Req return nil, errStepTooSmall } - result.Dedup, err = parseEnableDedupParam(r.FormValue("dedup")) + result.Dedup, err = parseEnableDedupParam(r.FormValue(queryv1.DedupParam)) if err != nil { return nil, err } @@ -92,22 +93,22 @@ func (c codec) DecodeRequest(_ context.Context, r *http.Request) (queryrange.Req result.AutoDownsampling = true result.MaxSourceResolution = result.Step / 5 } else { - result.MaxSourceResolution, err = parseDownsamplingParamMillis(r.FormValue("max_source_resolution")) + result.MaxSourceResolution, err = parseDownsamplingParamMillis(r.FormValue(queryv1.MaxSourceResolutionParam)) if err != nil { return nil, err } } - result.PartialResponse, err = parsePartialResponseParam(r.FormValue("partial_response"), c.partialResponse) + result.PartialResponse, err = parsePartialResponseParam(r.FormValue(queryv1.PartialResponseParam), c.partialResponse) if err != nil { return nil, err } - if len(r.Form["replicaLabels[]"]) > 0 { - result.ReplicaLabels = r.Form["replicaLabels[]"] + if len(r.Form[queryv1.ReplicaLabelsParam]) > 0 { + result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam] } - result.StoreMatchers, err = parseStoreMatchersParam(r.Form["storeMatch[]"]) + result.StoreMatchers, err = parseStoreMatchersParam(r.Form[queryv1.StoreMatcherParam]) if err != nil { return nil, err } @@ -131,21 +132,21 @@ func (c codec) EncodeRequest(ctx context.Context, r queryrange.Request) (*http.R return nil, httpgrpc.Errorf(http.StatusBadRequest, "invalid request format") } params := url.Values{ - "start": []string{encodeTime(thanosReq.Start)}, - "end": []string{encodeTime(thanosReq.End)}, - "step": []string{encodeDurationMillis(thanosReq.Step)}, - "query": []string{thanosReq.Query}, - "dedup": []string{strconv.FormatBool(thanosReq.Dedup)}, - "partial_response": []string{strconv.FormatBool(thanosReq.PartialResponse)}, - "replicaLabels[]": thanosReq.ReplicaLabels, + "start": []string{encodeTime(thanosReq.Start)}, + "end": []string{encodeTime(thanosReq.End)}, + "step": []string{encodeDurationMillis(thanosReq.Step)}, + "query": []string{thanosReq.Query}, + queryv1.DedupParam: []string{strconv.FormatBool(thanosReq.Dedup)}, + queryv1.PartialResponseParam: []string{strconv.FormatBool(thanosReq.PartialResponse)}, + queryv1.ReplicaLabelsParam: thanosReq.ReplicaLabels, } if thanosReq.AutoDownsampling { - params["max_source_resolution"] = []string{"auto"} + params[queryv1.MaxSourceResolutionParam] = []string{"auto"} } else if thanosReq.MaxSourceResolution != 0 { // Add this param only if it is set. Set to 0 will impact // auto-downsampling in the querier. - params["max_source_resolution"] = []string{encodeDurationMillis(thanosReq.MaxSourceResolution)} + params[queryv1.MaxSourceResolutionParam] = []string{encodeDurationMillis(thanosReq.MaxSourceResolution)} } if len(thanosReq.StoreMatchers) > 0 { @@ -153,7 +154,7 @@ func (c codec) EncodeRequest(ctx context.Context, r queryrange.Request) (*http.R if err != nil { return nil, httpgrpc.Errorf(http.StatusBadRequest, "invalid request format") } - params["storeMatch[]"] = storeMatchers + params[queryv1.StoreMatcherParam] = storeMatchers } u := &url.URL{ @@ -191,7 +192,7 @@ func parseEnableDedupParam(s string) (bool, error) { var err error enableDeduplication, err = strconv.ParseBool(s) if err != nil { - return enableDeduplication, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, "dedup") + return enableDeduplication, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, queryv1.DedupParam) } } @@ -204,7 +205,7 @@ func parseDownsamplingParamMillis(s string) (int64, error) { var err error maxSourceResolution, err = parseDurationMillis(s) if err != nil { - return maxSourceResolution, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, "max_source_resolution") + return maxSourceResolution, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, queryv1.MaxSourceResolutionParam) } } @@ -220,7 +221,7 @@ func parsePartialResponseParam(s string, defaultEnablePartialResponse bool) (boo var err error defaultEnablePartialResponse, err = strconv.ParseBool(s) if err != nil { - return defaultEnablePartialResponse, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, "partial_response") + return defaultEnablePartialResponse, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, queryv1.PartialResponseParam) } } @@ -232,11 +233,11 @@ func parseStoreMatchersParam(ss []string) ([][]storepb.LabelMatcher, error) { for _, s := range ss { matchers, err := parser.ParseMetricSelector(s) if err != nil { - return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, "storeMatch[]") + return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, queryv1.StoreMatcherParam) } stm, err := storepb.TranslatePromMatchers(matchers...) if err != nil { - return nil, httpgrpc.Errorf(http.StatusBadRequest, "storeMatch[]") + return nil, httpgrpc.Errorf(http.StatusBadRequest, queryv1.StoreMatcherParam) } storeMatchers = append(storeMatchers, stm) } diff --git a/pkg/queryfrontend/codec_test.go b/pkg/queryfrontend/codec_test.go index cb32ee6fc12..4797c6b8b73 100644 --- a/pkg/queryfrontend/codec_test.go +++ b/pkg/queryfrontend/codec_test.go @@ -10,10 +10,12 @@ import ( "testing" "github.com/cortexproject/cortex/pkg/querier/queryrange" + "github.com/weaveworks/common/httpgrpc" + + queryv1 "github.com/thanos-io/thanos/pkg/api/query" "github.com/thanos-io/thanos/pkg/compact" "github.com/thanos-io/thanos/pkg/store/storepb" "github.com/thanos-io/thanos/pkg/testutil" - "github.com/weaveworks/common/httpgrpc" ) func TestCodec_DecodeRequest(t *testing.T) { @@ -218,7 +220,7 @@ func TestCodec_EncodeRequest(t *testing.T) { return r.URL.Query().Get("start") == "123" && r.URL.Query().Get("end") == "456" && r.URL.Query().Get("step") == "1" && - r.URL.Query().Get("dedup") == "true" + r.URL.Query().Get(queryv1.DedupParam) == "true" }, }, { @@ -233,7 +235,7 @@ func TestCodec_EncodeRequest(t *testing.T) { return r.URL.Query().Get("start") == "123" && r.URL.Query().Get("end") == "456" && r.URL.Query().Get("step") == "1" && - r.URL.Query().Get("partial_response") == "true" + r.URL.Query().Get(queryv1.PartialResponseParam) == "true" }, }, { @@ -248,7 +250,7 @@ func TestCodec_EncodeRequest(t *testing.T) { return r.URL.Query().Get("start") == "123" && r.URL.Query().Get("end") == "456" && r.URL.Query().Get("step") == "1" && - r.URL.Query().Get("max_source_resolution") == "300" + r.URL.Query().Get(queryv1.MaxSourceResolutionParam) == "300" }, }, { @@ -263,7 +265,7 @@ func TestCodec_EncodeRequest(t *testing.T) { return r.URL.Query().Get("start") == "123" && r.URL.Query().Get("end") == "456" && r.URL.Query().Get("step") == "1" && - r.URL.Query().Get("max_source_resolution") == "3600" + r.URL.Query().Get(queryv1.MaxSourceResolutionParam) == "3600" }, }, } { diff --git a/pkg/store/proxy.go b/pkg/store/proxy.go index 1bc60b9f9f3..43fbee6b613 100644 --- a/pkg/store/proxy.go +++ b/pkg/store/proxy.go @@ -270,7 +270,7 @@ func (s *ProxyStore) Series(r *storepb.SeriesRequest, srv storepb.Store_SeriesSe ok, _ = storeMatches(st, r.MinTime, r.MaxTime, storeMatcher, r.Matchers...) }) if !ok { - storeDebugMsgs = append(storeDebugMsgs, fmt.Sprintf("Store %s filtered out", st)) + storeDebugMsgs = append(storeDebugMsgs, fmt.Sprintf("store %s filtered out", st)) continue } storeDebugMsgs = append(storeDebugMsgs, fmt.Sprintf("Store %s queried", st))