Skip to content

ref(grouping): Cache enhancements in split form #92000

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

Merged
merged 6 commits into from
May 22, 2025

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented May 20, 2025

Right now, even when we're using split enhancements, the version we cache isn't split, which means that every time we pull them out of the cache, the work to split the rules has to be done all over again. This is obviously not ideal, so this PR changes the way we compute (and parse) the cached string so that it now includes the split rules.

Since the rust enhancer can only parse base64 strings including one set of rules, this is done by computing a separate string for each of the three kinds of rules - rules in their original/pre-split form, classifier rules, and contributes rules - and then concatenating them, separated by a character which can't ever appear in base64. Older base64 strings containing only the original rules and no delimiter can still be parsed, because for them, splitting on the delimiter will be a no-op.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 20, 2025
Copy link

codecov bot commented May 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

⚠️ Parser warning

The parser emitted a warning. Please review your JUnit XML file:

Warning while parsing testcase attributes: Limit of string is 1000 chars, for name, we got 2083 at 1:157235 in /home/runner/work/sentry/sentry/.artifacts/pytest.junit.xml
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #92000       +/-   ##
===========================================
+ Coverage   36.78%   87.63%   +50.84%     
===========================================
  Files        9662    10359      +697     
  Lines      540758   587563    +46805     
  Branches    22604    22604               
===========================================
+ Hits       198930   514885   +315955     
+ Misses     341407    72257   -269150     
  Partials      421      421               

@lobsterkatie lobsterkatie marked this pull request as ready for review May 21, 2025 01:30
@lobsterkatie lobsterkatie requested a review from a team as a code owner May 21, 2025 01:30
Copy link
Member

@yuvmen yuvmen left a comment

Choose a reason for hiding this comment

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

looks good 👍

@lobsterkatie lobsterkatie merged commit 6a4e926 into master May 22, 2025
62 checks passed
@lobsterkatie lobsterkatie deleted the kmclb-cache-enhancements-in-split-form branch May 22, 2025 18:53
andrewshie-sentry pushed a commit that referenced this pull request Jun 2, 2025
Right now, even when we're using split enhancements, the version we cache isn't split, which means that every time we pull them out of the cache, the work to split the rules has to be done all over again. This is obviously not ideal, so this PR changes the way we compute (and parse) the cached string so that it now includes the split rules.

Since the rust enhancer can only parse base64 strings including one set of rules, this is done by computing a separate string for each of the three kinds of rules - rules in their original/pre-split form, classifier rules, and contributes rules - and then concatenating them, separated by a character which can't ever appear in base64. Older base64 strings containing only the original rules and no delimiter can still be parsed, because for them, splitting on the delimiter will be a no-op.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants