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

sentry_sdk/integrations/django/caching.py

@@ -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)
+
+                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
@@ -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 is not None:
                         item_size = len(str(value))
                         span.set_data(SPANDATA.CACHE_HIT, True)
                     else:
@@ -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)
+        _patch_cache_method(cache, "get_many", address, port)
         cache._sentry_patched = True
 
 

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