Optimized label regex matcher with literal prefix and/or suffix #7453
Conversation
I feel that would slow down the development, and we would only optimize those regexes that are of interest to PromQL usage, so it might make more sense here. Anything which looks very general we could put it upstream and remove it from here. |
|
Prefixes and suffixes sound pretty general to me. Also our goal should be to have the best code overall over time, not to do whatever is the most expedient at any given moment. |
This optimisation fits quite well in Prometheus because all our label regex are anchored to the begin/end of text. That being said, I don't think it's necessarily bad have a short experimenting path doing improvements here and then try to upstream whatever we see could make sense for the general use case. However, doing such optimizations in Prometheus would allow us to have a faster feedback loop for the Prometheus use case. Realistically, how many weeks/months we would have to wait before an improvement we upstream in Go will be released and Prometheus will build with that? |
|
I'm just a bit suspicious about prefixes, as I'm not sure we should be getting such a big speedup compared to a regex's FSM. For suffixes I'd not expect a regex to do better for. |
There was a problem hiding this comment.
Amazing. LGTM
However I have to agree with @brian-brazil - regexp library should do that offhand. I would suggest adding an issue on Golang regexp lib to start this discussion and merge this. The potential contribution will be available to use only after few months if any (: Some TODO with link to such issue would be nice 🤗
That's a good point. I was as well, especially because the regex engine has an optimization for prefixes, but the micro benchmarking showed some performance improvement doing the prefix matching ourselves while iterating over a large set of label values for which only a small % matches. |
I think the optimisation only applies for unanchored patterns, you can see if you use LiteralPrefix as a proxy for the |
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
add-filter-to-label-matchers
branch
from
edfb402
to
359143d
Compare
|
I think this PR is good to go. Discussions in Go can follow this. I will merge this in ~2h if no concerns. |
|
👍 |
We're running the Cortex blocks storage (based on TSDB) for a large customer (30M active series) and we see many slow queries caused by regex label matchers on the head (most queries have time range < 1h).
In this specific context, we see very high cardinality label values but only few of them actually match the regex. Profiling these requests we've noticed that most of the time executing the query is spent in
m.Matches()called bypostingsForMatcher().We've also noticed these queries frequently use regex patterns in the format
literal.*or.*literal, so I've experimented a bit if there's any way to optimise for such use case. In this PR I'm proposing an enhancement to check against prefix/suffix literals (in the regex) before running the regex engine.I've also added a couple of cases to the benchmark and below you can see the results. In particular, this is an extract of performances for cases using at least 1 regex matcher:
Full benchmark output