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 all 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
19 changes: 16 additions & 3 deletions sentry_sdk/integrations/django/caching.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ def _instrument_call(
):
# type: (CacheHandler, str, Callable[..., Any], tuple[Any, ...], dict[str, Any], Optional[str], Optional[int]) -> Any
is_set_operation = method_name.startswith("set")
is_get_operation = not is_set_operation
is_get_method = method_name == "get"
is_get_many_method = method_name == "get_many"

op = OP.CACHE_PUT if is_set_operation else OP.CACHE_GET
description = _get_span_description(method_name, args, kwargs)
Expand All @@ -69,8 +70,20 @@ def _instrument_call(
span.set_data(SPANDATA.CACHE_KEY, key)

item_size = None
if is_get_operation:
if value:
if is_get_many_method:
if value != {}:
item_size = len(str(value))
span.set_data(SPANDATA.CACHE_HIT, True)
else:
span.set_data(SPANDATA.CACHE_HIT, False)
elif is_get_method:
default_value = None
if len(args) >= 2:
default_value = args[1]
elif "default" in kwargs:
default_value = kwargs["default"]

if value != default_value:
item_size = len(str(value))
span.set_data(SPANDATA.CACHE_HIT, True)
else:
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

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

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