Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Oct 29, 2025
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4f2f430
fix(django): Improve logic for classifying cache hits or misses
alexander-alderman-webb Oct 24, 2025
3d1a539
.
alexander-alderman-webb Oct 28, 2025
780a2d3
fix existing tests
alexander-alderman-webb Oct 28, 2025
41fe272
test new behaviour
alexander-alderman-webb Oct 28, 2025
4133b71
typing
alexander-alderman-webb Oct 28, 2025
fbfa6b0
add forked
alexander-alderman-webb Oct 28, 2025
85b5c55
comment
alexander-alderman-webb Oct 28, 2025
7805b48
.
alexander-alderman-webb Oct 28, 2025
dcc112b
.
alexander-alderman-webb Oct 28, 2025
6eb25eb
.
alexander-alderman-webb Oct 28, 2025
391902b
test
alexander-alderman-webb Oct 28, 2025
3a4aeea
do not break out into own function
alexander-alderman-webb Oct 28, 2025
626b529
remove method
alexander-alderman-webb Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 68 additions & 16 deletions sentry_sdk/integrations/django/caching.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,72 @@
from typing import Optional


METHODS_TO_INSTRUMENT = [
"set",
"set_many",
"get",
"get_many",
]


def _get_span_description(method_name, args, kwargs):
# type: (str, tuple[Any], dict[str, Any]) -> str
return _key_as_string(_get_safe_key(method_name, args, kwargs))


def _set_address_and_port(span, address, port):
# type: (sentry_sdk.tracing.Span, Optional[str], Optional[int]) -> None
if address is not None:
span.set_data(SPANDATA.NETWORK_PEER_ADDRESS, address)

if port is not None:
span.set_data(SPANDATA.NETWORK_PEER_PORT, port)


def _patch_get_cache(cache, address, port):
# type: (CacheHandler, Optional[str], Optional[int]) -> None
from sentry_sdk.integrations.django import DjangoIntegration

original_method = cache.get

@ensure_integration_enabled(DjangoIntegration, original_method)
def _instrument_call(cache, original_method, args, kwargs, address, port):
# type: (CacheHandler, Callable[..., Any], tuple[Any, ...], dict[str, Any], Optional[str], Optional[int]) -> Any
op = OP.CACHE_GET
description = _get_span_description("get", args, kwargs)

default_value = None
if len(args) >= 2:
default_value = args[1]
elif "default" in kwargs:
default_value = kwargs["default"]

with sentry_sdk.start_span(
op=op,
name=description,
origin=DjangoIntegration.origin,
) as span:
value = original_method(*args, **kwargs)

with capture_internal_exceptions():
_set_address_and_port(span, address, port)

key = _get_safe_key("get", args, kwargs)
if key is not None:
span.set_data(SPANDATA.CACHE_KEY, key)

item_size = None
if value != default_value:
item_size = len(str(value))
span.set_data(SPANDATA.CACHE_HIT, True)
else:
span.set_data(SPANDATA.CACHE_HIT, False)
Copy link

@cursor cursor bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Cache Miss Detection Fails on Default Value Matches

The cache hit/miss detection logic in _patch_get_cache uses simple equality comparison value != default_value to determine cache hits. This fails for a legitimate edge case: when a cache entry is explicitly set to a value that matches the provided default parameter (e.g., cache.set("key", "null"); cache.get("key", "null")), the code incorrectly marks it as a cache miss because the returned value equals the default value. The implementation cannot distinguish between "key doesn't exist and returned the default" vs "key exists with a value that happens to equal the default". This violates the intended behavior of detecting actual cache hits.

Fix in Cursor Fix in Web

Copy link
Contributor Author

@alexander-alderman-webb alexander-alderman-webb Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. The previous check also suffered from this problem.


if item_size is not None:
span.set_data(SPANDATA.CACHE_ITEM_SIZE, item_size)

return value

@functools.wraps(original_method)
def sentry_method(*args, **kwargs):
# type: (*Any, **Any) -> Any
return _instrument_call(cache, original_method, args, kwargs, address, port)

setattr(cache, "get", sentry_method)


