Fix cardinality and label names/values requests handling on POST requests

CHANGELOG.md

@@ -10,7 +10,7 @@
 * [CHANGE] Querier: `-query-frontend.cache-unaligned-requests` has been moved from a global flag to a per-tenant override. #5312
 * [CHANGE] Ingester: removed `cortex_ingester_shipper_dir_syncs_total` and `cortex_ingester_shipper_dir_sync_failures_total` metrics. The former metric was not much useful, and the latter was never incremented. #5396
 * [FEATURE] Cardinality API: Add a new `count_method` parameter which enables counting active series #5136
-* [FEATURE] Query-frontend: added experimental support to cache cardinality, label names and label values query responses. The cache will be used when `-query-frontend.cache-results` is enabled, and `-query-frontend.results-cache-ttl-for-cardinality-query` or `-query-frontend.results-cache-ttl-for-labels-query` set to a value greater than 0. The following metrics have been added to track the query results cache hit ratio per `request_type`: #5212 #5235 #5426
+* [FEATURE] Query-frontend: added experimental support to cache cardinality, label names and label values query responses. The cache will be used when `-query-frontend.cache-results` is enabled, and `-query-frontend.results-cache-ttl-for-cardinality-query` or `-query-frontend.results-cache-ttl-for-labels-query` set to a value greater than 0. The following metrics have been added to track the query results cache hit ratio per `request_type`: #5212 #5235 #5426 #5524
   * `cortex_frontend_query_result_cache_requests_total{request_type="query_range|cardinality|label_names_and_values"}`
   * `cortex_frontend_query_result_cache_hits_total{request_type="query_range|cardinality|label_names_and_values"}`
 * [FEATURE] Added `-<prefix>.s3.list-objects-version` flag to configure the S3 list objects version.

integration/querier_label_name_values_test.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/grafana/dskit/test"
 	"github.com/grafana/e2e"
+	e2ecache "github.com/grafana/e2e/cache"
 	e2edb "github.com/grafana/e2e/db"
 	"github.com/prometheus/prometheus/model/labels"
 	"github.com/prometheus/prometheus/prompb"
