From 02dfb6197b5c437539ce559e7ba8d6bd71fbdde9 Mon Sep 17 00:00:00 2001 From: Oron Sharabi Date: Mon, 15 Aug 2022 14:46:53 +0300 Subject: [PATCH 1/4] Filter external labels from matchers on LabelValues/LabelNames Signed-off-by: Oron Sharabi --- pkg/store/bucket.go | 57 ++++++++++++++++++++++++++++++---------- pkg/store/bucket_test.go | 46 ++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 14 deletions(-) diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index 8cd3d26419..d880935c9d 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -1254,6 +1254,11 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq if len(reqBlockMatchers) > 0 && !b.matchRelabelLabels(reqBlockMatchers) { continue } + // filter ext labels + reqSeriesMatchersNoExtLabels, ok := b.FilterExtLabelsMatchers(reqSeriesMatchers) + if !ok { + continue + } resHints.AddQueriedBlock(b.meta.ULID) @@ -1270,7 +1275,7 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq defer runutil.CloseWithLogOnErr(s.logger, indexr, "label names") var result []string - if len(reqSeriesMatchers) == 0 { + if len(reqSeriesMatchersNoExtLabels) == 0 { // Do it via index reader to have pending reader registered correctly. // LabelNames are already sorted. res, err := indexr.block.indexHeaderReader.LabelNames() @@ -1288,7 +1293,7 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq result = strutil.MergeSlices(res, extRes) } else { - seriesSet, _, err := blockSeries(newCtx, b.extLset, indexr, nil, reqSeriesMatchers, nil, seriesLimiter, true, req.Start, req.End, nil, nil) + seriesSet, _, err := blockSeries(newCtx, b.extLset, indexr, nil, reqSeriesMatchersNoExtLabels, nil, seriesLimiter, true, req.Start, req.End, nil, nil) if err != nil { return errors.Wrapf(err, "fetch series for block %s", b.meta.ULID) } @@ -1341,6 +1346,25 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq }, nil } +func (b *bucketBlock) FilterExtLabelsMatchers(matchers []*labels.Matcher) ([]*labels.Matcher, bool) { + // we filter external labels from matchers + // so it won't try to match series on them. + var result []*labels.Matcher + for _, m := range matchers { + // get value of external label from block + v := b.extLset.Get(m.Name) + // if value is empty string the matcher is a valid one since it's not part of external labels + if v == "" { + result = append(result, m) + } else if v != "" && v != m.Value { + // if matcher is external label but value is different we don't want to look in block anyway + return []*labels.Matcher{}, false + } + } + + return result, true +} + // LabelValues implements the storepb.StoreServer interface. func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesRequest) (*storepb.LabelValuesResponse, error) { reqSeriesMatchers, err := storepb.MatchersToPromMatchers(req.Matchers...) @@ -1366,16 +1390,6 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR } } - // If we have series matchers, add != "" matcher, to only select series that have given label name. - if len(reqSeriesMatchers) > 0 { - m, err := labels.NewMatcher(labels.MatchNotEqual, req.Label, "") - if err != nil { - return nil, status.Error(codes.InvalidArgument, err.Error()) - } - - reqSeriesMatchers = append(reqSeriesMatchers, m) - } - s.mtx.RLock() var mtx sync.Mutex @@ -1391,6 +1405,21 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR if len(reqBlockMatchers) > 0 && !b.matchRelabelLabels(reqBlockMatchers) { continue } + // filter ext labels + reqSeriesMatchersNoExtLabels, ok := b.FilterExtLabelsMatchers(reqSeriesMatchers) + if !ok { + continue + } + + // If we have series matchers, add != "" matcher, to only select series that have given label name. + if len(reqSeriesMatchersNoExtLabels) > 0 { + m, err := labels.NewMatcher(labels.MatchNotEqual, req.Label, "") + if err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + + reqSeriesMatchersNoExtLabels = append(reqSeriesMatchersNoExtLabels, m) + } resHints.AddQueriedBlock(b.meta.ULID) @@ -1406,7 +1435,7 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR defer runutil.CloseWithLogOnErr(s.logger, indexr, "label values") var result []string - if len(reqSeriesMatchers) == 0 { + if len(reqSeriesMatchersNoExtLabels) == 0 { // Do it via index reader to have pending reader registered correctly. res, err := indexr.block.indexHeaderReader.LabelValues(req.Label) if err != nil { @@ -1419,7 +1448,7 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR } result = res } else { - seriesSet, _, err := blockSeries(newCtx, b.extLset, indexr, nil, reqSeriesMatchers, nil, seriesLimiter, true, req.Start, req.End, nil, nil) + seriesSet, _, err := blockSeries(newCtx, b.extLset, indexr, nil, reqSeriesMatchersNoExtLabels, nil, seriesLimiter, true, req.Start, req.End, nil, nil) if err != nil { return errors.Wrapf(err, "fetch series for block %s", b.meta.ULID) } diff --git a/pkg/store/bucket_test.go b/pkg/store/bucket_test.go index 407571cbcd..df2c60041d 100644 --- a/pkg/store/bucket_test.go +++ b/pkg/store/bucket_test.go @@ -188,6 +188,52 @@ func TestBucketBlock_Property(t *testing.T) { properties.TestingRun(t) } +func TestBucketFilterExtLabelsMatchers(t *testing.T) { + defer testutil.TolerantVerifyLeak(t) + + dir := t.TempDir() + bkt, err := filesystem.NewBucket(dir) + testutil.Ok(t, err) + defer func() { testutil.Ok(t, bkt.Close()) }() + + blockID := ulid.MustNew(1, nil) + meta := &metadata.Meta{ + BlockMeta: tsdb.BlockMeta{ULID: blockID}, + Thanos: metadata.Thanos{ + Labels: map[string]string{ + "a": "b", + "c": "d", + }, + }, + } + b, err := newBucketBlock(context.Background(), log.NewNopLogger(), newBucketStoreMetrics(nil), meta, bkt, path.Join(dir, blockID.String()), nil, nil, nil, nil) + ms := []*labels.Matcher { + {Type: labels.MatchNotEqual, Name: "a", Value: "b"}, + } + res, _ := b.FilterExtLabelsMatchers(ms) + testutil.Equals(t, len(res), 0) + + ms = []*labels.Matcher { + {Type: labels.MatchNotEqual, Name: "a", Value: "a"}, + } + _, ok := b.FilterExtLabelsMatchers(ms) + testutil.Equals(t, ok, false) + + ms = []*labels.Matcher { + {Type: labels.MatchNotEqual, Name: "a", Value: "a"}, + {Type: labels.MatchNotEqual, Name: "c", Value: "d"}, + } + res, _ = b.FilterExtLabelsMatchers(ms) + testutil.Equals(t, len(res), 0) + + ms = []*labels.Matcher { + {Type: labels.MatchNotEqual, Name: "a2", Value: "a"}, + } + res, _ = b.FilterExtLabelsMatchers(ms) + testutil.Equals(t, len(res), 1) + testutil.Equals(t, res, ms) +} + func TestBucketBlock_matchLabels(t *testing.T) { defer testutil.TolerantVerifyLeak(t) From 893ba45dd0d02bbe9f167f830f71abf1ff52a8b6 Mon Sep 17 00:00:00 2001 From: Oron Sharabi Date: Mon, 15 Aug 2022 18:44:10 +0300 Subject: [PATCH 2/4] Fix linting Signed-off-by: Oron Sharabi --- pkg/store/bucket.go | 4 ++-- pkg/store/bucket_test.go | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index d880935c9d..5fe03936a0 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -1349,7 +1349,7 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq func (b *bucketBlock) FilterExtLabelsMatchers(matchers []*labels.Matcher) ([]*labels.Matcher, bool) { // we filter external labels from matchers // so it won't try to match series on them. - var result []*labels.Matcher + var result []*labels.Matcher for _, m := range matchers { // get value of external label from block v := b.extLset.Get(m.Name) @@ -1405,7 +1405,7 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR if len(reqBlockMatchers) > 0 && !b.matchRelabelLabels(reqBlockMatchers) { continue } - // filter ext labels + // filter ext labels reqSeriesMatchersNoExtLabels, ok := b.FilterExtLabelsMatchers(reqSeriesMatchers) if !ok { continue diff --git a/pkg/store/bucket_test.go b/pkg/store/bucket_test.go index df2c60041d..bff5f090e5 100644 --- a/pkg/store/bucket_test.go +++ b/pkg/store/bucket_test.go @@ -207,31 +207,31 @@ func TestBucketFilterExtLabelsMatchers(t *testing.T) { }, } b, err := newBucketBlock(context.Background(), log.NewNopLogger(), newBucketStoreMetrics(nil), meta, bkt, path.Join(dir, blockID.String()), nil, nil, nil, nil) - ms := []*labels.Matcher { + ms := []*labels.Matcher{ {Type: labels.MatchNotEqual, Name: "a", Value: "b"}, - } + } res, _ := b.FilterExtLabelsMatchers(ms) testutil.Equals(t, len(res), 0) - ms = []*labels.Matcher { + ms = []*labels.Matcher{ {Type: labels.MatchNotEqual, Name: "a", Value: "a"}, - } + } _, ok := b.FilterExtLabelsMatchers(ms) testutil.Equals(t, ok, false) - ms = []*labels.Matcher { + ms = []*labels.Matcher{ {Type: labels.MatchNotEqual, Name: "a", Value: "a"}, - {Type: labels.MatchNotEqual, Name: "c", Value: "d"}, - } + {Type: labels.MatchNotEqual, Name: "c", Value: "d"}, + } res, _ = b.FilterExtLabelsMatchers(ms) testutil.Equals(t, len(res), 0) - ms = []*labels.Matcher { + ms = []*labels.Matcher{ {Type: labels.MatchNotEqual, Name: "a2", Value: "a"}, - } + } res, _ = b.FilterExtLabelsMatchers(ms) testutil.Equals(t, len(res), 1) - testutil.Equals(t, res, ms) + testutil.Equals(t, res, ms) } func TestBucketBlock_matchLabels(t *testing.T) { From f497e09185fa81a72871dd8b061884b1cd768a2d Mon Sep 17 00:00:00 2001 From: Oron Sharabi Date: Mon, 15 Aug 2022 19:03:13 +0300 Subject: [PATCH 3/4] Fix linter Signed-off-by: Oron Sharabi --- pkg/store/bucket_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/store/bucket_test.go b/pkg/store/bucket_test.go index bff5f090e5..70d010b9f4 100644 --- a/pkg/store/bucket_test.go +++ b/pkg/store/bucket_test.go @@ -206,7 +206,7 @@ func TestBucketFilterExtLabelsMatchers(t *testing.T) { }, }, } - b, err := newBucketBlock(context.Background(), log.NewNopLogger(), newBucketStoreMetrics(nil), meta, bkt, path.Join(dir, blockID.String()), nil, nil, nil, nil) + b, _ := newBucketBlock(context.Background(), log.NewNopLogger(), newBucketStoreMetrics(nil), meta, bkt, path.Join(dir, blockID.String()), nil, nil, nil, nil) ms := []*labels.Matcher{ {Type: labels.MatchNotEqual, Name: "a", Value: "b"}, } From d561e4c2225f594438f186de8f08a959e038fc0f Mon Sep 17 00:00:00 2001 From: Oron Sharabi Date: Wed, 17 Aug 2022 18:28:39 +0300 Subject: [PATCH 4/4] Fix comments to be full Sentences Signed-off-by: Oron Sharabi --- pkg/store/bucket.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index 5fe03936a0..4862896857 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -1254,7 +1254,7 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq if len(reqBlockMatchers) > 0 && !b.matchRelabelLabels(reqBlockMatchers) { continue } - // filter ext labels + // Filter external labels from matchers. reqSeriesMatchersNoExtLabels, ok := b.FilterExtLabelsMatchers(reqSeriesMatchers) if !ok { continue @@ -1347,17 +1347,16 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq } func (b *bucketBlock) FilterExtLabelsMatchers(matchers []*labels.Matcher) ([]*labels.Matcher, bool) { - // we filter external labels from matchers - // so it won't try to match series on them. + // We filter external labels from matchers so we won't try to match series on them. var result []*labels.Matcher for _, m := range matchers { - // get value of external label from block + // Get value of external label from block. v := b.extLset.Get(m.Name) - // if value is empty string the matcher is a valid one since it's not part of external labels + // If value is empty string the matcher is a valid one since it's not part of external labels. if v == "" { result = append(result, m) } else if v != "" && v != m.Value { - // if matcher is external label but value is different we don't want to look in block anyway + // If matcher is external label but value is different we don't want to look in block anyway. return []*labels.Matcher{}, false } } @@ -1405,7 +1404,7 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR if len(reqBlockMatchers) > 0 && !b.matchRelabelLabels(reqBlockMatchers) { continue } - // filter ext labels + // Filter external labels from matchers. reqSeriesMatchersNoExtLabels, ok := b.FilterExtLabelsMatchers(reqSeriesMatchers) if !ok { continue