added unit test and remoed query stats flags

Signed-off-by: someshkoli <[email protected]>

cmd/thanos/query.go

@@ -146,8 +146,6 @@ func registerQuery(app *extkingpin.App) {
 	enableMetricMetadataPartialResponse := cmd.Flag("metric-metadata.partial-response", "Enable partial response for metric metadata endpoint. --no-metric-metadata.partial-response for disabling.").
 		Hidden().Default("true").Bool()
 
-	enableQueryStats := cmd.Flag("query.stats", "Enable query stats in response.").Default("false").Bool()
-
 	defaultEvaluationInterval := extkingpin.ModelDuration(cmd.Flag("query.default-evaluation-interval", "Set default evaluation interval for sub queries.").Default("1m"))
 
 	defaultRangeQueryStep := extkingpin.ModelDuration(cmd.Flag("query.default-step", "Set default step for range queries. Default step is only used when step is not set in UI. In such cases, Thanos UI will use default step to calculate resolution (resolution = max(rangeSeconds / 250, defaultStep)). This will not work from Grafana, but Grafana has __step variable which can be used.").
@@ -254,7 +252,6 @@ func registerQuery(app *extkingpin.App) {
 			*enableRulePartialResponse,
 			*enableTargetPartialResponse,
 			*enableMetricMetadataPartialResponse,
-			*enableQueryStats,
 			fileSD,
 			time.Duration(*dnsSDInterval),
 			*dnsSDResolver,
@@ -315,7 +312,6 @@ func runQuery(
 	enableRulePartialResponse bool,
 	enableTargetPartialResponse bool,
 	enableMetricMetadataPartialResponse bool,
-	enableQueryStats bool,
 	fileSD *file.Discovery,
 	dnsSDInterval time.Duration,
 	dnsSDResolver string,
@@ -579,7 +575,6 @@ func runQuery(
 			enableRulePartialResponse,
 			enableTargetPartialResponse,
 			enableMetricMetadataPartialResponse,
-			enableQueryStats,
 			queryReplicaLabels,
 			flagsMap,
 			defaultRangeQueryStep,

docs/components/query.md

@@ -381,7 +381,6 @@ Flags:
                                  able to query without deduplication using
                                  'dedup=false' parameter. Data includes time
                                  series, recording rules, and alerting rules.
-      --query.stats              Enable query stats in response.
       --query.timeout=2m         Maximum time to process query by query node.
       --request.logging-config=<content>
                                  Alternative to 'request.logging-config-file'

pkg/api/query/v1.go

@@ -69,7 +69,7 @@ const (
 	MatcherParam             = "match[]"
 	StoreMatcherParam        = "storeMatch[]"
 	Step                     = "step"
-	Stat                     = "stat"
+	Stats                    = "stats"
 )
 
 // QueryAPI is an API used by Thanos Querier.
@@ -91,7 +91,6 @@ type QueryAPI struct {
 	enableTargetPartialResponse         bool
 	enableMetricMetadataPartialResponse bool
 	enableExemplarPartialResponse       bool
-	enableQueryStats                    bool
 	disableCORS                         bool
 
 	replicaLabels []string
@@ -119,7 +118,6 @@ func NewQueryAPI(
 	enableRulePartialResponse bool,
 	enableTargetPartialResponse bool,
 	enableMetricMetadataPartialResponse bool,
-	enableQueryStats bool,
 	replicaLabels []string,
 	flagsMap map[string]string,
 	defaultRangeQueryStep time.Duration,
@@ -145,7 +143,6 @@ func NewQueryAPI(
 		enableRulePartialResponse:              enableRulePartialResponse,
 		enableTargetPartialResponse:            enableTargetPartialResponse,
 		enableMetricMetadataPartialResponse:    enableMetricMetadataPartialResponse,
-		enableQueryStats:                       enableQueryStats,
 		replicaLabels:                          replicaLabels,
 		storeSet:                               storeSet,
 		defaultRangeQueryStep:                  defaultRangeQueryStep,
@@ -196,7 +193,7 @@ func (qapi *QueryAPI) Register(r *route.Router, tracer opentracing.Tracer, logge
 type queryData struct {
 	ResultType parser.ValueType  `json:"resultType"`
 	Result     parser.Value      `json:"result"`
-	Stat       *stats.QueryStats `json:"stats,omitempty"`
+	Stats      *stats.QueryStats `json:"stats,omitempty"`
 	// Additional Thanos Response field.
 	Warnings []error `json:"warnings,omitempty"`
 }
@@ -293,17 +290,6 @@ func (qapi *QueryAPI) parseStep(r *http.Request, defaultRangeQueryStep time.Dura
 	return d, nil
 }
 
-func (qapi *QueryAPI) parseStat(r *http.Request, defaultEnableStat bool) (bool, *api.ApiError) {
-	if val := r.FormValue(Stat); val != "" {
-		stat, err := strconv.ParseBool(val)
-		if err != nil {
-			return false, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Wrapf(err, "'%s' parameter", Stat)}
-		}
-		return stat, nil
-	}
-	return defaultEnableStat, nil
-}
-
 func (qapi *QueryAPI) query(r *http.Request) (interface{}, []error, *api.ApiError) {
 	ts, err := parseTimeParam(r, "time", qapi.baseAPI.Now())
 	if err != nil {
@@ -347,7 +333,6 @@ func (qapi *QueryAPI) query(r *http.Request) (interface{}, []error, *api.ApiErro
 		return nil, nil, apiErr
 	}
 
-	enableStat, err := qapi.parseStat(r, qapi.enableQueryStats)
 	if apiErr != nil {
 		return nil, nil, apiErr
 	}
@@ -386,13 +371,13 @@ func (qapi *QueryAPI) query(r *http.Request) (interface{}, []error, *api.ApiErro
 
 	// Optional stats field in response if parameter "stats" is not empty.
 	var qs *stats.QueryStats
-	if enableStat {
+	if r.FormValue(Stats) != "" {
 		qs = stats.NewQueryStats(qry.Stats())
 	}
 	return &queryData{
 		ResultType: res.Value.Type(),
 		Result:     res.Value,
-		Stat:       qs,
+		Stats:      qs,
 	}, res.Warnings, nil
 }
 
@@ -465,7 +450,6 @@ func (qapi *QueryAPI) queryRange(r *http.Request) (interface{}, []error, *api.Ap
 		return nil, nil, apiErr
 	}
 
-	enableStat, err := qapi.parseStat(r, qapi.enableQueryStats)
 	if apiErr != nil {
 		return nil, nil, apiErr
 	}
@@ -511,13 +495,13 @@ func (qapi *QueryAPI) queryRange(r *http.Request) (interface{}, []error, *api.Ap
 
 	// Optional stats field in response if parameter "stats" is not empty.
 	var qs *stats.QueryStats
-	if enableStat {
+	if r.FormValue(Stats) != "" {
 		qs = stats.NewQueryStats(qry.Stats())
 	}
 	return &queryData{
 		ResultType: res.Value.Type(),
 		Result:     res.Value,
-		Stat:       qs,
+		Stats:      qs,
 	}, res.Warnings, nil
 }
 

pkg/api/query/v1_test.go

@@ -44,6 +44,7 @@ import (
 	"github.com/prometheus/prometheus/storage"
 	"github.com/prometheus/prometheus/tsdb"
 	"github.com/prometheus/prometheus/tsdb/tsdbutil"
+	"github.com/prometheus/prometheus/util/stats"
 
 	"github.com/thanos-io/thanos/pkg/compact"
 
@@ -72,8 +73,21 @@ type endpointTestCase struct {
 	response interface{}
 	errType  baseAPI.ErrorType
 }
+type responeCompareFunction func(interface{}, interface{}) bool
 
-func testEndpoint(t *testing.T, test endpointTestCase, name string) bool {
+// Does a deep compare for two http responses.
+func deepCompare(a interface{}, b interface{}) bool {
+	return reflect.DeepEqual(a, b)
+}
+
+// Checks if both responses have Stats present or not.
+func lookupStats(a interface{}, b interface{}) bool {
+	ra := a.(*queryData)
+	rb := b.(*queryData)
+	return (ra.Stats == nil && rb.Stats == nil) || (ra.Stats != nil && rb.Stats != nil)
+}
+
+func testEndpoint(t *testing.T, test endpointTestCase, name string, responseCompareFunc responeCompareFunction) bool {
 	return t.Run(name, func(t *testing.T) {
 		// Build a context with the correct request params.
 		ctx := context.Background()
@@ -115,7 +129,7 @@ func testEndpoint(t *testing.T, test endpointTestCase, name string) bool {
 			t.Fatalf("Expected error of type %q but got none", test.errType)
 		}
 
-		if !reflect.DeepEqual(resp, test.response) {
+		if !responseCompareFunc(resp, test.response) {
 			t.Fatalf("Response does not match, expected:\n%+v\ngot:\n%+v", test.response, resp)
 		}
 	})
@@ -597,7 +611,35 @@ func TestQueryEndpoints(t *testing.T) {
 	}
 
 	for i, test := range tests {
-		if ok := testEndpoint(t, test, fmt.Sprintf("#%d %s", i, test.query.Encode())); !ok {
+		if ok := testEndpoint(t, test, fmt.Sprintf("#%d %s", i, test.query.Encode()), deepCompare); !ok {
+			return
+		}
+	}
+
+	tests = []endpointTestCase{
+		{
+			endpoint: api.query,
+			query: url.Values{
+				"query": []string{"2"},
+				"time":  []string{"123.4"},
+			},
+			response: &queryData{},
+		},
+		{
+			endpoint: api.query,
+			query: url.Values{
+				"query": []string{"2"},
+				"time":  []string{"123.4"},
+				"stats": []string{"true"},
+			},
+			response: &queryData{
+				Stats: &stats.QueryStats{},
+			},
+		},
+	}
+
+	for i, test := range tests {
+		if ok := testEndpoint(t, test, fmt.Sprintf("#%d %s", i, test.query.Encode()), lookupStats); !ok {
 			return
 		}
 	}
@@ -1158,7 +1200,7 @@ func TestMetadataEndpoints(t *testing.T) {
 	}
 
 	for i, test := range tests {
-		if ok := testEndpoint(t, test, strings.TrimSpace(fmt.Sprintf("#%d %s", i, test.query.Encode()))); !ok {
+		if ok := testEndpoint(t, test, strings.TrimSpace(fmt.Sprintf("#%d %s", i, test.query.Encode())), deepCompare); !ok {
 			return
 		}
 	}