Fix breadcrumbs in redis (#3680)

This moves the creation of `redis` breadcrumbs from the `maybe_create_breadcrumbs_from_span` into the integrations. And as well fixes some span related tests by using `render_span_tree`.

Author: antonpirker

sentry_sdk/integrations/redis/_async_common.py

@@ -3,12 +3,14 @@
 from sentry_sdk.integrations.redis.consts import SPAN_ORIGIN
 from sentry_sdk.integrations.redis.modules.caches import (
     _compile_cache_span_properties,
-    _set_cache_data,
+    _get_cache_data,
 )
 from sentry_sdk.integrations.redis.modules.queries import _compile_db_span_properties
 from sentry_sdk.integrations.redis.utils import (
-    _set_client_data,
-    _set_pipeline_data,
+    _create_breadcrumb,
+    _get_client_data,
+    _get_pipeline_data,
+    _update_span,
 )
 from sentry_sdk.tracing import Span
 from sentry_sdk.utils import capture_internal_exceptions
@@ -23,9 +25,9 @@
 
 
 def patch_redis_async_pipeline(
-    pipeline_cls, is_cluster, get_command_args_fn, set_db_data_fn
+    pipeline_cls, is_cluster, get_command_args_fn, get_db_data_fn
 ):
-    # type: (Union[type[Pipeline[Any]], type[ClusterPipeline[Any]]], bool, Any, Callable[[Span, Any], None]) -> None
+    # type: (Union[type[Pipeline[Any]], type[ClusterPipeline[Any]]], bool, Any, Callable[[Any], dict[str, Any]]) -> None
     old_execute = pipeline_cls.execute
 
     from sentry_sdk.integrations.redis import RedisIntegration
@@ -41,22 +43,25 @@ async def _sentry_execute(self, *args, **kwargs):
             origin=SPAN_ORIGIN,
         ) as span:
             with capture_internal_exceptions():
-                set_db_data_fn(span, self)
-                _set_pipeline_data(
-                    span,
-                    is_cluster,
-                    get_command_args_fn,
-                    False if is_cluster else self.is_transaction,
-                    self._command_stack if is_cluster else self.command_stack,
+                span_data = get_db_data_fn(self)
+                pipeline_data = _get_pipeline_data(
+                    is_cluster=is_cluster,
+                    get_command_args_fn=get_command_args_fn,
+                    is_transaction=False if is_cluster else self.is_transaction,
+                    command_stack=(
+                        self._command_stack if is_cluster else self.command_stack
+                    ),
                 )
+                _update_span(span, span_data, pipeline_data)
+                _create_breadcrumb("redis.pipeline.execute", span_data, pipeline_data)
 
             return await old_execute(self, *args, **kwargs)
 
     pipeline_cls.execute = _sentry_execute  # type: ignore
 
 
-def patch_redis_async_client(cls, is_cluster, set_db_data_fn):
-    # type: (Union[type[StrictRedis[Any]], type[RedisCluster[Any]]], bool, Callable[[Span, Any], None]) -> None
+def patch_redis_async_client(cls, is_cluster, get_db_data_fn):
+    # type: (Union[type[StrictRedis[Any]], type[RedisCluster[Any]]], bool, Callable[[Any], dict[str, Any]]) -> None
     old_execute_command = cls.execute_command
 
     from sentry_sdk.integrations.redis import RedisIntegration
@@ -92,15 +97,20 @@ async def _sentry_execute_command(self, name, *args, **kwargs):
         )
         db_span.__enter__()
 
-        set_db_data_fn(db_span, self)
-        _set_client_data(db_span, is_cluster, name, *args)
+        db_span_data = get_db_data_fn(self)
+        db_client_span_data = _get_client_data(is_cluster, name, *args)
+        _update_span(db_span, db_span_data, db_client_span_data)
+        _create_breadcrumb(
+            db_properties["description"], db_span_data, db_client_span_data
+        )
 
         value = await old_execute_command(self, name, *args, **kwargs)
 
         db_span.__exit__(None, None, None)
 
         if cache_span:
-            _set_cache_data(cache_span, self, cache_properties, value)
+            cache_span_data = _get_cache_data(self, cache_properties, value)
+            _update_span(cache_span, cache_span_data)
             cache_span.__exit__(None, None, None)
 
         return value

sentry_sdk/integrations/redis/_sync_common.py

@@ -3,12 +3,14 @@
 from sentry_sdk.integrations.redis.consts import SPAN_ORIGIN
 from sentry_sdk.integrations.redis.modules.caches import (
     _compile_cache_span_properties,
-    _set_cache_data,
+    _get_cache_data,
 )
 from sentry_sdk.integrations.redis.modules.queries import _compile_db_span_properties
 from sentry_sdk.integrations.redis.utils import (
-    _set_client_data,
-    _set_pipeline_data,
+    _create_breadcrumb,
+    _get_client_data,
+    _get_pipeline_data,
+    _update_span,
 )
 from sentry_sdk.tracing import Span
 from sentry_sdk.utils import capture_internal_exceptions
