Skip to content
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.

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

feat(codeowners): Find codeowner path matches with rust #1746

Merged
merged 4 commits into from Jan 27, 2023

Conversation

NisanthanNanthakumar
Copy link
Contributor

@NisanthanNanthakumar NisanthanNanthakumar commented Jan 14, 2023

Proof of Concept PR

Benchmarked this solution agains the baseline python implementation getsentry/sentry#43294

This code is benchmarking ~3 times faster than the baseline python implementation. So it is ready to be merged into Sentry.

@jjbayer
Copy link
Member

jjbayer commented Jan 24, 2023

@NisanthanNanthakumar I did not look at your benchmark in detail, but in my experience running a benchmark with a global LRU cache can easily skew results, because you get much better cache locality in the experiment than in production. Have you considered using pre-compiled regexes on the Python side instead?

@NisanthanNanthakumar
Copy link
Contributor Author

@jjbayer yea I realized that too. Thats why the plan is to push to production under a feature flag and run it against a sampled subset of the sentry org events and compare the performance between the two implementations in prod.

WRT the pre-compiled regexes in Python suggestion, the re package already handles that caching optimization here: https://github.com/python/cpython/blob/5a8ed019f9b826d2ba5766688d51005624cd0699/Lib/re/__init__.py#L302

Copy link
Member

@snigdhas snigdhas left a comment

Choose a reason for hiding this comment

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

Thanks for walking me through it!

fix changelog

new line

dash
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

I did not check the pattern translation logic in detail, but the rest looks good to me!

We will have to release a new version of the Python library if you want to run this in production.

@NisanthanNanthakumar NisanthanNanthakumar merged commit 923b234 into master Jan 27, 2023
@NisanthanNanthakumar NisanthanNanthakumar deleted the rust-codeowners branch January 27, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants