From b4aee0ef54975d050c8635fe0ffb096dd90e34f7 Mon Sep 17 00:00:00 2001 From: Michael Hoffmann Date: Tue, 23 Jan 2024 11:10:40 +0100 Subject: [PATCH] Store: fix label values edge case (#7082) 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 | 1 + pkg/store/acceptance_test.go | 23 +++++++++++++++++++++++ pkg/store/bucket.go | 5 +++-- pkg/store/prometheus.go | 36 ++++++++++++++++++++++-------------- pkg/store/tsdb.go | 29 +++++++++++++++++++---------- 5 files changed, 68 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 264a6351ea..92c627febf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re ### Fixed - [#7083](https://github.com/thanos-io/thanos/pull/7083) Store Gateway: Fix lazy expanded postings with 0 length failed to be cached. +- [#7082](https://github.com/thanos-io/thanos/pull/7082) Stores: fix label values edge case when requesting external label values with matchers ### Added diff --git a/pkg/store/acceptance_test.go b/pkg/store/acceptance_test.go index 069d732555..113a4a87f5 100644 --- a/pkg/store/acceptance_test.go +++ b/pkg/store/acceptance_test.go @@ -256,6 +256,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 05c4995c7a..feb2538f1f 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -1950,8 +1950,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 197364ca04..eea0334dd0 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,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 diff --git a/pkg/store/tsdb.go b/pkg/store/tsdb.go index 8953946a29..0b860b0675 100644 --- a/pkg/store/tsdb.go +++ b/pkg/store/tsdb.go @@ -339,26 +339,35 @@ 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") + // 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...) + + for set.Next() { + return &storepb.LabelValuesResponse{Values: []string{val}}, nil + } + return &storepb.LabelValuesResponse{}, nil + } + res, _, err := q.LabelValues(ctx, r.Label, matchers...) if err != nil { return nil, status.Error(codes.Internal, err.Error())