Cache panel incorrectly reports hits and misses for cache.get() when a default value is supplied or None is cached

[Harsh3006]

The Cache panel currently determines whether a cache.get() operation is a hit or a miss solely by checking whether the returned value is None.

This leads to incorrect hit/miss reporting in the following cases:

1. Missing key with a default value

cache.get("missing", "default")

The cache backend does not contain the key, so this is a cache miss. However, the Cache panel reports it as a cache hit because the returned value is the supplied default ("default"), which is not None.

2. Cached value is None

cache.set("key", None)
cache.get("key")

The key exists in the cache, so this is a cache hit. However, the Cache panel reports it as a cache miss because the cached value is None.

Expected behavior

The Cache panel should distinguish between:

• a missing cache key, and
• a cache key whose stored value is None (or any value equal to the caller's supplied default).

Background

Django's cache framework recommends using a unique sentinel object when calling cache.get() to distinguish between a missing key and a stored value of None, see the highlighted guidance here. The current implementation does not account for this behavior, which results in incorrect cache hit/miss statistics for the scenarios described above.

[tim-schilling]

@Harsh3006 moving our conversation here. So there are some things we could do to improve the situation:

1. Determine what the default value is. Currently we assume it's None, but it may not be. And when it's not, we incorrectly treat those cases as hits.
2. Add a note to the cache panel's template and documentation that indicates the number of misses may be off.
3. Related to 2, In the panel template and documentation direct people to a GitHub discussion for them to voice their support for better tracking
4. Add an opt-in setting to control more accurate tracking that enables Improve cache hit/miss reporting for cache.get() #2418, with documentation explaining the concern
5. Add cache backend specific instrumentation that determines if the value was missing or the default was stored
   • For example, DatabaseCache.get() uses get_many() to return a dict where it does .get(key, default). We should be able to wrap get_many() such that we could see if result is an empty dict or has a value.
6. Open a new feature request for Django to support some observability tooling to track this at the framework level

• 1, 2, 3 are probably the easiest and best places for us to start.
• 4 I'm a bit unsure of as an optional feature which is why I'd like 3 before it so we know how many people actually care. otherwise we're maintaining something that's not being used.
• 5 could be a lot of work as we may need something custom for each backend. And if we do, what about backends that aren't in core?
• 6 is a good idea, but will take at least a year for us to be able to take advantage of.

[Harsh3006]

Thanks @tim-schilling, this makes sense. I agree that 1, 2, and 3 are a good place to start.

For (1), I can update the instrumentation to determine the caller's supplied default rather than always assuming None. My understanding is that we would then use the return value/default comparison as a best-effort heuristic for determining a miss, while documenting that a cached value equal to the supplied default is still inherently ambiguous.

The implementation would keep _record_call() unchanged from its original behavior and handle this in _store_call_info(). For name == "get", we can determine the supplied default with:

default = kwargs.get("default", args[1] if len(args) > 1 else None)

Then, instead of checking whether return_value is None, we would use:

if return_value == default:
    self.misses += 1
else:
    self.hits += 1

This would improve the current behavior for calls with a non-None default, while still having the unavoidable ambiguity when the cached value compares equal to the supplied default.

For the remaining ambiguity, we can document the behavior in both the cache panel template and the panel documentation, such as:

Note: Cache hit/miss statistics for cache.get() calls may not always be accurate. See issue #2417 for details and discussion about improving cache hit/miss tracking.

Once you confirm the above approach, I'll proceed with the default-value handling and the corresponding panel/documentation note, and leave the opt-in/backend-specific approaches for later based on feedback.

[tim-schilling]

That all generally makes sense. I think visually, we'll need to figure out where to present the note on the panel. That can be hashed out on the PR though.

[Harsh3006]

Thanks @tim-schilling. One quick clarification before I finalize the note: when you mentioned directing users to a GitHub Discussion to voice support for better cache hit/miss tracking, did you mean the existing issue #2417, or did you mean to open a new thread in the Discussions page?

[tim-schilling]

I meant a discussion not the issue, but it's not a strong preference.

[Harsh3006]

We've started a GitHub Discussion to explore alternative approaches and gauge interest in more accurate cache hit/miss tracking.

Please share your thoughts or use cases there: #2441.