sentry icon indicating copy to clipboard operation
sentry copied to clipboard

fix(grouping): Fix grouphash caching metric cache expiry tagging

Open lobsterkatie opened this issue 1 month ago • 1 comments

Currently, we only tag our grouping.get_or_create_grouphashes.check_secondary_hash_existence and grouping.get_or_create_grouphashes.get_or_create_grouphash metrics with the cache expiry time in cases where there's a cache hit. When there was only one possible expiry time, that worked fine, because anything that was a cache miss also can be assumed to have used that same expiry period. However, now that we're testing multiple expiry times at once, it's impossible to know which expiry time led to a given miss, which in turn throws off our hit-rate calculations.

To fix that problem, this PR adds the expiry_seconds tag to misses as well. Since by definition misses don't find anything in the cache, this also switches from using a cached expiry value to using the current expiry value. Finally, so that using the current value doesn't pollute results based on hits stored with an old value, it also introduces the use of cache versioning. That way, any time the list of potential cache times changes, no previously-stored values will be found.

lobsterkatie avatar Dec 08 '25 19:12 lobsterkatie

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #104537      +/-   ##
===========================================
- Coverage   80.63%    80.52%   -0.12%     
===========================================
  Files        9335      9330       -5     
  Lines      402949    400659    -2290     
  Branches    25689     25689              
===========================================
- Hits       324921    322621    -2300     
- Misses      77562     77572      +10     
  Partials      466       466              

codecov[bot] avatar Dec 08 '25 19:12 codecov[bot]

You’re not entirely wrong about the complication. I could have just had an option for version and manually incremented it any time the trial values were changed, too, which would have been simpler. This was a (maybe weird) way of me being a little lazy, LOL. Since this is all temporary anyway, I ended up prioritizing ease of testing/comparison over maintainability, which I probably wouldn't have done otherwise.

lobsterkatie avatar Dec 09 '25 17:12 lobsterkatie