fix(django): Improve logic for classifying cache hits and misses

[alexander-alderman-webb]

Description

Patch django.core.cache.get() separately from django.core.cache.get_many() to determine cache hits and misses independently for both methods.

When calling get_many() only register a cache miss when the returned value is an empty dictionary.

When calling get(), register a cache miss when

• the return value is None and no default value is provided; or
• the return value is equal to a provided default value.

Issues

Closes #5027

Reminders

• Please add tests to validate your changes, and lint your code using tox -e linters.
• Add GH Issue ID & Linear ID (if applicable)
• PR title should use conventional commit style (feat:, fix:, ref:, meta:)
• For external contributors: CONTRIBUTING.md, Sentry SDK development docs, Discord community

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.93%. Comparing base (104db8c) to head (626b529).
⚠️ Report is 7 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5029      +/-   ##
==========================================
- Coverage   83.94%   83.93%   -0.01%     
==========================================
  Files         178      178              
  Lines       17834    17850      +16     
  Branches     3170     3175       +5     
==========================================
+ Hits        14971    14983      +12     
- Misses       1901     1905       +4     
  Partials      962      962              

Files with missing lines	Coverage Δ
sentry_sdk/integrations/django/caching.py	97.16% <100.00%> (+0.32%)	⬆️

... and 8 files with indirect coverage changes

[alexander-alderman-webb]

Previously marked as a cache miss because [] is falsy.

I do not understand the intent behind asserting a cache miss here, because we assert information about the corresponding cache.put further up in the test.

sentry-python/tests/integrations/django/test_cache_module.py

Lines 206 to 216 in 64c145f

	# first_event - cache.put
	assert first_event["spans"][1]["op"] == "cache.put"
	assert first_event["spans"][1]["description"].startswith(
	"views.decorators.cache.cache_header."
	)
	assert first_event["spans"][1]["data"]["network.peer.address"] is not None
	assert first_event["spans"][1]["data"]["cache.key"][0].startswith(
	"views.decorators.cache.cache_header."
	)
	assert "cache.hit" not in first_event["spans"][1]["data"]
	assert first_event["spans"][1]["data"]["cache.item_size"] == 2

[sentrivana]

This test confuses me too. It calls the not_cached_view view, which, unlike the cached_view, doesn't have the @cache_page decorator. Is there some automatic caching going on anyway even without the decorator? Is some caching always happening because of our use of the use_django_caching_with_middlewares fixture in this test? It definitely seems like it since we're getting cache spans.

The cache miss asserts look wrong to me too. It should be a cache hit. But I don't understand why cache.item_size is now different?

[alexander-alderman-webb]

Yes, the caching only occurs because of the use_django_caching_with_middlewares() fixture here. Without the fixture no spans are generated.

The item's size is 2 because len(str([])) == 2, while previously we did not store the item size at all because we were in the else branch below due to [] being falsy:

sentry-python/sentry_sdk/integrations/django/caching.py

Lines 73 to 77 in 64c145f

	if value:
	item_size = len(str(value))
	span.set_data(SPANDATA.CACHE_HIT, True)
	else:
	span.set_data(SPANDATA.CACHE_HIT, False)

The empty list value originates from

https://github.com/django/django/blob/9ba3f74a46d15f9f2f45ad4ef8cdd245a888e58e/django/utils/cache.py#L437

[alexander-alderman-webb]

Previously marked as a cache miss because [] is falsy.

Similar reasoning to the other assertion I changed.

[sentrivana]

Why make a separate function just for this case? As far as I can tell the logic is still mostly the same as in _patch_cache_method. Would it maybe make more sense to instead modify _patch_cache_method directly to handle get too, just with some extra logic around what the default value should be?

The problem with duplicating logic like this is that the shared parts can easily go out of sync unintentionally (someone edits one of the funcs, misses the other).

[alexander-alderman-webb]

Yes agreed, and the diff is much smaller as well now!

[sentrivana]

This test confuses me too. It calls the not_cached_view view, which, unlike the cached_view, doesn't have the @cache_page decorator. Is there some automatic caching going on anyway even without the decorator? Is some caching always happening because of our use of the use_django_caching_with_middlewares fixture in this test? It definitely seems like it since we're getting cache spans.

The cache miss asserts look wrong to me too. It should be a cache hit. But I don't understand why cache.item_size is now different?

[alexander-alderman-webb]

I tried out your suggestion and I agree that not making a separate function for each method is better here so we don't duplicate logic.

[alexander-alderman-webb]

Yes, the caching only occurs because of the use_django_caching_with_middlewares() fixture here. Without the fixture no spans are generated.

The item's size is 2 because len(str([])) == 2, while previously we did not store the item size at all because we were in the else branch below due to [] being falsy:

sentry-python/sentry_sdk/integrations/django/caching.py

Lines 73 to 77 in 64c145f

	if value:
	item_size = len(str(value))
	span.set_data(SPANDATA.CACHE_HIT, True)
	else:
	span.set_data(SPANDATA.CACHE_HIT, False)

The empty list value originates from

https://github.com/django/django/blob/9ba3f74a46d15f9f2f45ad4ef8cdd245a888e58e/django/utils/cache.py#L437