Apply suggestions from code review

Co-authored-by: Marco Pracucci <[email protected]>
Signed-off-by: Tyler Reid <[email protected]>

Author: 2 people

CHANGELOG.md

@@ -18,9 +18,7 @@
   * `cortex_alertmanager_state_persist_failed_total`
 * [ENHANCEMENT] Blocks storage: support ingesting exemplars.  Enabled by setting new CLI flag `-blocks-storage.tsdb.max-exemplars=<n>` or config option `blocks_storage.tsdb.max_exemplars` to positive value. #4124
 * [ENHANCEMENT] Distributor: Added distributors ring status section in the admin page. #4151
-* [ENHANCEMENT] Ingester/Block Storage: Added ingester and block storage support for `max_series_per_query` / `-ingester.max-series-per-query`
-  If cortex is running in chunk mode the series limit is only supported for fetched series from the ingester. If cortex is in blocks mode this
-  this limit is for fetched series from block storage and ingester. #4179
+* [ENHANCEMENT] Querier: Added `-ingester.max-series-per-query` support for blocks storage. When Cortex is running with blocks storage, the limit is enforced in the querier and applies both to data received from ingesters and store-gateway (long-term storage). #4179
 * [BUGFIX] Purger: fix `Invalid null value in condition for column range` caused by `nil` value in range for WriteBatch query. #4128
 * [BUGFIX] Ingester: fixed infrequent panic caused by a race condition between TSDB mmap-ed head chunks truncation and queries. #4176
 

docs/configuration/arguments.md

@@ -484,8 +484,8 @@ Valid per-tenant limits are (with their corresponding flags for default values):
 
 - `max_series_per_query` / `-ingester.max-series-per-query`
 
-    When running Cortex chunks storage: limit enforced when fetching metrics from ingesters only.  
-    When running Cortex blocks storage: limit enforced when fetching metrics both from ingesters and store-gateways.
+    When running Cortex chunks storage: limit enforced in the ingesters only and it's a per-instance limit.
+    When running Cortex blocks storage: limit enforced in the queriers both on samples fetched from ingesters and store-gateways (long-term storage).
 
 - `max_samples_per_query` / `-ingester.max-samples-per-query`
 

pkg/distributor/distributor_test.go

@@ -963,7 +963,7 @@ func TestDistributor_QueryStream_ShouldReturnErrorIfMaxSeriesPerQueryLimitIsReac
 	})
 	defer stopAll(ds, r)
 
-	// Push a number of series below the max series limit. Each series has 1 sample,
+	// Push a number of series below the max series limit. Each series has 1 sample.
 	initialSeries := maxSeriesLimit
 	writeReq := makeWriteRequest(0, initialSeries, 0)
 	writeRes, err := ds[0].Push(ctx, writeReq)

pkg/util/limiter/query_limiter.go

@@ -29,7 +29,7 @@ var (
 	errMaxSeriesHit = "The query hit the max number of series limit while fetching chunks %s (limit: %d)"
 )
 
-// NewQueryLimiter makes a new per-query rate limiter. Each per-query limiter
+// NewQueryLimiter makes a new per-query limiter. Each per-query limiter
 // is configured using the `maxSeriesPerQuery` and `maxChunkBytesPerQuery` limits.
 func NewQueryLimiter(maxSeriesPerQuery int) *QueryLimiter {
 	return &QueryLimiter{
@@ -50,8 +50,8 @@ func AddQueryLimiterToContext(ctx context.Context, limiter *QueryLimiter) contex
 	return context.WithValue(ctx, qlCtxKey, limiter)
 }
 
-// QueryLimiterFromContextWithFallback returns a Query Limiter from the current context.
-// IF there is Per Query Limiter on the context we will return a new no-op limiter
+// QueryLimiterFromContextWithFallback returns a QueryLimiter from the current context.
+// If there is not a QueryLimiter on the context it will return a new no-op limiter.
 func QueryLimiterFromContextWithFallback(ctx context.Context) *QueryLimiter {
 	ql, ok := ctx.Value(qlCtxKey).(*QueryLimiter)
 	if !ok {
@@ -61,9 +61,7 @@ func QueryLimiterFromContextWithFallback(ctx context.Context) *QueryLimiter {
 	return ql
 }
 
-// AddSeries Add labels for series to the count of unique series. If the
-// added series label causes us to go over the limit of maxSeriesPerQuery we will
-// return a validation error
+// AddSeries adds the input series and returns an error if the limit is reached.
 func (ql *QueryLimiter) AddSeries(labelAdapter []cortexpb.LabelAdapter, matchers []*labels.Matcher) error {
 	// If the max series is unlimited just return without managing map
 	if ql.maxSeriesPerQuery == 0 {

pkg/util/limiter/query_limiter_test.go

@@ -11,7 +11,7 @@ import (
 	"github.com/cortexproject/cortex/pkg/cortexpb"
 )
 
-func TestPerQueryLimiter_AddFingerPrint(t *testing.T) {
+func TestQueryLimiter_AddSeries_ShouldReturnNoErrorOnLimitNotExceeded(t *testing.T) {
 	const (
 		metricName = "test_metric"
 	)
@@ -35,10 +35,9 @@ func TestPerQueryLimiter_AddFingerPrint(t *testing.T) {
 	err := limiter.AddSeries(cortexpb.FromLabelsToLabelAdapters(series2), matchers)
 	assert.Equal(t, 2, limiter.UniqueSeries())
 	assert.Nil(t, err)
-
 }
 
-func TestPerQueryLimiter_AddFingerPrintExceedLimit(t *testing.T) {
+func TestQueryLimiter_AddSeriers_ShouldReturnErrorOnLimitExceeded(t *testing.T) {
 	const (
 		metricName = "test_metric"
 	)
@@ -59,12 +58,12 @@ func TestPerQueryLimiter_AddFingerPrintExceedLimit(t *testing.T) {
 		limiter = NewQueryLimiter(1)
 	)
 	err := limiter.AddSeries(cortexpb.FromLabelsToLabelAdapters(series1), matchers)
-	assert.Equal(t, nil, err)
+	require.NoError(t, err)
 	err = limiter.AddSeries(cortexpb.FromLabelsToLabelAdapters(series2), matchers)
 	require.Error(t, err)
 }
 
-func BenchmarkPerQueryLimiter_AddFingerPrint(b *testing.B) {
+func BenchmarkQueryLimiter_AddSeries(b *testing.B) {
 	const (
 		metricName = "test_metric"
 	)