Skip to content

Regex set optimisation when looking series in ingester #2475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 17, 2020
Merged

Regex set optimisation when looking series in ingester #2475

merged 3 commits into from
Apr 17, 2020

Conversation

gouthamve
Copy link
Contributor

@gouthamve gouthamve commented Apr 16, 2020 •
edited
Loading

Similar to #2446

Super useful for templated grafana dashboards which send matchers such
as =~"a|b|c|d|e"

Signed-off-by: Goutham Veeramachaneni [email protected]

What this PR does:

Which issue(s) this PR fixes:
Fixes #1993

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@gouthamve
Regex set optimisation when looking series in ingester
5e618a8 
Similar to cortexproject#2446

Super useful for templated grafana dashboards which send matchers such
as =~"a|b|c|d|"

Signed-off-by: Goutham Veeramachaneni <[email protected]>
@gouthamve gouthamve requested a review from pstibrany April 16, 2020 15:31
cyriltovena
cyriltovena approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

You should get that merged asap!

@cyriltovena
Copy link
Contributor

Also closes #1993

pstibrany
pstibrany approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice!

@pstibrany
Copy link
Contributor

Changelog entry is missing

@pstibrany
Copy link
Contributor

Changelog entry is missing

Perhaps can just extend entry for #2446, up to you.

@pstibrany pstibrany mentioned this pull request Apr 16, 2020
Merged
2 tasks
@gouthamve
Add CHANGELOG entry
59385d5 
Signed-off-by: Goutham Veeramachaneni <[email protected]>
@gouthamve
Add benchmark for set optimisation
b2023da 
Signed-off-by: Goutham Veeramachaneni <[email protected]>
@gouthamve
Copy link
Contributor Author 

➜  cortex git:(regex-opt-ingester) ✗ benchcmp old.txt new.txt
benchcmp is deprecated in favor of benchstat: https://pkg.go.dev/golang.org/x/perf/cmd/benchstat
benchmark                                             old ns/op     new ns/op     delta
BenchmarkSetRegexLookup/select_all-8                  435911065     882075        -99.80%
BenchmarkSetRegexLookup/select_two-8                  247328056     23848         -99.99%
BenchmarkSetRegexLookup/select_half-8                 327012500     530910        -99.84%
BenchmarkSetRegexLookup/select_none-8                 231561666     24398         -99.99%
BenchmarkSetRegexLookup/equality_matcher-8            2474          2488          +0.57%
BenchmarkSetRegexLookup/regex_(non-set)_matcher-8     274129027     276701117     +0.94%

Wow! I can't wait to put this in production.

@gouthamve gouthamve merged commit 50afddf into cortexproject:master Apr 17, 2020
@gouthamve gouthamve deleted the regex-opt-ingester branch April 17, 2020 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
3 more reviewers

@pstibrany pstibrany pstibrany approved these changes

@cyriltovena cyriltovena cyriltovena approved these changes

@codesome codesome codesome approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimize for regexp matchers in ingesters
4 participants
@gouthamve @cyriltovena @pstibrany @codesome