Apply suggestions from code review

Co-authored-by: Arve Knudsen <[email protected]>
grafana · Jan 24, 2025 · 74686a7 · 74686a7
1 parent 69ea948
commit 74686a7
Show file tree

Hide file tree

Showing 5 changed files with 8 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,7 +9,6 @@
 * [CHANGE] Distributor: OTLP and push handler replace all non-UTF8 characters with the unicode replacement character `\uFFFD` in error messages before propagating them. #10236
 * [CHANGE] Querier: pass query matchers to queryable `IsApplicable` hook. #10256
 * [CHANGE] Build: removed Mimir Alpine Docker image and related CI tests. #10469
-* [CHANGE] Distributor, Ingester, StoreGateway: Make use of LabelHints.Limit for label names and values requests. #10410
 * [CHANGE] Query-frontend: Add `topic` label to `cortex_ingest_storage_strong_consistency_requests_total`, `cortex_ingest_storage_strong_consistency_failures_total`, and `cortex_ingest_storage_strong_consistency_wait_duration_seconds` metrics. #10220
 * [CHANGE] Ruler: cap the rate of retries for remote query evaluation to 170/sec. This is configurable via `-ruler.query-frontend.max-retries-rate`. #10375 #10403
 * [CHANGE] Query-frontend: Add `topic` label to `cortex_ingest_storage_reader_last_produced_offset_requests_total`, `cortex_ingest_storage_reader_last_produced_offset_failures_total`, `cortex_ingest_storage_reader_last_produced_offset_request_duration_seconds`, `cortex_ingest_storage_reader_partition_start_offset_requests_total`, `cortex_ingest_storage_reader_partition_start_offset_failures_total`, `cortex_ingest_storage_reader_partition_start_offset_request_duration_seconds` metrics. #10462

diff --git a/pkg/ingester/client/compat.go b/pkg/ingester/client/compat.go
@@ -116,7 +116,7 @@ func ToLabelValuesRequest(labelName model.LabelName, from, to model.Time, hints
 	}
 
 	var limit int64
-	if hints != nil {
+	if hints != nil && hints.Limit > 0 {
 		limit = int64(hints.Limit)
 	}
 	return &LabelValuesRequest{
@@ -156,7 +156,7 @@ func ToLabelNamesRequest(from, to model.Time, hints *storage.LabelHints, matcher
 	}
 
 	var limit int64
-	if hints != nil {
+	if hints != nil && hints.Limit > 0 {
 		limit = int64(hints.Limit)
 	}
 

diff --git a/pkg/querier/blocks_store_queryable.go b/pkg/querier/blocks_store_queryable.go
@@ -1187,7 +1187,7 @@ func createSeriesRequest(minT, maxT int64, matchers []storepb.LabelMatcher, skip
 
 func createLabelNamesRequest(minT, maxT int64, blockIDs []ulid.ULID, hints *storage.LabelHints, matchers []storepb.LabelMatcher) (*storepb.LabelNamesRequest, error) {
 	var limit int64
-	if hints != nil {
+	if hints != nil && hints.Limit > 0 {
 		limit = int64(hints.Limit)
 	}
 	req := &storepb.LabelNamesRequest{
@@ -1220,7 +1220,7 @@ func createLabelNamesRequest(minT, maxT int64, blockIDs []ulid.ULID, hints *stor
 
 func createLabelValuesRequest(minT, maxT int64, label string, blockIDs []ulid.ULID, hints *storage.LabelHints, matchers ...*labels.Matcher) (*storepb.LabelValuesRequest, error) {
 	var limit int64
-	if hints != nil {
+	if hints != nil && hints.Limit > 0 {
 		limit = int64(hints.Limit)
 	}
 

diff --git a/pkg/querier/distributor_queryable.go b/pkg/querier/distributor_queryable.go
@@ -226,7 +226,7 @@ func (q *distributorQuerier) LabelValues(ctx context.Context, name string, hints
 	return lvs, nil, err
 }
 
-func (q *distributorQuerier) LabelNames(ctx context.Context, labelHints *storage.LabelHints, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) {
+func (q *distributorQuerier) LabelNames(ctx context.Context, hints *storage.LabelHints, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) {
 	spanLog, ctx := spanlogger.NewWithLogger(ctx, q.logger, "distributorQuerier.LabelNames")
 	defer spanLog.Span.Finish()
 
@@ -244,7 +244,7 @@ func (q *distributorQuerier) LabelNames(ctx context.Context, labelHints *storage
 	now := time.Now().UnixMilli()
 	q.mint = clampMinTime(spanLog, q.mint, now, -queryIngestersWithin, "query ingesters within")
 
-	ln, err := q.distributor.LabelNames(ctx, model.Time(q.mint), model.Time(q.maxt), labelHints, matchers...)
+	ln, err := q.distributor.LabelNames(ctx, model.Time(q.mint), model.Time(q.maxt), hints, matchers...)
 	return ln, nil, err
 }
 

diff --git a/pkg/querier/distributor_queryable_test.go b/pkg/querier/distributor_queryable_test.go
@@ -644,7 +644,6 @@ func TestDistributorQuerier_LabelNames(t *testing.T) {
 	hints := &storage.LabelHints{Limit: 1}
 	labelNames := []string{"foo", "job"}
 
-	t.Run("querierLabelNames", func(t *testing.T) {
 		t.Run("with matchers", func(t *testing.T) {
 			d := &mockDistributor{}
 			d.On("LabelNames", mock.Anything, model.Time(mint), model.Time(maxt), &storage.LabelHints{}, someMatchers).
@@ -660,7 +659,7 @@ func TestDistributorQuerier_LabelNames(t *testing.T) {
 			assert.Equal(t, labelNames, names)
 		})
 
-		t.Run("with limit", func(t *testing.T) {
+		t.Run("with matchers and limit", func(t *testing.T) {
 			d := &mockDistributor{}
 			d.On("LabelNames", mock.Anything, model.Time(mint), model.Time(maxt), hints, []*labels.Matcher{}).
 				Return(labelNames[:hints.Limit], nil)
@@ -669,7 +668,7 @@ func TestDistributorQuerier_LabelNames(t *testing.T) {
 			querier, err := queryable.Querier(mint, maxt)
 			require.NoError(t, err)
 
-			names, warnings, err := querier.LabelNames(ctx, hints, []*labels.Matcher{}...)
+			names, warnings, err := querier.LabelNames(ctx, hints, someMatchers...)
 			require.NoError(t, err)
 			assert.Empty(t, warnings)
 			assert.Equal(t, labelNames[:hints.Limit], names)