Made cache.key span data field a list (#3110)

* Made cache.key span data field a list

---------

Co-authored-by: Ivana Kellyerova <[email protected]>

Author: antonpirker

sentry_sdk/integrations/django/caching.py

@@ -1,6 +1,6 @@
 import functools
 from typing import TYPE_CHECKING
-from sentry_sdk.integrations.redis.utils import _get_safe_key
+from sentry_sdk.integrations.redis.utils import _get_safe_key, _key_as_string
 from urllib3.util import parse_url as urlparse
 
 from django import VERSION as DJANGO_VERSION
@@ -30,7 +30,7 @@
 
 def _get_span_description(method_name, args, kwargs):
     # type: (str, tuple[Any], dict[str, Any]) -> str
-    return _get_safe_key(method_name, args, kwargs)
+    return _key_as_string(_get_safe_key(method_name, args, kwargs))
 
 
 def _patch_cache_method(cache, method_name, address, port):
@@ -61,7 +61,7 @@ def _instrument_call(
                     span.set_data(SPANDATA.NETWORK_PEER_PORT, port)
 
                 key = _get_safe_key(method_name, args, kwargs)
-                if key != "":
+                if key is not None:
                     span.set_data(SPANDATA.CACHE_KEY, key)
 
                 item_size = None

sentry_sdk/integrations/redis/modules/caches.py

@@ -4,7 +4,7 @@
 
 from sentry_sdk._types import TYPE_CHECKING
 from sentry_sdk.consts import OP, SPANDATA
-from sentry_sdk.integrations.redis.utils import _get_safe_key
+from sentry_sdk.integrations.redis.utils import _get_safe_key, _key_as_string
 from sentry_sdk.utils import capture_internal_exceptions
 
 GET_COMMANDS = ("get", "mget")
@@ -30,10 +30,11 @@ def _get_op(name):
 def _compile_cache_span_properties(redis_command, args, kwargs, integration):
     # type: (str, tuple[Any, ...], dict[str, Any], RedisIntegration) -> dict[str, Any]
     key = _get_safe_key(redis_command, args, kwargs)
+    key_as_string = _key_as_string(key)
 
     is_cache_key = False
     for prefix in integration.cache_prefixes:
-        if key.startswith(prefix):
+        if key_as_string.startswith(prefix):
             is_cache_key = True
             break
 
@@ -47,6 +48,7 @@ def _compile_cache_span_properties(redis_command, args, kwargs, integration):
             redis_command, args, kwargs, integration
         ),
         "key": key,
+        "key_as_string": key_as_string,
         "redis_command": redis_command.lower(),
         "is_cache_key": is_cache_key,
         "value": value,
@@ -57,7 +59,7 @@ def _compile_cache_span_properties(redis_command, args, kwargs, integration):
 
 def _get_cache_span_description(redis_command, args, kwargs, integration):
     # type: (str, tuple[Any, ...], dict[str, Any], RedisIntegration) -> str
-    description = _get_safe_key(redis_command, args, kwargs)
+    description = _key_as_string(_get_safe_key(redis_command, args, kwargs))
 
     data_should_be_truncated = (
         integration.max_data_size and len(description) > integration.max_data_size

sentry_sdk/integrations/redis/utils.py

@@ -44,38 +44,60 @@ def _get_safe_command(name, args):
     return command
 
 
+def _safe_decode(key):
+    # type: (Any) -> str
+    if isinstance(key, bytes):
+        try:
+            return key.decode()
+        except UnicodeDecodeError:
+            return ""
+
+    return key
+
+
+def _key_as_string(key):
+    # type: (Any) -> str
+    if isinstance(key, (dict, list, tuple)):
+        key = ", ".join(_safe_decode(x) for x in key)
+    elif isinstance(key, bytes):
+        key = _safe_decode(key)
+    elif key is None:
+        key = ""
+    else:
+        key = str(key)
+
+    return key
+
+
 def _get_safe_key(method_name, args, kwargs):
-    # type: (str, Optional[tuple[Any, ...]], Optional[dict[str, Any]]) -> str
+    # type: (str, Optional[tuple[Any, ...]], Optional[dict[str, Any]]) -> Optional[tuple[str, ...]]
     """
-    Gets the keys (or keys) from the given method_name.
+    Gets the key (or keys) from the given method_name.
     The method_name could be a redis command or a django caching command
     """
-    key = ""
+    key = None
+
     if args is not None and method_name.lower() in _MULTI_KEY_COMMANDS:
         # for example redis "mget"
-        key = ", ".join(x.decode() if isinstance(x, bytes) else x for x in args)
+        key = tuple(args)
 
     elif args is not None and len(args) >= 1:
         # for example django "set_many/get_many" or redis "get"
-        key = args[0].decode() if isinstance(args[0], bytes) else args[0]
+        if isinstance(args[0], (dict, list, tuple)):
+            key = tuple(args[0])
+        else:
+            key = (args[0],)
 
     elif kwargs is not None and "key" in kwargs:
-        # this is a legacy case for older versions of django (I guess)
-        key = (
-            kwargs["key"].decode()
-            if isinstance(kwargs["key"], bytes)
-            else kwargs["key"]
-        )
-
-    if isinstance(key, dict):
-        # Django caching set_many() has a dictionary {"key": "data", "key2": "data2"}
-        # as argument. In this case only return the keys of the dictionary (to not leak data)
-        key = ", ".join(x.decode() if isinstance(x, bytes) else x for x in key.keys())
-
-    if isinstance(key, list):
-        key = ", ".join(x.decode() if isinstance(x, bytes) else x for x in key)
-
-    return str(key)
+        # this is a legacy case for older versions of Django
+        if isinstance(kwargs["key"], (list, tuple)):
+            if len(kwargs["key"]) > 0:
+                key = tuple(kwargs["key"])
+        else:
+            if kwargs["key"] is not None:
+                key = (kwargs["key"],)
+
+    return key
 
 
 def _parse_rediscluster_command(command):

tests/integrations/django/test_cache_module.py

@@ -1,9 +1,9 @@
-import pytest
 import os
 import random
+import uuid
 
+import pytest
 from django import VERSION as DJANGO_VERSION
-
 from werkzeug.test import Client
 
 try:
@@ -198,7 +198,7 @@ def test_cache_spans_middleware(
         "views.decorators.cache.cache_header."
     )
     assert first_event["spans"][0]["data"]["network.peer.address"] is not None
-    assert first_event["spans"][0]["data"]["cache.key"].startswith(
+    assert first_event["spans"][0]["data"]["cache.key"][0].startswith(
         "views.decorators.cache.cache_header."
     )
     assert not first_event["spans"][0]["data"]["cache.hit"]
@@ -209,7 +209,7 @@ def test_cache_spans_middleware(
         "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"].startswith(
+    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"]
@@ -220,7 +220,7 @@ def test_cache_spans_middleware(
         "views.decorators.cache.cache_header."
     )
     assert second_event["spans"][0]["data"]["network.peer.address"] is not None
-    assert second_event["spans"][0]["data"]["cache.key"].startswith(
+    assert second_event["spans"][0]["data"]["cache.key"][0].startswith(
         "views.decorators.cache.cache_header."
     )
     assert not second_event["spans"][0]["data"]["cache.hit"]
@@ -231,7 +231,7 @@ def test_cache_spans_middleware(
         "views.decorators.cache.cache_page."
     )
     assert second_event["spans"][1]["data"]["network.peer.address"] is not None
-    assert second_event["spans"][1]["data"]["cache.key"].startswith(
+    assert second_event["spans"][1]["data"]["cache.key"][0].startswith(
         "views.decorators.cache.cache_page."
     )
     assert second_event["spans"][1]["data"]["cache.hit"]
@@ -264,7 +264,7 @@ def test_cache_spans_decorator(sentry_init, client, capture_events, use_django_c
         "views.decorators.cache.cache_header."
     )
     assert first_event["spans"][0]["data"]["network.peer.address"] is not None
-    assert first_event["spans"][0]["data"]["cache.key"].startswith(
+    assert first_event["spans"][0]["data"]["cache.key"][0].startswith(
         "views.decorators.cache.cache_header."
     )
     assert not first_event["spans"][0]["data"]["cache.hit"]
@@ -275,7 +275,7 @@ def test_cache_spans_decorator(sentry_init, client, capture_events, use_django_c
         "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"].startswith(
+    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"]
@@ -286,7 +286,7 @@ def test_cache_spans_decorator(sentry_init, client, capture_events, use_django_c
         "views.decorators.cache.cache_page."
     )
     assert second_event["spans"][1]["data"]["network.peer.address"] is not None
-    assert second_event["spans"][1]["data"]["cache.key"].startswith(
+    assert second_event["spans"][1]["data"]["cache.key"][0].startswith(
         "views.decorators.cache.cache_page."
     )
     assert second_event["spans"][1]["data"]["cache.hit"]
@@ -322,7 +322,7 @@ def test_cache_spans_templatetag(
         "template.cache.some_identifier."
     )
     assert first_event["spans"][0]["data"]["network.peer.address"] is not None
-    assert first_event["spans"][0]["data"]["cache.key"].startswith(
+    assert first_event["spans"][0]["data"]["cache.key"][0].startswith(
         "template.cache.some_identifier."
     )
     assert not first_event["spans"][0]["data"]["cache.hit"]
@@ -333,7 +333,7 @@ def test_cache_spans_templatetag(
         "template.cache.some_identifier."
     )
     assert first_event["spans"][1]["data"]["network.peer.address"] is not None
-    assert first_event["spans"][1]["data"]["cache.key"].startswith(
+    assert first_event["spans"][1]["data"]["cache.key"][0].startswith(
         "template.cache.some_identifier."
     )
     assert "cache.hit" not in first_event["spans"][1]["data"]
@@ -344,7 +344,7 @@ def test_cache_spans_templatetag(
         "template.cache.some_identifier."
     )
     assert second_event["spans"][0]["data"]["network.peer.address"] is not None
-    assert second_event["spans"][0]["data"]["cache.key"].startswith(
+    assert second_event["spans"][0]["data"]["cache.key"][0].startswith(
         "template.cache.some_identifier."
     )
     assert second_event["spans"][0]["data"]["cache.hit"]
@@ -358,6 +358,7 @@ def test_cache_spans_templatetag(
         ("get", None, None, ""),
         ("get", [], {}, ""),
         ("get", ["bla", "blub", "foo"], {}, "bla"),
+        ("get", [uuid.uuid4().bytes], {}, ""),
         (
             "get_many",
             [["bla1", "bla2", "bla3"], "blub", "foo"],

tests/integrations/redis/test_redis_cache_module.py

@@ -1,10 +1,12 @@
+import uuid
+
 import pytest
 
 import fakeredis
 from fakeredis import FakeStrictRedis
 
 from sentry_sdk.integrations.redis import RedisIntegration
-from sentry_sdk.integrations.redis.utils import _get_safe_key
+from sentry_sdk.integrations.redis.utils import _get_safe_key, _key_as_string
 from sentry_sdk.utils import parse_version
 import sentry_sdk
 
@@ -137,7 +139,9 @@ def test_cache_data(sentry_init, capture_events):
 
     assert spans[0]["op"] == "cache.get"
     assert spans[0]["description"] == "mycachekey"
-    assert spans[0]["data"]["cache.key"] == "mycachekey"
+    assert spans[0]["data"]["cache.key"] == [
+        "mycachekey",
+    ]
     assert spans[0]["data"]["cache.hit"] == False  # noqa: E712
     assert "cache.item_size" not in spans[0]["data"]
     # very old fakeredis can not handle port and/or host.
@@ -155,7 +159,9 @@ def test_cache_data(sentry_init, capture_events):
 
     assert spans[2]["op"] == "cache.put"
     assert spans[2]["description"] == "mycachekey"
-    assert spans[2]["data"]["cache.key"] == "mycachekey"
+    assert spans[2]["data"]["cache.key"] == [
+        "mycachekey",
+    ]
     assert "cache.hit" not in spans[1]["data"]
     assert spans[2]["data"]["cache.item_size"] == 18
     # very old fakeredis can not handle port.
@@ -173,7 +179,9 @@ def test_cache_data(sentry_init, capture_events):
 
     assert spans[4]["op"] == "cache.get"
     assert spans[4]["description"] == "mycachekey"
-    assert spans[4]["data"]["cache.key"] == "mycachekey"
+    assert spans[4]["data"]["cache.key"] == [
+        "mycachekey",
+    ]
     assert spans[4]["data"]["cache.hit"] == True  # noqa: E712
     assert spans[4]["data"]["cache.item_size"] == 18
     # very old fakeredis can not handle port.
@@ -193,17 +201,72 @@ def test_cache_data(sentry_init, capture_events):
 @pytest.mark.parametrize(
     "method_name,args,kwargs,expected_key",
     [
-        (None, None, None, ""),
-        ("", None, None, ""),
-        ("set", ["bla", "valuebla"], None, "bla"),
-        ("setex", ["bla", 10, "valuebla"], None, "bla"),
-        ("get", ["bla"], None, "bla"),
-        ("mget", ["bla", "blub", "foo"], None, "bla, blub, foo"),
-        ("set", [b"bla", "valuebla"], None, "bla"),
-        ("setex", [b"bla", 10, "valuebla"], None, "bla"),
-        ("get", [b"bla"], None, "bla"),
-        ("mget", [b"bla", "blub", "foo"], None, "bla, blub, foo"),
+        (None, None, None, None),
+        ("", None, None, None),
+        ("set", ["bla", "valuebla"], None, ("bla",)),
+        ("setex", ["bla", 10, "valuebla"], None, ("bla",)),
+        ("get", ["bla"], None, ("bla",)),
+        ("mget", ["bla", "blub", "foo"], None, ("bla", "blub", "foo")),
+        ("set", [b"bla", "valuebla"], None, (b"bla",)),
+        ("setex", [b"bla", 10, "valuebla"], None, (b"bla",)),
+        ("get", [b"bla"], None, (b"bla",)),
+        ("mget", [b"bla", "blub", "foo"], None, (b"bla", "blub", "foo")),
+        ("not-important", None, {"something": "bla"}, None),
+        ("not-important", None, {"key": None}, None),
+        ("not-important", None, {"key": "bla"}, ("bla",)),
+        ("not-important", None, {"key": b"bla"}, (b"bla",)),
+        ("not-important", None, {"key": []}, None),
+        (
+            "not-important",
+            None,
+            {
+                "key": [
+                    "bla",
+                ]
+            },
+            ("bla",),
+        ),
+        (
+            "not-important",
+            None,
+            {"key": [b"bla", "blub", "foo"]},
+            (b"bla", "blub", "foo"),
+        ),
+        (
+            "not-important",
+            None,
+            {"key": b"\x00c\x0f\xeaC\xe1L\x1c\xbff\xcb\xcc\xc1\xed\xc6\t"},
+            (b"\x00c\x0f\xeaC\xe1L\x1c\xbff\xcb\xcc\xc1\xed\xc6\t",),
+        ),
+        (
+            "get",
+            [b"\x00c\x0f\xeaC\xe1L\x1c\xbff\xcb\xcc\xc1\xed\xc6\t"],
+            None,
+            (b"\x00c\x0f\xeaC\xe1L\x1c\xbff\xcb\xcc\xc1\xed\xc6\t",),
+        ),
     ],
 )
 def test_get_safe_key(method_name, args, kwargs, expected_key):
     assert _get_safe_key(method_name, args, kwargs) == expected_key
+
+
+@pytest.mark.parametrize(
+    "key,expected_key",
+    [
+        (None, ""),
+        (("bla",), "bla"),
+        (("bla", "blub", "foo"), "bla, blub, foo"),
+        ((b"bla",), "bla"),
+        ((b"bla", "blub", "foo"), "bla, blub, foo"),
+        (
+            [
+                "bla",
+            ],
+            "bla",
+        ),
+        (["bla", "blub", "foo"], "bla, blub, foo"),
+        ([uuid.uuid4().bytes], ""),
+    ],
+)
+def test_key_as_string(key, expected_key):
+    assert _key_as_string(key) == expected_key