chore(grouping): Clean up grouphash caching#108274
Merged
lobsterkatie merged 6 commits intomasterfrom Feb 18, 2026
Merged
Conversation
github-actions
bot
added
the
Scope: Backend
yuvmen
approved these changes
Feb 13, 2026
Member
yuvmen
left a comment
yuvmen
left a comment
There was a problem hiding this comment.
looks good, gave yourself and the review an easy time with all those TODO notes, nice job 👍
JonasBa
pushed a commit
that referenced
this pull request
Feb 19, 2026
This undoes the changes made in #104460 and #104537, which enabled us to experiment with expiry times for the caches we use for grouphashes during ingest. I'd hoped that leaving stuff in the cache longer would increase the hit rate, but it didn't really make any difference - all 3 expiry times (1, 2, and 5 minutes) had essentially identical hit rates. (See PR for graph showing this.) At the same time, leaving things in the cache longer did (unsurprisingly) mean that we used more cache space by having more entries in the cache at any one time. (PR also includes a graph showing this.) Clearly, using all of that space in the cache isn't worth it, so this PR removes the code letting expiry time be variable and switches to just using the smallest expiry time of 1 minute. It also removes the versioning of our cache values, because it only existed to allow us to switch from one set of experimental values to another. Finally, it removes the `expiry_seconds` tag from our metrics, since the value will no longer vary.
1 task
mchen-sentry
pushed a commit
that referenced
this pull request
Feb 24, 2026
This undoes the changes made in #104460 and #104537, which enabled us to experiment with expiry times for the caches we use for grouphashes during ingest. I'd hoped that leaving stuff in the cache longer would increase the hit rate, but it didn't really make any difference - all 3 expiry times (1, 2, and 5 minutes) had essentially identical hit rates. (See PR for graph showing this.) At the same time, leaving things in the cache longer did (unsurprisingly) mean that we used more cache space by having more entries in the cache at any one time. (PR also includes a graph showing this.) Clearly, using all of that space in the cache isn't worth it, so this PR removes the code letting expiry time be variable and switches to just using the smallest expiry time of 1 minute. It also removes the versioning of our cache values, because it only existed to allow us to switch from one set of experimental values to another. Finally, it removes the `expiry_seconds` tag from our metrics, since the value will no longer vary.
cvxluo
added a commit
that referenced
this pull request
Mar 2, 2026
The cache expiry values were experimentally verified in #108274 to not need tuning (1, 2, and 5 minute TTLs all had identical hit rates). Hardcode the 60s TTL directly and remove the options. Co-authored-by: Claude <noreply@anthropic.com>
cvxluo
added a commit
that referenced
this pull request
Mar 2, 2026
The cache expiry values were experimentally verified in #108274 to not need tuning (1, 2, and 5 minute TTLs all had identical hit rates). Hardcode the 60s TTL directly and remove the options. Co-authored-by: Claude <noreply@anthropic.com>
cvxluo
added a commit
that referenced
this pull request
Mar 2, 2026
The cache expiry values were experimentally verified in #108274 to not need tuning (1, 2, and 5 minute TTLs all had identical hit rates). Hardcode the 60s TTL directly and remove the options. Co-authored-by: Claude <noreply@anthropic.com>
cvxluo
added a commit
that referenced
this pull request
Mar 2, 2026
The cache expiry values were experimentally verified in #108274 to not need tuning (1, 2, and 5 minute TTLs all had identical hit rates). Hardcode the 60s TTL directly and remove the options. Co-authored-by: Claude <noreply@anthropic.com>
cvxluo
added a commit
that referenced
this pull request
Mar 2, 2026
The cache expiry values were experimentally verified in #108274 to not need tuning (1, 2, and 5 minute TTLs all had identical hit rates). Hardcode the 60s TTL directly and remove the options. Co-authored-by: Claude <noreply@anthropic.com>
cvxluo
added a commit
that referenced
this pull request
Mar 2, 2026
The cache expiry values were experimentally verified in #108274 to not need tuning (1, 2, and 5 minute TTLs all had identical hit rates). Hardcode the 60s TTL directly and remove the options. Co-authored-by: Claude <noreply@anthropic.com>
cvxluo
added a commit
that referenced
this pull request
Mar 2, 2026
The cache expiry values were experimentally verified in #108274 to not need tuning (1, 2, and 5 minute TTLs all had identical hit rates). Hardcode the 60s TTL directly and remove the options. Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This undoes the changes made in #104460 and #104537, which enabled us to experiment with expiry times for the caches we use for grouphashes during ingest. I'd hoped that leaving stuff in the cache longer would increase the hit rate, but as we can see from the graph below, it didn't really make any difference - all 3 expiry times (1, 2, and 5 minutes) had essentially identical hit rates.
At the same time, leaving things in the cache longer did (unsurprisingly) mean that we used more cache space by having more entries in the cache at any one time:
Clearly, using all of that space in the cache isn't worth it, so this PR removes the code letting expiry time be variable and switches to just using the smallest expiry time of 1 minute. It also removes the versioning of our cache values, because it only existed to allow us to switch from one set of experimental values to another. Finally, it removes the
expiry_secondstag from our metrics, since the value will no longer vary.