Skip to content

Commit

Permalink
Store: fix regex matching with set that matches empty (thanos-io#6692)
Browse files Browse the repository at this point in the history 
Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
  • Loading branch information
@MichaHoffmann @coleenquadros
MichaHoffmann authored and coleenquadros committed Sep 18, 2023
1 parent 0140411 commit f70c32a
Show file tree
Hide file tree
Showing 3 changed files with 246 additions and 8 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
239 changes: 238 additions & 1 deletion pkg/store/acceptance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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"),
},
},
},
},
} {
Expand Down
13 changes: 6 additions & 7 deletions pkg/store/bucket.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit f70c32a

Please sign in to comment.