def _patch_cache_method(cache, method_name, address, port):
# type: (CacheHandler, str, Optional[str], Optional[int]) -> None
from sentry_sdk.integrations.django import DjangoIntegration
Expand All @@ -58,19 +111,15 @@ def _instrument_call(
value = original_method(*args, **kwargs)

with capture_internal_exceptions():
if address is not None:
span.set_data(SPANDATA.NETWORK_PEER_ADDRESS, address)

if port is not None:
span.set_data(SPANDATA.NETWORK_PEER_PORT, port)
_set_address_and_port(span, address, port)

key = _get_safe_key(method_name, args, kwargs)
if key is not None:
span.set_data(SPANDATA.CACHE_KEY, key)

item_size = None
if is_get_operation:
if value:
if value != {}:
item_size = len(str(value))
span.set_data(SPANDATA.CACHE_HIT, True)
else:
Expand Down Expand Up @@ -102,8 +151,11 @@ def sentry_method(*args, **kwargs):
def _patch_cache(cache, address=None, port=None):
# type: (CacheHandler, Optional[str], Optional[int]) -> None
if not hasattr(cache, "_sentry_patched"):
for method_name in METHODS_TO_INSTRUMENT:
_patch_cache_method(cache, method_name, address, port)
_patch_cache_method(cache, "set", address, port)
_patch_cache_method(cache, "set_many", address, port)
# Separate patch to account for custom default values on cache misses.
_patch_get_cache(cache, address, port)
Copy link
Contributor

@sentrivana sentrivana Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@alexander-alderman-webb alexander-alderman-webb Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_patch_cache_method(cache, "get_many", address, port)
cache._sentry_patched = True


Expand Down
76 changes: 72 additions & 4 deletions tests/integrations/django/test_cache_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,8 @@ def test_cache_spans_middleware(
assert second_event["spans"][0]["data"]["cache.key"][0].startswith(
"views.decorators.cache.cache_header."
)
assert not second_event["spans"][0]["data"]["cache.hit"]
assert "cache.item_size" not in second_event["spans"][0]["data"]
assert second_event["spans"][0]["data"]["cache.hit"]
assert second_event["spans"][0]["data"]["cache.item_size"] == 2
Comment on lines -226 to +227
Copy link
Contributor Author

@alexander-alderman-webb alexander-alderman-webb Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@sentrivana sentrivana Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@alexander-alderman-webb alexander-alderman-webb Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# second_event - cache.get 2
assert second_event["spans"][1]["op"] == "cache.get"
assert second_event["spans"][1]["description"].startswith(
Expand Down Expand Up @@ -501,14 +501,76 @@ def test_cache_spans_item_size(sentry_init, client, capture_events, use_django_c

assert len(second_event["spans"]) == 2
assert second_event["spans"][0]["op"] == "cache.get"
assert not second_event["spans"][0]["data"]["cache.hit"]
assert "cache.item_size" not in second_event["spans"][0]["data"]
assert second_event["spans"][0]["data"]["cache.hit"]
assert second_event["spans"][0]["data"]["cache.item_size"] == 2
Comment on lines -504 to +505
Copy link
Contributor Author

@alexander-alderman-webb alexander-alderman-webb Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Similar reasoning to the other assertion I changed.


assert second_event["spans"][1]["op"] == "cache.get"
assert second_event["spans"][1]["data"]["cache.hit"]
assert second_event["spans"][1]["data"]["cache.item_size"] == 58


@pytest.mark.forked
@pytest_mark_django_db_decorator()
def test_cache_spans_get_custom_default(
sentry_init, capture_events, use_django_caching
):
sentry_init(
integrations=[
DjangoIntegration(
cache_spans=True,
middleware_spans=False,
signals_spans=False,
)
],
traces_sample_rate=1.0,
)
events = capture_events()

id = os.getpid()

from django.core.cache import cache

with sentry_sdk.start_transaction():
cache.set(f"S{id}", "Sensitive1")
cache.set(f"S{id + 1}", "")

cache.get(f"S{id}", "null")
cache.get(f"S{id}", default="null")

cache.get(f"S{id + 1}", "null")
cache.get(f"S{id + 1}", default="null")

cache.get(f"S{id + 2}", "null")
cache.get(f"S{id + 2}", default="null")

(transaction,) = events
assert len(transaction["spans"]) == 8

assert transaction["spans"][0]["op"] == "cache.put"
assert transaction["spans"][0]["description"] == f"S{id}"

assert transaction["spans"][1]["op"] == "cache.put"
assert transaction["spans"][1]["description"] == f"S{id + 1}"

for span in (transaction["spans"][2], transaction["spans"][3]):
assert span["op"] == "cache.get"
assert span["description"] == f"S{id}"
assert span["data"]["cache.hit"]
assert span["data"]["cache.item_size"] == 10

for span in (transaction["spans"][4], transaction["spans"][5]):
assert span["op"] == "cache.get"
assert span["description"] == f"S{id + 1}"
assert span["data"]["cache.hit"]
assert span["data"]["cache.item_size"] == 0

for span in (transaction["spans"][6], transaction["spans"][7]):
assert span["op"] == "cache.get"
assert span["description"] == f"S{id + 2}"
assert not span["data"]["cache.hit"]
assert "cache.item_size" not in span["data"]


@pytest.mark.forked
@pytest_mark_django_db_decorator()
def test_cache_spans_get_many(sentry_init, capture_events, use_django_caching):
Expand Down Expand Up @@ -538,24 +600,30 @@ def test_cache_spans_get_many(sentry_init, capture_events, use_django_caching):

assert transaction["spans"][0]["op"] == "cache.get"
assert transaction["spans"][0]["description"] == f"S{id}, S{id + 1}"
assert not transaction["spans"][0]["data"]["cache.hit"]

assert transaction["spans"][1]["op"] == "cache.get"
assert transaction["spans"][1]["description"] == f"S{id}"
assert not transaction["spans"][1]["data"]["cache.hit"]

assert transaction["spans"][2]["op"] == "cache.get"
assert transaction["spans"][2]["description"] == f"S{id + 1}"
assert not transaction["spans"][2]["data"]["cache.hit"]

assert transaction["spans"][3]["op"] == "cache.put"
assert transaction["spans"][3]["description"] == f"S{id}"

assert transaction["spans"][4]["op"] == "cache.get"
assert transaction["spans"][4]["description"] == f"S{id}, S{id + 1}"
assert transaction["spans"][4]["data"]["cache.hit"]

assert transaction["spans"][5]["op"] == "cache.get"
assert transaction["spans"][5]["description"] == f"S{id}"
assert transaction["spans"][5]["data"]["cache.hit"]

assert transaction["spans"][6]["op"] == "cache.get"
assert transaction["spans"][6]["description"] == f"S{id + 1}"
assert not transaction["spans"][6]["data"]["cache.hit"]


@pytest.mark.forked
Expand Down