rhubarbgroup / redis-cache

A persistent object cache backend for WordPress powered by Redis. Supports Predis, PhpRedis, Relay, replication, sentinels, clustering and WP-CLI.
https://wordpress.org/plugins/redis-cache/
GNU General Public License v3.0
425 stars 148 forks source link

Potentially extra `cache_misses` counts for `get_multiple()` #515

Closed sjeng closed 5 months ago

sjeng commented 5 months ago

Description

This might not be a bug, and instead be an misunderstanding of how cache_misses works. Also super low priority since impact is minimal.

When using the get_multiple() method in unforced mode, cache_misses gets incremented if the internal cache does not hold the data yet. If the later $redis->mget() call also fails to return data, cache_misses gets incremented again fro the same key.

I guess the question is also whether internal cache misses should increment the overall cache_misses counter as well? Seems to make more sense personally that cache_misses counter should only track misses with Redis calls, but I may be ignorant to some obvious cases where it makes sense the other way.

Code: https://github.com/rhubarbgroup/redis-cache/blob/b582fbad902fd11d4d035ed62b1cec144b52081c/includes/object-cache.php#L2025-L2027

Expected Behavior

When call the get_multiple() method, cache_misses should only increment if Redis cache misses, not when internal cache misses.

Actual Behavior

cache_misses get incremented on internal cache misses when calling get_multiple(), regardless of whether Redis has the key or not.

Additional context

Makes ratio lower, and lower numbers make monkey brain sad.

tillkruss commented 5 months ago

Good question. In Object Cache Pro we split up the internal cache misses and Redis misses, while in Redis Object Cache its only "WordPress" cache misses that include both counts.

If you want to submit a PR that tracks both, I'm happy to merge it.

sjeng commented 5 months ago

Thanks for the response!

There is really no value for us to track both misses, so I probably won't bother with a PR at this point. Will close this for now.