@@ -24,9 +26,9 @@ def patch_redis_pipeline(
     pipeline_cls,
     is_cluster,
     get_command_args_fn,
-    set_db_data_fn,
+    get_db_data_fn,
 ):
-    # type: (Any, bool, Any, Callable[[Span, Any], None]) -> None
+    # type: (Any, bool, Any, Callable[[Any], dict[str, Any]]) -> None
     old_execute = pipeline_cls.execute
 
     from sentry_sdk.integrations.redis import RedisIntegration
@@ -42,22 +44,23 @@ def sentry_patched_execute(self, *args, **kwargs):
             origin=SPAN_ORIGIN,
         ) as span:
             with capture_internal_exceptions():
-                set_db_data_fn(span, self)
-                _set_pipeline_data(
-                    span,
-                    is_cluster,
-                    get_command_args_fn,
-                    False if is_cluster else self.transaction,
-                    self.command_stack,
+                span_data = get_db_data_fn(self)
+                pipeline_data = _get_pipeline_data(
+                    is_cluster=is_cluster,
+                    get_command_args_fn=get_command_args_fn,
+                    is_transaction=False if is_cluster else self.transaction,
+                    command_stack=self.command_stack,
                 )
+                _update_span(span, span_data, pipeline_data)
+                _create_breadcrumb("redis.pipeline.execute", span_data, pipeline_data)
 
             return old_execute(self, *args, **kwargs)
 
     pipeline_cls.execute = sentry_patched_execute
 
 
-def patch_redis_client(cls, is_cluster, set_db_data_fn):
-    # type: (Any, bool, Callable[[Span, Any], None]) -> None
+def patch_redis_client(cls, is_cluster, get_db_data_fn):
+    # type: (Any, bool, Callable[[Any], dict[str, Any]]) -> None
     """
     This function can be used to instrument custom redis client classes or
     subclasses.
@@ -97,15 +100,20 @@ def sentry_patched_execute_command(self, name, *args, **kwargs):
         )
         db_span.__enter__()
 
-        set_db_data_fn(db_span, self)
-        _set_client_data(db_span, is_cluster, name, *args)
+        db_span_data = get_db_data_fn(self)
+        db_client_span_data = _get_client_data(is_cluster, name, *args)
+        _update_span(db_span, db_span_data, db_client_span_data)
+        _create_breadcrumb(
+            db_properties["description"], db_span_data, db_client_span_data
+        )
 
         value = old_execute_command(self, name, *args, **kwargs)
 
         db_span.__exit__(None, None, None)
 
         if cache_span:
-            _set_cache_data(cache_span, self, cache_properties, value)
+            cache_span_data = _get_cache_data(self, cache_properties, value)
+            _update_span(cache_span, cache_span_data)
             cache_span.__exit__(None, None, None)
 
         return value

sentry_sdk/integrations/redis/modules/caches.py

@@ -75,22 +75,24 @@ def _get_cache_span_description(redis_command, args, kwargs, integration):
     return description
 
 
-def _set_cache_data(span, redis_client, properties, return_value):
-    # type: (Span, Any, dict[str, Any], Optional[Any]) -> None
+def _get_cache_data(redis_client, properties, return_value):
+    # type: (Any, dict[str, Any], Optional[Any]) -> dict[str, Any]
+    data = {}
+
     with capture_internal_exceptions():
-        span.set_data(SPANDATA.CACHE_KEY, properties["key"])
+        data[SPANDATA.CACHE_KEY] = properties["key"]
 
         if properties["redis_command"] in GET_COMMANDS:
             if return_value is not None:
-                span.set_data(SPANDATA.CACHE_HIT, True)
+                data[SPANDATA.CACHE_HIT] = True
                 size = (
                     len(str(return_value).encode("utf-8"))
                     if not isinstance(return_value, bytes)
                     else len(return_value)
                 )
-                span.set_data(SPANDATA.CACHE_ITEM_SIZE, size)
+                data[SPANDATA.CACHE_ITEM_SIZE] = size
             else:
-                span.set_data(SPANDATA.CACHE_HIT, False)
+                data[SPANDATA.CACHE_HIT] = False
 
         elif properties["redis_command"] in SET_COMMANDS:
             if properties["value"] is not None:
@@ -99,7 +101,7 @@ def _set_cache_data(span, redis_client, properties, return_value):
                     if not isinstance(properties["value"], bytes)
                     else len(properties["value"])
                 )
-                span.set_data(SPANDATA.CACHE_ITEM_SIZE, size)
+                data[SPANDATA.CACHE_ITEM_SIZE] = size
 
         try:
             connection_params = redis_client.connection_pool.connection_kwargs
@@ -114,8 +116,10 @@ def _set_cache_data(span, redis_client, properties, return_value):
 
         host = connection_params.get("host")
         if host is not None:
-            span.set_data(SPANDATA.NETWORK_PEER_ADDRESS, host)
+            data[SPANDATA.NETWORK_PEER_ADDRESS] = host
 
         port = connection_params.get("port")
         if port is not None:
-            span.set_data(SPANDATA.NETWORK_PEER_PORT, port)
+            data[SPANDATA.NETWORK_PEER_PORT] = port
+
+    return data

sentry_sdk/integrations/redis/modules/queries.py

@@ -43,26 +43,30 @@ def _get_db_span_description(integration, command_name, args):
     return description
 
 
-def _set_db_data_on_span(span, connection_params):
-    # type: (Span, dict[str, Any]) -> None
-    span.set_data(SPANDATA.DB_SYSTEM, "redis")
+def _get_connection_data(connection_params):
+    # type: (dict[str, Any]) -> dict[str, Any]
+    data = {
+        SPANDATA.DB_SYSTEM: "redis",
+    }
 
     db = connection_params.get("db")
     if db is not None:
-        span.set_data(SPANDATA.DB_NAME, str(db))
+        data[SPANDATA.DB_NAME] = str(db)
 
     host = connection_params.get("host")
     if host is not None:
-        span.set_data(SPANDATA.SERVER_ADDRESS, host)
+        data[SPANDATA.SERVER_ADDRESS] = host
 
     port = connection_params.get("port")
     if port is not None:
-        span.set_data(SPANDATA.SERVER_PORT, port)
+        data[SPANDATA.SERVER_PORT] = port
+
+    return data
 
 
-def _set_db_data(span, redis_instance):
-    # type: (Span, Redis[Any]) -> None
+def _get_db_data(redis_instance):
+    # type: (Redis[Any]) -> dict[str, Any]
     try:
-        _set_db_data_on_span(span, redis_instance.connection_pool.connection_kwargs)
+        return _get_connection_data(redis_instance.connection_pool.connection_kwargs)
     except AttributeError:
-        pass  # connections_kwargs may be missing in some cases
+        return {}  # connections_kwargs may be missing in some cases

sentry_sdk/integrations/redis/rb.py

@@ -5,7 +5,7 @@
 """
 
 from sentry_sdk.integrations.redis._sync_common import patch_redis_client
-from sentry_sdk.integrations.redis.modules.queries import _set_db_data
+from sentry_sdk.integrations.redis.modules.queries import _get_db_data
 
 
 def _patch_rb():
@@ -18,15 +18,15 @@ def _patch_rb():
         patch_redis_client(
             rb.clients.FanoutClient,
             is_cluster=False,
-            set_db_data_fn=_set_db_data,
+            get_db_data_fn=_get_db_data,
         )
         patch_redis_client(
             rb.clients.MappingClient,
             is_cluster=False,
-            set_db_data_fn=_set_db_data,
+            get_db_data_fn=_get_db_data,
         )
         patch_redis_client(
             rb.clients.RoutingClient,
             is_cluster=False,
-            set_db_data_fn=_set_db_data,
+            get_db_data_fn=_get_db_data,
         )

sentry_sdk/integrations/redis/redis.py

@@ -8,7 +8,7 @@
     patch_redis_client,
     patch_redis_pipeline,
 )