@@ -98,16 +99,26 @@ func TestQuerierLabelNamesAndValues(t *testing.T) {
 			require.NoError(t, err)
 			defer s.Close()
 
+			// Start dependencies.
+			memcached := e2ecache.NewMemcached()
+			consul := e2edb.NewConsul()
+			require.NoError(t, s.StartAndWaitReady(consul, memcached))
+
 			// Set configuration.
 			flags := mergeFlags(BlocksStorageFlags(), BlocksStorageS3Flags(), map[string]string{
 				"-querier.cardinality-analysis-enabled": "true",
 				"-ingester.ring.replication-factor":     "3",
+
+				// Enable the cardinality results cache with a very short TTL just to exercise its code.
+				"-query-frontend.cache-results":                           "true",
+				"-query-frontend.results-cache.backend":                   "memcached",
+				"-query-frontend.results-cache.memcached.addresses":       "dns+" + memcached.NetworkEndpoint(e2ecache.MemcachedPort),
+				"-query-frontend.results-cache-ttl-for-cardinality-query": "1ms",
 			})
 
-			// Start dependencies.
-			consul := e2edb.NewConsul()
+			// Start minio.
 			minio := e2edb.NewMinio(9000, flags["-blocks-storage.s3.bucket-name"])
-			require.NoError(t, s.StartAndWaitReady(consul, minio))
+			require.NoError(t, s.StartAndWaitReady(minio))
 
 			// Start the query-frontend.
 			queryFrontend := e2emimir.NewQueryFrontend("query-frontend", flags)

pkg/cardinality/request.go

@@ -5,6 +5,7 @@ package cardinality
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 
@@ -60,22 +61,26 @@ func (r *LabelNamesRequest) String() string {
 // DecodeLabelNamesRequest decodes the input http.Request into a LabelNamesRequest.
 // The input http.Request can either be a GET or POST with URL-encoded parameters.
 func DecodeLabelNamesRequest(r *http.Request) (*LabelNamesRequest, error) {
+	if err := r.ParseForm(); err != nil {
+		return nil, err
+	}
+
+	return DecodeLabelNamesRequestFromValues(r.Form)
+}
+
+// DecodeLabelNamesRequestFromValues is like DecodeLabelNamesRequest but takes url.Values in input.
+func DecodeLabelNamesRequestFromValues(values url.Values) (*LabelNamesRequest, error) {
 	var (
 		parsed = &LabelNamesRequest{}
 		err    error
 	)
 
-	err = r.ParseForm()
-	if err != nil {
-		return nil, err
-	}
-
-	parsed.Matchers, err = extractSelector(r)
+	parsed.Matchers, err = extractSelector(values)
 	if err != nil {
 		return nil, err
 	}
 
-	parsed.Limit, err = extractLimit(r)
+	parsed.Limit, err = extractLimit(values)
 	if err != nil {
 		return nil, err
 	}
@@ -126,31 +131,36 @@ func (r *LabelValuesRequest) String() string {
 // DecodeLabelValuesRequest decodes the input http.Request into a LabelValuesRequest.
 // The input http.Request can either be a GET or POST with URL-encoded parameters.
 func DecodeLabelValuesRequest(r *http.Request) (*LabelValuesRequest, error) {
+	if err := r.ParseForm(); err != nil {
+		return nil, err
+	}
+
+	return DecodeLabelValuesRequestFromValues(r.Form)
+}
+
+// DecodeLabelValuesRequestFromValues is like DecodeLabelValuesRequest but takes url.Values in input.
+func DecodeLabelValuesRequestFromValues(values url.Values) (*LabelValuesRequest, error) {
 	var (
 		parsed = &LabelValuesRequest{}
 		err    error
 	)
 
-	if err = r.ParseForm(); err != nil {
-		return nil, err
-	}
-
-	parsed.LabelNames, err = extractLabelNames(r)
+	parsed.LabelNames, err = extractLabelNames(values)
 	if err != nil {
 		return nil, err
 	}
 
-	parsed.Matchers, err = extractSelector(r)
+	parsed.Matchers, err = extractSelector(values)
 	if err != nil {
 		return nil, err
 	}
 
-	parsed.Limit, err = extractLimit(r)
+	parsed.Limit, err = extractLimit(values)
 	if err != nil {
 		return nil, err
 	}
 
-	parsed.CountMethod, err = extractCountMethod(r)
+	parsed.CountMethod, err = extractCountMethod(values)
 	if err != nil {
 		return nil, err
 	}
@@ -159,8 +169,8 @@ func DecodeLabelValuesRequest(r *http.Request) (*LabelValuesRequest, error) {
 }
 
 // extractSelector parses and gets selector query parameter containing a single matcher
-func extractSelector(r *http.Request) (matchers []*labels.Matcher, err error) {
-	selectorParams := r.Form["selector"]
+func extractSelector(values url.Values) (matchers []*labels.Matcher, err error) {
+	selectorParams := values["selector"]
 	if len(selectorParams) == 0 {
 		return nil, nil
 	}
@@ -187,8 +197,8 @@ func extractSelector(r *http.Request) (matchers []*labels.Matcher, err error) {
 }
 
 // extractLimit parses and validates request param `limit` if it's defined, otherwise returns default value.
-func extractLimit(r *http.Request) (limit int, err error) {
-	limitParams := r.Form["limit"]
+func extractLimit(values url.Values) (limit int, err error) {
+	limitParams := values["limit"]
 	if len(limitParams) == 0 {
 		return defaultLimit, nil
 	}
@@ -209,8 +219,8 @@ func extractLimit(r *http.Request) (limit int, err error) {
 }
 
 // extractLabelNames parses and gets label_names query parameter containing an array of label values
-func extractLabelNames(r *http.Request) ([]model.LabelName, error) {
-	labelNamesParams := r.Form["label_names[]"]
+func extractLabelNames(values url.Values) ([]model.LabelName, error) {
+	labelNamesParams := values["label_names[]"]
 	if len(labelNamesParams) == 0 {
 		return nil, fmt.Errorf("'label_names[]' param is required")
 	}
@@ -231,8 +241,8 @@ func extractLabelNames(r *http.Request) ([]model.LabelName, error) {
 }
 
 // extractCountMethod parses and validates request param `count_method` if it's defined, otherwise returns default value.
-func extractCountMethod(r *http.Request) (countMethod CountMethod, err error) {
-	countMethodParams := r.Form["count_method"]
+func extractCountMethod(values url.Values) (countMethod CountMethod, err error) {
+	countMethodParams := values["count_method"]
 	if len(countMethodParams) == 0 {
 		return defaultCountMethod, nil
 	}

pkg/cardinality/request_test.go

@@ -19,7 +19,7 @@ func TestDecodeLabelNamesRequest(t *testing.T) {
 		params = url.Values{
 			"selector": []string{`{second!="2",first="1"}`},
 			"limit":    []string{"100"},
-		}.Encode()
+		}
 
 		expected = &LabelNamesRequest{
 			Matchers: []*labels.Matcher{
@@ -30,8 +30,8 @@ func TestDecodeLabelNamesRequest(t *testing.T) {
 		}
 	)
 
-	t.Run("GET request", func(t *testing.T) {
-		req, err := http.NewRequest("GET", "http://localhost?"+params, nil)
+	t.Run("DecodeLabelNamesRequest() with GET request", func(t *testing.T) {
+		req, err := http.NewRequest("GET", "http://localhost?"+params.Encode(), nil)
 		require.NoError(t, err)
 
 		actual, err := DecodeLabelNamesRequest(req)
@@ -40,8 +40,8 @@ func TestDecodeLabelNamesRequest(t *testing.T) {
 		assert.Equal(t, expected, actual)
 	})
 
-	t.Run("POST request", func(t *testing.T) {
-		req, err := http.NewRequest("POST", "http://localhost/", strings.NewReader(params))
+	t.Run("DecodeLabelNamesRequest() with POST request", func(t *testing.T) {
+		req, err := http.NewRequest("POST", "http://localhost/", strings.NewReader(params.Encode()))
 		require.NoError(t, err)
 		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 
@@ -50,6 +50,13 @@ func TestDecodeLabelNamesRequest(t *testing.T) {
 
 		assert.Equal(t, expected, actual)
 	})
+
+	t.Run("DecodeLabelNamesRequestFromValues()", func(t *testing.T) {
+		actual, err := DecodeLabelNamesRequestFromValues(params)
+		require.NoError(t, err)
+
+		assert.Equal(t, expected, actual)
+	})
 }
 
 func TestLabelNamesRequest_String(t *testing.T) {
@@ -71,7 +78,7 @@ func TestDecodeLabelValuesRequest(t *testing.T) {
 			"label_names[]": []string{"metric_2", "metric_1"},
 			"count_method":  []string{"active"},
 			"limit":         []string{"100"},
-		}.Encode()
+		}
 
 		expected = &LabelValuesRequest{
 			LabelNames: []model.LabelName{"metric_1", "metric_2"},
@@ -84,9 +91,8 @@ func TestDecodeLabelValuesRequest(t *testing.T) {
 		}
 	)
 
-	t.Run("GET request", func(t *testing.T) {
-
-		req, err := http.NewRequest("GET", "http://localhost?"+params, nil)
+	t.Run("DecodeLabelValuesRequest() GET request", func(t *testing.T) {
+		req, err := http.NewRequest("GET", "http://localhost?"+params.Encode(), nil)
 		require.NoError(t, err)
 
 		actual, err := DecodeLabelValuesRequest(req)
@@ -95,8 +101,8 @@ func TestDecodeLabelValuesRequest(t *testing.T) {
 		assert.Equal(t, expected, actual)
 	})
 
-	t.Run("POST request", func(t *testing.T) {
-		req, err := http.NewRequest("POST", "http://localhost/", strings.NewReader(params))
+	t.Run("DecodeLabelValuesRequest() POST request", func(t *testing.T) {
+		req, err := http.NewRequest("POST", "http://localhost/", strings.NewReader(params.Encode()))
 		require.NoError(t, err)
 		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 
@@ -105,6 +111,13 @@ func TestDecodeLabelValuesRequest(t *testing.T) {
 
 		assert.Equal(t, expected, actual)
 	})
+
+	t.Run("DecodeLabelValuesRequestFromValues() GET request", func(t *testing.T) {
+		actual, err := DecodeLabelValuesRequestFromValues(params)
+		require.NoError(t, err)
+
+		assert.Equal(t, expected, actual)
+	})
 }
 
 func TestLabelValuesRequest_String(t *testing.T) {

pkg/frontend/querymiddleware/cardinality_query_cache.go

@@ -5,6 +5,7 @@ package querymiddleware
 import (
 	"errors"
 	"net/http"
+	"net/url"
 	"strings"
 	"time"
 
@@ -36,10 +37,10 @@ func (c *cardinalityQueryCache) getTTL(userID string) time.Duration {
 	return c.limits.ResultsCacheTTLForCardinalityQuery(userID)
 }
 
-func (c *cardinalityQueryCache) parseRequest(req *http.Request) (*genericQueryRequest, error) {
+func (c *cardinalityQueryCache) parseRequest(path string, values url.Values) (*genericQueryRequest, error) {
 	switch {
-	case strings.HasSuffix(req.URL.Path, cardinalityLabelNamesPathSuffix):
-		parsed, err := cardinality.DecodeLabelNamesRequest(req)
+	case strings.HasSuffix(path, cardinalityLabelNamesPathSuffix):
+		parsed, err := cardinality.DecodeLabelNamesRequestFromValues(values)
 		if err != nil {
 			return nil, err
 		}
@@ -48,8 +49,8 @@ func (c *cardinalityQueryCache) parseRequest(req *http.Request) (*genericQueryRe
 			cacheKey:       parsed.String(),
 			cacheKeyPrefix: cardinalityLabelNamesQueryCachePrefix,
 		}, nil
-	case strings.HasSuffix(req.URL.Path, cardinalityLabelValuesPathSuffix):
-		parsed, err := cardinality.DecodeLabelValuesRequest(req)
+	case strings.HasSuffix(path, cardinalityLabelValuesPathSuffix):
+		parsed, err := cardinality.DecodeLabelValuesRequestFromValues(values)
 		if err != nil {
 			return nil, err
 		}

pkg/frontend/querymiddleware/cardinality_query_cache_test.go

@@ -107,12 +107,14 @@ func TestCardinalityQueryCache_RoundTrip_WithTenantFederation(t *testing.T) {
 func TestCardinalityQueryCache_RoundTrip(t *testing.T) {
 	testGenericQueryCacheRoundTrip(t, newCardinalityQueryCacheRoundTripper, "cardinality", map[string]testGenericQueryCacheRequestType{
 		"label names request": {
-			url:            mustParseURL(t, `/prometheus/api/v1/cardinality/label_names?selector={job="test"}&limit=100`),
+			reqPath:        "/prometheus/api/v1/cardinality/label_names",
+			reqData:        url.Values{"selector": []string{`{job="test"}`}, "limit": []string{"100"}},
 			cacheKey:       "user-1:job=\"test\"\x00100",
 			hashedCacheKey: cardinalityLabelNamesQueryCachePrefix + cacheHashKey("user-1:job=\"test\"\x00100"),
 		},
 		"label values request": {
-			url:            mustParseURL(t, `/prometheus/api/v1/cardinality/label_values?selector={job="test"}&label_names[]=metric_1&label_names[]=metric_2&limit=100`),
+			reqPath:        "/prometheus/api/v1/cardinality/label_values",
+			reqData:        url.Values{"selector": []string{`{job="test"}`}, "label_names[]": []string{"metric_1", "metric_2"}, "limit": []string{"100"}},
 			cacheKey:       "user-1:metric_1\x01metric_2\x00job=\"test\"\x00inmemory\x00100",
 			hashedCacheKey: cardinalityLabelValuesQueryCachePrefix + cacheHashKey("user-1:metric_1\x01metric_2\x00job=\"test\"\x00inmemory\x00100"),
 		},

pkg/frontend/querymiddleware/generic_query_cache.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"net/url"
 	"time"
 
 	"github.com/go-kit/log"
@@ -14,6 +15,7 @@ import (
 	"github.com/grafana/dskit/tenant"
 
 	apierror "github.com/grafana/mimir/pkg/api/error"
+	"github.com/grafana/mimir/pkg/util"
 	"github.com/grafana/mimir/pkg/util/spanlogger"
 	"github.com/grafana/mimir/pkg/util/validation"
 )
@@ -28,8 +30,8 @@ type genericQueryRequest struct {
 }
 
 type genericQueryDelegate interface {
-	// parseRequest parses the input req and returns a genericQueryRequest, or an error if parsing fails.
-	parseRequest(req *http.Request) (*genericQueryRequest, error)
+	// parseRequest parses the input request and returns a genericQueryRequest, or an error if parsing fails.
+	parseRequest(path string, values url.Values) (*genericQueryRequest, error)
 
 	// getTTL returns the cache TTL for the input userID.
 	getTTL(userID string) time.Duration
@@ -80,7 +82,14 @@ func (c *genericQueryCache) RoundTrip(req *http.Request) (*http.Response, error)
 	}
 
 	// Decode the request.
-	queryReq, err := c.delegate.parseRequest(req)
+	reqValues, err := util.ParseRequestFormWithoutConsumingBody(req)
+	if err != nil {
+		// This is considered a non-recoverable error, so we return error instead of passing
+		// the request to the downstream.
+		return nil, apierror.New(apierror.TypeBadData, err.Error())
+	}
+
+	queryReq, err := c.delegate.parseRequest(req.URL.Path, reqValues)
 	if err != nil {
 		// Logging as info because it's not an actionable error here.
 		// We defer it to the downstream.