From 275ba5fe20507dbf47cf263f92e8bac3d64d4c7d Mon Sep 17 00:00:00 2001 From: Michael Hoffmann Date: Sat, 2 Sep 2023 11:07:59 +0200 Subject: [PATCH] Store: fix regex matching with set that matches empty Signed-off-by: Michael Hoffmann --- CHANGELOG.md | 2 + pkg/store/acceptance_test.go | 239 ++++++++++++++++++++++++++++++++++- pkg/store/bucket.go | 13 +- 3 files changed, 246 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 137ebe2f42..9cb5b88033 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 +- [#6692](https://github.com/thanos-io/thanos/pull/6692) Store: Fix matching bug when using empty alternative in regex matcher, for example (a||b). + ### Added - [#6605](https://github.com/thanos-io/thanos/pull/6605) Query Frontend: Support vertical sharding binary expression with metric name when no matching labels specified. diff --git a/pkg/store/acceptance_test.go b/pkg/store/acceptance_test.go index d90d893da2..ecc23f5aa3 100644 --- a/pkg/store/acceptance_test.go +++ b/pkg/store/acceptance_test.go @@ -214,7 +214,7 @@ func testStoreAPIsAcceptance(t *testing.T, startStore func(t *testing.T, extLset }, }, { - // Tests mostly taken from https://github.com/prometheus/prometheus/blob/95e705612c1d557f1681bd081a841b78f93ee158/tsdb/querier_test.go#L1898, though some are still missing + // Testcases taken from https://github.com/prometheus/prometheus/blob/95e705612c1d557f1681bd081a841b78f93ee158/tsdb/querier_test.go#L1898 desc: "matching behavior", appendFn: func(app storage.Appender) { _, err := app.Append(0, labels.FromStrings("n", "1"), 0, 0) @@ -403,6 +403,243 @@ func testStoreAPIsAcceptance(t *testing.T, startStore func(t *testing.T, extLset labels.FromStrings("n", "2.5", "region", "eu-west"), }, }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_RE, Name: "i", Value: "^$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_RE, Name: "i", Value: "^.*$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "a", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "b", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_RE, Name: "i", Value: "^.+$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "i", "a", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "b", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_NRE, Name: "n", Value: "^1$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "2", "region", "eu-west"), + labels.FromStrings("n", "2.5", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_NRE, Name: "n", Value: "1"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "2", "region", "eu-west"), + labels.FromStrings("n", "2.5", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_NRE, Name: "n", Value: "1|2.5"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "2", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_NRE, Name: "n", Value: "(1|2.5)"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "2", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_NRE, Name: "i", Value: "^a$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "b", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_NRE, Name: "i", Value: "^a?$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "i", "b", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_NRE, Name: "i", Value: "^$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "i", "a", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "b", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_NRE, Name: "i", Value: "^.*$"}, + }, + expectedLabels: []labels.Labels{}, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_NRE, Name: "i", Value: "^.+$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_NEQ, Name: "i", Value: ""}, + {Type: storepb.LabelMatcher_EQ, Name: "i", Value: "a"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "i", "a", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "n", Value: "1"}, + {Type: storepb.LabelMatcher_NEQ, Name: "i", Value: "b"}, + {Type: storepb.LabelMatcher_RE, Name: "i", Value: "^(b|a).*$"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "i", "a", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_RE, Name: "n", Value: "(1|2)"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "a", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "b", "region", "eu-west"), + labels.FromStrings("n", "2", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_RE, Name: "i", Value: "a|b"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "i", "a", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "b", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_RE, Name: "i", Value: "(a|b)"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "i", "a", "region", "eu-west"), + labels.FromStrings("n", "1", "i", "b", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_RE, Name: "n", Value: "x1|2"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "2", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_RE, Name: "n", Value: "2|2\\.5"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "2", "region", "eu-west"), + labels.FromStrings("n", "2.5", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_RE, Name: "i", Value: "c||d"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "region", "eu-west"), + labels.FromStrings("n", "2", "region", "eu-west"), + labels.FromStrings("n", "2.5", "region", "eu-west"), + }, + }, + { + start: timestamp.FromTime(minTime), + end: timestamp.FromTime(maxTime), + matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_RE, Name: "i", Value: "(c||d)"}, + }, + expectedLabels: []labels.Labels{ + labels.FromStrings("n", "1", "region", "eu-west"), + labels.FromStrings("n", "2", "region", "eu-west"), + labels.FromStrings("n", "2.5", "region", "eu-west"), + }, + }, }, }, } { diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index 0d780fc2c7..8a2664eeb4 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -2574,13 +2574,6 @@ func matchersToPostingGroups(ctx context.Context, lvalsFn func(name string) ([]s // NOTE: Derived from tsdb.postingsForMatcher. index.Merge is equivalent to map duplication. func toPostingGroup(ctx context.Context, lvalsFn func(name string) ([]string, error), m *labels.Matcher) (*postingGroup, []string, error) { - if m.Type == labels.MatchRegexp { - if vals := findSetMatches(m.Value); len(vals) > 0 { - sort.Strings(vals) - return newPostingGroup(false, m.Name, vals, nil), nil, nil - } - } - // If the matcher selects an empty value, it selects all the series which don't // have the label name set too. See: https://github.com/prometheus/prometheus/issues/3575 // and https://github.com/prometheus/prometheus/pull/3578#issuecomment-351653555. @@ -2619,6 +2612,12 @@ func toPostingGroup(ctx context.Context, lvalsFn func(name string) ([]string, er return newPostingGroup(true, m.Name, nil, toRemove), vals, nil } + if m.Type == labels.MatchRegexp { + if vals := findSetMatches(m.Value); len(vals) > 0 { + sort.Strings(vals) + return newPostingGroup(false, m.Name, vals, nil), nil, nil + } + } // Fast-path for equal matching. if m.Type == labels.MatchEqual {