Optimize away label=~".*" matchers in TSDB layer. #6996

pstibrany · 2020-03-17T13:01:19Z

This is alternative to #6995, except it removes these useless matchers in TSDB layer.

Benchmarks in progress, will update the post when finished.

Update: well, first benchmarks show difference where you'd expect -- when using multiple matchers, one of them being ~= ".*". I need to let them run longer, and they block my computer, so I'll do that later.

roidelapluie · 2020-03-17T13:07:14Z

Can we also tackle the case !~".*" ?

pstibrany · 2020-03-17T13:14:15Z

Can we also tackle the case !~".*" ?

By returning empty set, and thus postings? Yes, I think we can.

pstibrany · 2020-03-17T13:34:10Z

I did not add a new test for label!~".*", since there is already a test that triggered this branch here:

prometheus/tsdb/querier_test.go

Lines 2076 to 2078 in fe802f2

    
           	matchers: []*labels.Matcher{labels.MustNewMatcher(labels.MatchEqual, "n", "1"), labels.MustNewMatcher(labels.MatchNotRegexp, "i", "^.*$")}, 
        
           	exp:      []labels.Labels{}, 
        
           },

tsdb/querier.go

brian-brazil · 2020-03-17T15:04:36Z

tsdb/querier.go

+		if m.Type == labels.MatchRegexp && (m.Value == ".*" || m.Value == "^.*$") && len(ms) > 1 {
+			// Ignore this matcher completely. This matches any value, including no value, so it's a no-op.
+			// It's safe to ignore, because there must be some matcher matching non-empty string.
+			// Some tests only use single label=~".*" matcher, and for those, we include the length condition.


It's not just about tests, it's about correctness. Can we fast path those?

You're right. I've modified the code to use all postings if we find this kind of matcher. We only return all postings, if there are no other positive matchers.

bwplotka

Looks good from my side.

NOTE: The same thing would be nice to implement in Thanos - we don't use PostingForMatchers.

We do it here: https://github.com/thanos-io/thanos/blob/2dc375b7a64c5f0d7da488b44664fef1d4865871/pkg/store/bucket.go#L1334

brian-brazil

This still doesn't look right. Can you add to the tests we have for the other regex fast path?

tsdb/querier.go

codesome

LGTM. And as Brian said, some test cases for fast path and cases around that would be good.

brian-brazil · 2020-03-18T09:25:28Z

We've had regressions before for things like this, so we really need a test.

pstibrany · 2020-03-18T09:38:54Z

I've added more test cases covering the shortcuts, and verified that they work with old version of PostingsForMatchers, as well as with new shortcuts.

We've had regressions before for things like this, so we really need a test.

I'm not quite sure how to approach this test. Any suggestions?

brian-brazil · 2020-03-18T12:13:26Z

See #6540

pstibrany · 2020-03-18T12:30:20Z

See #6540

Do you suggest to move these shortcuts into a separate function to make it more testable? I'd prefer to avoid doing that, as it would complicate the code. Given that, I'm out of ideas how to test it in a way that you're suggesting.

brian-brazil · 2020-03-18T12:45:53Z

That's how we're doing existing tests of this sort of thing, so I hoped it'd provide some inspiration. I mainly care that it is tested, so any reasonable approach.

pstibrany · 2020-04-01T11:29:19Z

That's how we're doing existing tests of this sort of thing, so I hoped it'd provide some inspiration. I mainly care that it is tested, so any reasonable approach.

I think I finally have a good way to test these shortcuts -- I simply check if returned posting is empty or "all", or expected label postings match. PTAL. (TestPostingsForMatchersShortcuts)

pstibrany · 2020-04-01T14:28:52Z

Tests are failing in react-app-test step, seems unrelated to my changes. (Perhaps I shouldn't have rebased on top of master)

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pstibrany · 2020-04-20T10:18:34Z

Is there more I should do here, or does the test look good now?

brian-brazil · 2020-04-20T10:56:16Z

Thinking on this there, this optimisation isn't correct at all as . doesn't match newline by default in Go.

pstibrany · 2020-04-20T14:18:33Z

Thinking on this there, this optimisation isn't correct at all as . doesn't match newline by default in Go.

That's a valid point. Since it is not very typical to have newlines in label values, and most people mean to match everything when they write .*, would it make sense to make a breaking change and let . match newline too? (By using "^(?s:" + v + ")$" for anchoring)

But I assume that such discussion has already happened in Prometheus before, and the answer is No.

beorn7 · 2020-04-20T14:43:05Z

It's even remotely plausible that somebody uses foo=~".*" to filter out labels with newlines in it. 😢

brian-brazil · 2020-04-20T15:05:51Z

That would be a breaking change to PromQL, and thus not something that's doable in the current major version of Prometheus. We'd also have to change how regexes across the other repos, and for the SNMP exporter at least newlines in strings have come up.

pstibrany · 2020-04-20T15:44:05Z

You're right. 😞

pstibrany · 2020-04-20T15:44:47Z

Thanks for your review and feedback!

roidelapluie · 2020-04-20T15:46:00Z

Thinking on this there, this optimisation isn't correct at all as . doesn't match newline by default in Go.

Should we have unit tests around this?

roidelapluie · 2020-04-20T16:01:09Z

Note: We have already got requests from users which use new lines in labels, so that is a thing.

brian-brazil reviewed Mar 17, 2020

View reviewed changes

tsdb/querier.go Outdated Show resolved Hide resolved

brian-brazil reviewed Mar 17, 2020

View reviewed changes

bwplotka approved these changes Mar 17, 2020

View reviewed changes

brian-brazil reviewed Mar 18, 2020

View reviewed changes

tsdb/querier.go Outdated Show resolved Hide resolved

codesome approved these changes Mar 18, 2020

View reviewed changes

pstibrany added 7 commits April 8, 2020 14:09

Optimize away label=~".*" matchers in TSDB layer.

138e57d

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Shortcut label!~".*" matcher.

901dd5f

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Shortcut l=~".*" via including all postings.

686b222

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Added test cases covering i=~".*" and i!~".*" shortcuts.

0d7e4f8

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Added more test cases covering =~".*" and !~".*" shortcuts.

090ea63

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Add test for shortcuts in PostingsForMatchers.

dc3f3c3

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Don't compare strings

cb070f9

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pstibrany mentioned this pull request Apr 20, 2020

Remove optimizations for label=~".*" and label!~".*". thanos-io/thanos#2475

Merged

2 tasks

pstibrany closed this Apr 20, 2020

pstibrany mentioned this pull request May 28, 2020

Just ignore matchers which match everything cortexproject/cortex#996

Closed

gouthamve mentioned this pull request Aug 31, 2020

PromQL: Dot in regexp should also match newlines (which is less surprising to most users and also allows optimising the .* regexp) #7859

Open

bboreham mentioned this pull request Aug 5, 2024

[CHANGE] regexp match \n and optimize performance #14505

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize away label=~".*" matchers in TSDB layer. #6996

Optimize away label=~".*" matchers in TSDB layer. #6996

pstibrany commented Mar 17, 2020 •

edited

Loading

roidelapluie commented Mar 17, 2020

pstibrany commented Mar 17, 2020

pstibrany commented Mar 17, 2020 •

edited

Loading

brian-brazil Mar 17, 2020

pstibrany Mar 17, 2020

bwplotka left a comment

brian-brazil left a comment

codesome left a comment

brian-brazil commented Mar 18, 2020

pstibrany commented Mar 18, 2020

brian-brazil commented Mar 18, 2020

pstibrany commented Mar 18, 2020 •

edited

Loading

brian-brazil commented Mar 18, 2020

pstibrany commented Apr 1, 2020 •

edited

Loading

pstibrany commented Apr 1, 2020

pstibrany commented Apr 20, 2020

brian-brazil commented Apr 20, 2020

pstibrany commented Apr 20, 2020

beorn7 commented Apr 20, 2020

brian-brazil commented Apr 20, 2020

pstibrany commented Apr 20, 2020

pstibrany commented Apr 20, 2020

roidelapluie commented Apr 20, 2020

roidelapluie commented Apr 20, 2020

Optimize away label=~".*" matchers in TSDB layer. #6996

Optimize away label=~".*" matchers in TSDB layer. #6996

Conversation

pstibrany commented Mar 17, 2020 • edited Loading

roidelapluie commented Mar 17, 2020

pstibrany commented Mar 17, 2020

pstibrany commented Mar 17, 2020 • edited Loading

brian-brazil Mar 17, 2020

Choose a reason for hiding this comment

pstibrany Mar 17, 2020

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

brian-brazil left a comment

Choose a reason for hiding this comment

codesome left a comment

Choose a reason for hiding this comment

brian-brazil commented Mar 18, 2020

pstibrany commented Mar 18, 2020

brian-brazil commented Mar 18, 2020

pstibrany commented Mar 18, 2020 • edited Loading

brian-brazil commented Mar 18, 2020

pstibrany commented Apr 1, 2020 • edited Loading

pstibrany commented Apr 1, 2020

pstibrany commented Apr 20, 2020

brian-brazil commented Apr 20, 2020

pstibrany commented Apr 20, 2020

beorn7 commented Apr 20, 2020

brian-brazil commented Apr 20, 2020

pstibrany commented Apr 20, 2020

pstibrany commented Apr 20, 2020

roidelapluie commented Apr 20, 2020

roidelapluie commented Apr 20, 2020

pstibrany commented Mar 17, 2020 •

edited

Loading

pstibrany commented Mar 17, 2020 •

edited

Loading

pstibrany commented Mar 18, 2020 •

edited

Loading

pstibrany commented Apr 1, 2020 •

edited

Loading