Store: fix label values edge case

If the requested label is an external label and we have series matchers
we should only return results if the series matchers actually match a
series.

Signed-off-by: Michael Hoffmann <[email protected]>

CHANGELOG.md

@@ -12,6 +12,8 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 
 ### Fixed
 
+- [7082](https://github.com/thanos-io/thanos/pull/7082) Store: fix label values edge case when requesting external label values with matchers
+
 ### Added
 
 ### Changed

pkg/store/acceptance_test.go

@@ -255,6 +255,29 @@ func testStoreAPIsAcceptance(t *testing.T, startStore startStoreFn) {
 				},
 			},
 		},
+		{
+			desc: "series matcher on other labels when requesting external labels",
+			appendFn: func(app storage.Appender) {
+				_, err := app.Append(0, labels.FromStrings("__name__", "up", "foo", "bar", "job", "C"), 0, 0)
+				testutil.Ok(t, err)
+				_, err = app.Append(0, labels.FromStrings("__name__", "up", "foo", "baz", "job", "C"), 0, 0)
+				testutil.Ok(t, err)
+
+				testutil.Ok(t, app.Commit())
+			},
+			labelValuesCalls: []labelValuesCallCase{
+				{
+					start: timestamp.FromTime(minTime),
+					end:   timestamp.FromTime(maxTime),
+					label: "region",
+					matchers: []storepb.LabelMatcher{
+						{Type: storepb.LabelMatcher_EQ, Name: "__name__", Value: "up"},
+						{Type: storepb.LabelMatcher_EQ, Name: "job", Value: "C"},
+					},
+					expectedValues: []string{"eu-west"},
+				},
+			},
+		},
 		{
 			// Testcases taken from https://github.com/prometheus/prometheus/blob/95e705612c1d557f1681bd081a841b78f93ee158/tsdb/querier_test.go#L1898
 			desc: "matching behavior",

pkg/store/bucket.go

@@ -1947,8 +1947,9 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR
 			continue
 		}
 
