diff --git a/sentry_sdk/integrations/django/caching.py b/sentry_sdk/integrations/django/caching.py index 8f5b1b9229..3c0e905c44 100644 --- a/sentry_sdk/integrations/django/caching.py +++ b/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 diff --git a/sentry_sdk/integrations/redis/modules/caches.py b/sentry_sdk/integrations/redis/modules/caches.py index 31824aafa3..754b2118b8 100644 --- a/sentry_sdk/integrations/redis/modules/caches.py +++ b/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 diff --git a/sentry_sdk/integrations/redis/utils.py b/sentry_sdk/integrations/redis/utils.py index 207468ac77..64b12395b6 100644 --- a/sentry_sdk/integrations/redis/utils.py +++ b/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): diff --git a/tests/integrations/django/test_cache_module.py b/tests/integrations/django/test_cache_module.py index c47b512b02..646c73ae04 100644 --- a/tests/integrations/django/test_cache_module.py +++ b/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"], diff --git a/tests/integrations/redis/test_redis_cache_module.py b/tests/integrations/redis/test_redis_cache_module.py index d96d074343..ef25983abe 100644 --- a/tests/integrations/redis/test_redis_cache_module.py +++ b/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 diff --git a/tests/integrations/redis/test_redis_cache_module_async.py b/tests/integrations/redis/test_redis_cache_module_async.py index 32e4beabea..d607f92fbd 100644 --- a/tests/integrations/redis/test_redis_cache_module_async.py +++ b/tests/integrations/redis/test_redis_cache_module_async.py @@ -128,7 +128,9 @@ async def test_cache_data(sentry_init, capture_events): assert spans[0]["op"] == "cache.get" assert spans[0]["description"] == "myasynccachekey" - assert spans[0]["data"]["cache.key"] == "myasynccachekey" + assert spans[0]["data"]["cache.key"] == [ + "myasynccachekey", + ] 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. @@ -146,7 +148,9 @@ async def test_cache_data(sentry_init, capture_events): assert spans[2]["op"] == "cache.put" assert spans[2]["description"] == "myasynccachekey" - assert spans[2]["data"]["cache.key"] == "myasynccachekey" + assert spans[2]["data"]["cache.key"] == [ + "myasynccachekey", + ] assert "cache.hit" not in spans[1]["data"] assert spans[2]["data"]["cache.item_size"] == 18 # very old fakeredis can not handle port. @@ -164,7 +168,9 @@ async def test_cache_data(sentry_init, capture_events): assert spans[4]["op"] == "cache.get" assert spans[4]["description"] == "myasynccachekey" - assert spans[4]["data"]["cache.key"] == "myasynccachekey" + assert spans[4]["data"]["cache.key"] == [ + "myasynccachekey", + ] assert spans[4]["data"]["cache.hit"] == True # noqa: E712 assert spans[4]["data"]["cache.item_size"] == 18 # very old fakeredis can not handle port.