-from sentry_sdk.integrations.redis.modules.queries import _set_db_data
+from sentry_sdk.integrations.redis.modules.queries import _get_db_data
 
 from typing import TYPE_CHECKING
 
@@ -26,13 +26,13 @@ def _patch_redis(StrictRedis, client):  # noqa: N803
     patch_redis_client(
         StrictRedis,
         is_cluster=False,
-        set_db_data_fn=_set_db_data,
+        get_db_data_fn=_get_db_data,
     )
     patch_redis_pipeline(
         client.Pipeline,
         is_cluster=False,
         get_command_args_fn=_get_redis_command_args,
-        set_db_data_fn=_set_db_data,
+        get_db_data_fn=_get_db_data,
     )
     try:
         strict_pipeline = client.StrictPipeline
@@ -43,7 +43,7 @@ def _patch_redis(StrictRedis, client):  # noqa: N803
             strict_pipeline,
             is_cluster=False,
             get_command_args_fn=_get_redis_command_args,
-            set_db_data_fn=_set_db_data,
+            get_db_data_fn=_get_db_data,
         )
 
     try:
@@ -59,11 +59,11 @@ def _patch_redis(StrictRedis, client):  # noqa: N803
         patch_redis_async_client(
             redis.asyncio.client.StrictRedis,
             is_cluster=False,
-            set_db_data_fn=_set_db_data,
+            get_db_data_fn=_get_db_data,
         )
         patch_redis_async_pipeline(
             redis.asyncio.client.Pipeline,
             False,
             _get_redis_command_args,
-            set_db_data_fn=_set_db_data,
+            get_db_data_fn=_get_db_data,
         )