From de519f23f1a7d69a74106e265f5970c0a8461dd8 Mon Sep 17 00:00:00 2001 From: Michael Hoffmann Date: Sat, 20 Jan 2024 18:08:23 +0100 Subject: [PATCH] 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 --- CHANGELOG.md | 2 ++ pkg/store/acceptance_test.go | 23 ++++++++++++++++ pkg/store/bucket.go | 5 ++-- pkg/store/prometheus.go | 37 +++++++++++++++---------- pkg/store/tsdb.go | 52 +++++++++++++++++++++--------------- 5 files changed, 82 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76137ab4de2..a8699011aa6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pkg/store/acceptance_test.go b/pkg/store/acceptance_test.go index 34abc649ab1..dae3e503c11 100644 --- a/pkg/store/acceptance_test.go +++ b/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", diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index fd4fb7392c4..7bd1647f668 100644 --- a/pkg/store/bucket.go +++ b/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 != "" 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 != "" 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()) diff --git a/pkg/store/prometheus.go b/pkg/store/prometheus.go index 197364ca040..99c7fefb4ff 100644 --- a/pkg/store/prometheus.go +++ b/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,28 @@ 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 match on an external label we need to check if we have any series matching + // the matchers. If we have no matchers we shall return the external label. + 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 diff --git a/pkg/store/tsdb.go b/pkg/store/tsdb.go index 8953946a292..8644bab399f 100644 --- a/pkg/store/tsdb.go +++ b/pkg/store/tsdb.go @@ -339,38 +339,48 @@ 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 match on an external label we need to check if we have any series matching + // the matchers. If we have no matchers we shall return the external label. + 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