-		// If we have series matchers, add <labelName> != "" matcher, to only select series that have given label name.
-		if len(reqSeriesMatchersNoExtLabels) > 0 {
+		// If we have series matchers and the Label is not an external one, add <labelName> != "" matcher
+		// to only select series that have given label name.
+		if len(reqSeriesMatchersNoExtLabels) > 0 && !b.extLset.Has(req.Label) {
 			m, err := labels.NewMatcher(labels.MatchNotEqual, req.Label, "")
 			if err != nil {
 				return nil, status.Error(codes.InvalidArgument, err.Error())

pkg/store/prometheus.go

@@ -108,6 +108,11 @@ func NewPrometheusStore(
 	return p, nil
 }
 
+func (p *PrometheusStore) labelCallsSupportMatchers() bool {
+	version, parseErr := semver.Parse(p.promVersion())
+	return parseErr == nil && version.GTE(baseVer)
+}
+
 // Info returns store information about the Prometheus instance.
 // NOTE(bwplotka): MaxTime & MinTime are not accurate nor adjusted dynamically.
 // This is fine for now, but might be needed in future.
@@ -656,8 +661,7 @@ func (p *PrometheusStore) LabelNames(ctx context.Context, r *storepb.LabelNamesR
 	}
 
 	var lbls []string
-	version, parseErr := semver.Parse(p.promVersion())
-	if len(matchers) == 0 || (parseErr == nil && version.GTE(baseVer)) {
+	if len(matchers) == 0 || p.labelCallsSupportMatchers() {
 		lbls, err = p.client.LabelNamesInGRPC(ctx, p.base, matchers, r.Start, r.End)
 		if err != nil {
 			return nil, err
@@ -707,23 +711,27 @@ func (p *PrometheusStore) LabelValues(ctx context.Context, r *storepb.LabelValue
 		return &storepb.LabelValuesResponse{}, nil
 	}
 
-	// First check for matching external label which has priority.
-	if l := extLset.Get(r.Label); l != "" {
-		for _, m := range matchers {
-			if !m.Matches(l) {
-				return &storepb.LabelValuesResponse{}, nil
-			}
-		}
-		return &storepb.LabelValuesResponse{Values: []string{l}}, nil
-	}
-
 	var (
 		sers []map[string]string
 		vals []string
 	)
 
-	version, parseErr := semver.Parse(p.promVersion())
-	if len(matchers) == 0 || (parseErr == nil && version.GTE(baseVer)) {
+	// If we request label values for an external label while selecting an additional matcher for other label values
+	if val := extLset.Get(r.Label); val != "" {
+		if len(matchers) == 0 {
+			return &storepb.LabelValuesResponse{Values: []string{val}}, nil
+		}
+		sers, err = p.client.SeriesInGRPC(ctx, p.base, matchers, r.Start, r.End)
+		if err != nil {
+			return nil, err
+		}
+		if len(sers) > 0 {
+			return &storepb.LabelValuesResponse{Values: []string{val}}, nil
+		}
+		return &storepb.LabelValuesResponse{}, nil
+	}
+
+	if len(matchers) == 0 || p.labelCallsSupportMatchers() {
 		vals, err = p.client.LabelValuesInGRPC(ctx, p.base, r.Label, matchers, r.Start, r.End)
 		if err != nil {
 			return nil, err

pkg/store/tsdb.go

@@ -339,38 +339,47 @@ func (s *TSDBStore) LabelValues(ctx context.Context, r *storepb.LabelValuesReque
 	if err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
-
 	if !match {
 		return &storepb.LabelValuesResponse{}, nil
 	}
 
-	if v := s.getExtLset().Get(r.Label); v != "" {
-		for _, m := range matchers {
-			if !m.Matches(v) {
-				return &storepb.LabelValuesResponse{}, nil
-			}
-		}
-		return &storepb.LabelValuesResponse{Values: []string{v}}, nil
-	}
-
 	q, err := s.db.ChunkQuerier(r.Start, r.End)
 	if err != nil {
 		return nil, status.Error(codes.Internal, err.Error())
 	}
 	defer runutil.CloseWithLogOnErr(s.logger, q, "close tsdb querier label values")
 
-	res, _, err := q.LabelValues(ctx, r.Label, matchers...)
-	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
-	}
+	var values []string
 
-	// Label values can come from a postings table of a memory-mapped block which can be deleted during
-	// head compaction. Since we close the block querier before we return from the function,
-	// we need to copy label values to make sure the client still has access to the data when
-	// a block is deleted.
-	values := make([]string, len(res))
-	for i := range res {
-		values[i] = strings.Clone(res[i])
+	// If we request label values for an external label while selecting an additional matcher for other label values
+	if val := s.getExtLset().Get(r.Label); val != "" {
+		if len(matchers) == 0 {
+			return &storepb.LabelValuesResponse{Values: []string{val}}, nil
+		}
+
+		hints := &storage.SelectHints{
+			Start: r.Start,
+			End:   r.End,
+			Func:  "series",
+		}
+		set := q.Select(ctx, false, hints, matchers...)
+
+		var series []labels.Labels
+		for set.Next() {
+			series = append(series, set.At().Labels())
+		}
+		if v := s.getExtLset().Get(r.Label); v != "" && len(series) > 0 {
+			return &storepb.LabelValuesResponse{Values: []string{v}}, nil
+		}
+		return &storepb.LabelValuesResponse{}, nil
+	} else {
+		res, _, err := q.LabelValues(ctx, r.Label, matchers...)
+		if err != nil {
+			return nil, status.Error(codes.Internal, err.Error())
+		}
+		for i := range res {
+			values = append(values, strings.Clone(res[i]))
+		}
 	}
 
 	return &storepb.LabelValuesResponse{Values: values}, nil