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

Determine cache hits and misses independently for `django.core.cache.get()` and `django.core.cache.get_many()`.

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.

Closes #5027

Author: alexander-alderman-webb

sentry_sdk/integrations/django/caching.py

@@ -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)
@@ -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:

tests/integrations/django/test_cache_module.py

@@ -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
     # second_event - cache.get 2
     assert second_event["spans"][1]["op"] == "cache.get"
     assert second_event["spans"][1]["description"].startswith(
@@ -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
 
     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):
@@ -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