From 93fd1598f8926c0964eb90e5d0979a3d3010ed6f Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 09:20:06 +0200 Subject: [PATCH 01/14] Rename _set_db_data_on_span to _get_connection_data --- sentry_sdk/integrations/redis/modules/queries.py | 16 ++++++++++------ sentry_sdk/integrations/redis/redis_cluster.py | 6 +++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/sentry_sdk/integrations/redis/modules/queries.py b/sentry_sdk/integrations/redis/modules/queries.py index e0d85a4ef7..bec7ece403 100644 --- a/sentry_sdk/integrations/redis/modules/queries.py +++ b/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): +def _get_connection_data(span, connection_params): # type: (Span, dict[str, Any]) -> None - span.set_data(SPANDATA.DB_SYSTEM, "redis") + 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 try: - _set_db_data_on_span(span, redis_instance.connection_pool.connection_kwargs) + _get_connection_data(span, redis_instance.connection_pool.connection_kwargs) except AttributeError: pass # connections_kwargs may be missing in some cases diff --git a/sentry_sdk/integrations/redis/redis_cluster.py b/sentry_sdk/integrations/redis/redis_cluster.py index 80cdc7235a..81828d912a 100644 --- a/sentry_sdk/integrations/redis/redis_cluster.py +++ b/sentry_sdk/integrations/redis/redis_cluster.py @@ -9,7 +9,7 @@ patch_redis_client, patch_redis_pipeline, ) -from sentry_sdk.integrations.redis.modules.queries import _set_db_data_on_span +from sentry_sdk.integrations.redis.modules.queries import _get_connection_data from sentry_sdk.integrations.redis.utils import _parse_rediscluster_command from sentry_sdk.utils import capture_internal_exceptions @@ -30,7 +30,7 @@ def _set_async_cluster_db_data(span, async_redis_cluster_instance): # type: (Span, AsyncRedisCluster[Any]) -> None default_node = async_redis_cluster_instance.get_default_node() if default_node is not None and default_node.connection_kwargs is not None: - _set_db_data_on_span(span, default_node.connection_kwargs) + _get_connection_data(span, default_node.connection_kwargs) def _set_async_cluster_pipeline_db_data(span, async_redis_cluster_pipeline_instance): @@ -53,7 +53,7 @@ def _set_cluster_db_data(span, redis_cluster_instance): "host": default_node.host, "port": default_node.port, } - _set_db_data_on_span(span, connection_params) + _get_connection_data(span, connection_params) def _patch_redis_cluster(): From b853560161e531db6c38748c4228b6c172963931 Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 09:28:37 +0200 Subject: [PATCH 02/14] Rename _set_db_data to _get_db_data --- sentry_sdk/integrations/redis/_async_common.py | 8 ++++---- sentry_sdk/integrations/redis/_sync_common.py | 9 +++++---- sentry_sdk/integrations/redis/modules/queries.py | 12 ++++++------ sentry_sdk/integrations/redis/rb.py | 8 ++++---- sentry_sdk/integrations/redis/redis.py | 12 ++++++------ sentry_sdk/integrations/redis/redis_cluster.py | 8 ++++---- .../integrations/redis/redis_py_cluster_legacy.py | 8 ++++---- 7 files changed, 33 insertions(+), 32 deletions(-) diff --git a/sentry_sdk/integrations/redis/_async_common.py b/sentry_sdk/integrations/redis/_async_common.py index 196e85e74b..3abc8c1b1b 100644 --- a/sentry_sdk/integrations/redis/_async_common.py +++ b/sentry_sdk/integrations/redis/_async_common.py @@ -23,7 +23,7 @@ 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 old_execute = pipeline_cls.execute @@ -41,7 +41,7 @@ async def _sentry_execute(self, *args, **kwargs): origin=SPAN_ORIGIN, ) as span: with capture_internal_exceptions(): - set_db_data_fn(span, self) + span_data = get_db_data_fn(self) _set_pipeline_data( span, is_cluster, @@ -55,7 +55,7 @@ async def _sentry_execute(self, *args, **kwargs): pipeline_cls.execute = _sentry_execute # type: ignore -def patch_redis_async_client(cls, is_cluster, set_db_data_fn): +def patch_redis_async_client(cls, is_cluster, get_db_data_fn): # type: (Union[type[StrictRedis[Any]], type[RedisCluster[Any]]], bool, Callable[[Span, Any], None]) -> None old_execute_command = cls.execute_command @@ -92,7 +92,7 @@ async def _sentry_execute_command(self, name, *args, **kwargs): ) db_span.__enter__() - set_db_data_fn(db_span, self) + db_span_data = get_db_data_fn(self) _set_client_data(db_span, is_cluster, name, *args) value = await old_execute_command(self, name, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/_sync_common.py b/sentry_sdk/integrations/redis/_sync_common.py index ef10e9e4f0..2ae2715e57 100644 --- a/sentry_sdk/integrations/redis/_sync_common.py +++ b/sentry_sdk/integrations/redis/_sync_common.py @@ -24,7 +24,7 @@ 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 old_execute = pipeline_cls.execute @@ -36,13 +36,14 @@ def sentry_patched_execute(self, *args, **kwargs): if sentry_sdk.get_client().get_integration(RedisIntegration) is None: return old_execute(self, *args, **kwargs) + with sentry_sdk.start_span( op=OP.DB_REDIS, name="redis.pipeline.execute", origin=SPAN_ORIGIN, ) as span: with capture_internal_exceptions(): - set_db_data_fn(span, self) + span_data = get_db_data_fn(self) _set_pipeline_data( span, is_cluster, @@ -56,7 +57,7 @@ def sentry_patched_execute(self, *args, **kwargs): pipeline_cls.execute = sentry_patched_execute -def patch_redis_client(cls, is_cluster, set_db_data_fn): +def patch_redis_client(cls, is_cluster, get_db_data_fn): # type: (Any, bool, Callable[[Span, Any], None]) -> None """ This function can be used to instrument custom redis client classes or @@ -97,7 +98,7 @@ def sentry_patched_execute_command(self, name, *args, **kwargs): ) db_span.__enter__() - set_db_data_fn(db_span, self) + db_span_data = get_db_data_fn(self) _set_client_data(db_span, is_cluster, name, *args) value = old_execute_command(self, name, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/modules/queries.py b/sentry_sdk/integrations/redis/modules/queries.py index bec7ece403..e2189b7f9c 100644 --- a/sentry_sdk/integrations/redis/modules/queries.py +++ b/sentry_sdk/integrations/redis/modules/queries.py @@ -43,8 +43,8 @@ def _get_db_span_description(integration, command_name, args): return description -def _get_connection_data(span, connection_params): - # type: (Span, dict[str, Any]) -> None +def _get_connection_data(connection_params): + # type: (dict[str, Any]) -> dict[str, Any] data = { SPANDATA.DB_SYSTEM: "redis", } @@ -64,9 +64,9 @@ def _get_connection_data(span, connection_params): 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: - _get_connection_data(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 diff --git a/sentry_sdk/integrations/redis/rb.py b/sentry_sdk/integrations/redis/rb.py index 1b3e2e530c..68d3c3a9d6 100644 --- a/sentry_sdk/integrations/redis/rb.py +++ b/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, ) diff --git a/sentry_sdk/integrations/redis/redis.py b/sentry_sdk/integrations/redis/redis.py index c92958a32d..935a828c3d 100644 --- a/sentry_sdk/integrations/redis/redis.py +++ b/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, ) diff --git a/sentry_sdk/integrations/redis/redis_cluster.py b/sentry_sdk/integrations/redis/redis_cluster.py index 81828d912a..56ace6ca49 100644 --- a/sentry_sdk/integrations/redis/redis_cluster.py +++ b/sentry_sdk/integrations/redis/redis_cluster.py @@ -67,13 +67,13 @@ def _patch_redis_cluster(): patch_redis_client( RedisCluster, is_cluster=True, - set_db_data_fn=_set_cluster_db_data, + get_db_data_fn=_set_cluster_db_data, ) patch_redis_pipeline( cluster.ClusterPipeline, is_cluster=True, get_command_args_fn=_parse_rediscluster_command, - set_db_data_fn=_set_cluster_db_data, + get_db_data_fn=_set_cluster_db_data, ) try: @@ -89,11 +89,11 @@ def _patch_redis_cluster(): patch_redis_async_client( async_cluster.RedisCluster, is_cluster=True, - set_db_data_fn=_set_async_cluster_db_data, + get_db_data_fn=_set_async_cluster_db_data, ) patch_redis_async_pipeline( async_cluster.ClusterPipeline, is_cluster=True, get_command_args_fn=_parse_rediscluster_command, - set_db_data_fn=_set_async_cluster_pipeline_db_data, + get_db_data_fn=_set_async_cluster_pipeline_db_data, ) diff --git a/sentry_sdk/integrations/redis/redis_py_cluster_legacy.py b/sentry_sdk/integrations/redis/redis_py_cluster_legacy.py index ad1c23633f..53b545c21b 100644 --- a/sentry_sdk/integrations/redis/redis_py_cluster_legacy.py +++ b/sentry_sdk/integrations/redis/redis_py_cluster_legacy.py @@ -9,7 +9,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 sentry_sdk.integrations.redis.utils import _parse_rediscluster_command @@ -23,7 +23,7 @@ def _patch_rediscluster(): patch_redis_client( rediscluster.RedisCluster, is_cluster=True, - set_db_data_fn=_set_db_data, + get_db_data_fn=_get_db_data, ) # up to v1.3.6, __version__ attribute is a tuple @@ -37,7 +37,7 @@ def _patch_rediscluster(): patch_redis_client( rediscluster.StrictRedisCluster, is_cluster=True, - set_db_data_fn=_set_db_data, + get_db_data_fn=_get_db_data, ) else: pipeline_cls = rediscluster.pipeline.ClusterPipeline @@ -46,5 +46,5 @@ def _patch_rediscluster(): pipeline_cls, is_cluster=True, get_command_args_fn=_parse_rediscluster_command, - set_db_data_fn=_set_db_data, + get_db_data_fn=_get_db_data, ) From 54e1e9fc1a4dbc15d0b22b76306d2a7af31ffdff Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 09:32:27 +0200 Subject: [PATCH 03/14] Rename _set_cluster_db_data to _get_cluster_db_data --- sentry_sdk/integrations/redis/redis_cluster.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/sentry_sdk/integrations/redis/redis_cluster.py b/sentry_sdk/integrations/redis/redis_cluster.py index 56ace6ca49..e6b9c32e9f 100644 --- a/sentry_sdk/integrations/redis/redis_cluster.py +++ b/sentry_sdk/integrations/redis/redis_cluster.py @@ -44,8 +44,8 @@ def _set_async_cluster_pipeline_db_data(span, async_redis_cluster_pipeline_insta ) -def _set_cluster_db_data(span, redis_cluster_instance): - # type: (Span, RedisCluster[Any]) -> None +def _get_cluster_db_data(redis_cluster_instance): + # type: (RedisCluster[Any]) -> dict[str, Any] default_node = redis_cluster_instance.get_default_node() if default_node is not None: @@ -53,7 +53,9 @@ def _set_cluster_db_data(span, redis_cluster_instance): "host": default_node.host, "port": default_node.port, } - _get_connection_data(span, connection_params) + return _get_connection_data(connection_params) + else: + return {} def _patch_redis_cluster(): @@ -67,13 +69,13 @@ def _patch_redis_cluster(): patch_redis_client( RedisCluster, is_cluster=True, - get_db_data_fn=_set_cluster_db_data, + get_db_data_fn=_get_cluster_db_data, ) patch_redis_pipeline( cluster.ClusterPipeline, is_cluster=True, get_command_args_fn=_parse_rediscluster_command, - get_db_data_fn=_set_cluster_db_data, + get_db_data_fn=_get_cluster_db_data, ) try: From 7275d57eabc09f2a9bbd81d3d561a3caeb4edfe6 Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 09:35:59 +0200 Subject: [PATCH 04/14] Renamce _set_async_cluster_db_data to _get_async_cluster_db_data --- sentry_sdk/integrations/redis/redis_cluster.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/sentry_sdk/integrations/redis/redis_cluster.py b/sentry_sdk/integrations/redis/redis_cluster.py index e6b9c32e9f..f8c87baf19 100644 --- a/sentry_sdk/integrations/redis/redis_cluster.py +++ b/sentry_sdk/integrations/redis/redis_cluster.py @@ -26,18 +26,19 @@ from sentry_sdk.tracing import Span -def _set_async_cluster_db_data(span, async_redis_cluster_instance): - # type: (Span, AsyncRedisCluster[Any]) -> None +def _get_async_cluster_db_data(async_redis_cluster_instance): + # type: (AsyncRedisCluster[Any]) -> dict[str, Any] default_node = async_redis_cluster_instance.get_default_node() if default_node is not None and default_node.connection_kwargs is not None: - _get_connection_data(span, default_node.connection_kwargs) + return _get_connection_data(default_node.connection_kwargs) + else: + return {} def _set_async_cluster_pipeline_db_data(span, async_redis_cluster_pipeline_instance): - # type: (Span, AsyncClusterPipeline[Any]) -> None + # type: (Span, AsyncClusterPipeline[Any]) -> dict[str, Any] with capture_internal_exceptions(): - _set_async_cluster_db_data( - span, + return _get_async_cluster_db_data( # the AsyncClusterPipeline has always had a `_client` attr but it is private so potentially problematic and mypy # does not recognize it - see https://github.com/redis/redis-py/blame/v5.0.0/redis/asyncio/cluster.py#L1386 async_redis_cluster_pipeline_instance._client, # type: ignore[attr-defined] @@ -91,7 +92,7 @@ def _patch_redis_cluster(): patch_redis_async_client( async_cluster.RedisCluster, is_cluster=True, - get_db_data_fn=_set_async_cluster_db_data, + get_db_data_fn=_get_async_cluster_db_data, ) patch_redis_async_pipeline( async_cluster.ClusterPipeline, From 57d19302dce48cc1e972edae8bb1c97ad03ada87 Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 09:36:41 +0200 Subject: [PATCH 05/14] Rename _set_async_cluster_pipeline_db_data to _get_async_cluster_pipeline_db_data --- sentry_sdk/integrations/redis/redis_cluster.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sentry_sdk/integrations/redis/redis_cluster.py b/sentry_sdk/integrations/redis/redis_cluster.py index f8c87baf19..dbcd20a65d 100644 --- a/sentry_sdk/integrations/redis/redis_cluster.py +++ b/sentry_sdk/integrations/redis/redis_cluster.py @@ -35,8 +35,8 @@ def _get_async_cluster_db_data(async_redis_cluster_instance): return {} -def _set_async_cluster_pipeline_db_data(span, async_redis_cluster_pipeline_instance): - # type: (Span, AsyncClusterPipeline[Any]) -> dict[str, Any] +def _get_async_cluster_pipeline_db_data(async_redis_cluster_pipeline_instance): + # type: (AsyncClusterPipeline[Any]) -> dict[str, Any] with capture_internal_exceptions(): return _get_async_cluster_db_data( # the AsyncClusterPipeline has always had a `_client` attr but it is private so potentially problematic and mypy @@ -98,5 +98,5 @@ def _patch_redis_cluster(): async_cluster.ClusterPipeline, is_cluster=True, get_command_args_fn=_parse_rediscluster_command, - get_db_data_fn=_set_async_cluster_pipeline_db_data, + get_db_data_fn=_get_async_cluster_pipeline_db_data, ) From c2dfdc385f0263cefa3a989e8c50d747606dbbf6 Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 09:45:43 +0200 Subject: [PATCH 06/14] Rename _get_pipeline_data to _set_pipeline_data --- sentry_sdk/integrations/redis/_async_common.py | 13 ++++++------- sentry_sdk/integrations/redis/_sync_common.py | 13 ++++++------- sentry_sdk/integrations/redis/utils.py | 18 +++++++++++------- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/sentry_sdk/integrations/redis/_async_common.py b/sentry_sdk/integrations/redis/_async_common.py index 3abc8c1b1b..43de49fa3e 100644 --- a/sentry_sdk/integrations/redis/_async_common.py +++ b/sentry_sdk/integrations/redis/_async_common.py @@ -8,7 +8,7 @@ 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, + _get_pipeline_data, ) from sentry_sdk.tracing import Span from sentry_sdk.utils import capture_internal_exceptions @@ -42,12 +42,11 @@ async def _sentry_execute(self, *args, **kwargs): ) as span: with capture_internal_exceptions(): span_data = get_db_data_fn(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, + 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, ) return await old_execute(self, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/_sync_common.py b/sentry_sdk/integrations/redis/_sync_common.py index 2ae2715e57..56aebb3ba4 100644 --- a/sentry_sdk/integrations/redis/_sync_common.py +++ b/sentry_sdk/integrations/redis/_sync_common.py @@ -8,7 +8,7 @@ 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, + _get_pipeline_data, ) from sentry_sdk.tracing import Span from sentry_sdk.utils import capture_internal_exceptions @@ -44,12 +44,11 @@ def sentry_patched_execute(self, *args, **kwargs): ) as span: with capture_internal_exceptions(): span_data = get_db_data_fn(self) - _set_pipeline_data( - span, - is_cluster, - get_command_args_fn, - False if is_cluster else self.transaction, - self.command_stack, + 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, ) return old_execute(self, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/utils.py b/sentry_sdk/integrations/redis/utils.py index 894c56305b..7b7255a5bf 100644 --- a/sentry_sdk/integrations/redis/utils.py +++ b/sentry_sdk/integrations/redis/utils.py @@ -105,12 +105,14 @@ def _parse_rediscluster_command(command): return command.args -def _set_pipeline_data( - span, is_cluster, get_command_args_fn, is_transaction, command_stack +def _get_pipeline_data( + is_cluster, get_command_args_fn, is_transaction, command_stack ): - # type: (Span, bool, Any, bool, Sequence[Any]) -> None - span.set_tag("redis.is_cluster", is_cluster) - span.set_tag("redis.transaction", is_transaction) + # type: (bool, Any, bool, Sequence[Any]) -> dict[str, Any] + data = { + "redis.is_cluster": is_cluster, + "redis.transaction": is_transaction, + } commands = [] for i, arg in enumerate(command_stack): @@ -120,8 +122,10 @@ def _set_pipeline_data( command = get_command_args_fn(arg) commands.append(_get_safe_command(command[0], command[1:])) - span.set_data("redis.commands.count", len(command_stack)) - span.set_data("redis.commands.first_ten", commands) + data["redis.commands.count"] = len(command_stack) + data["redis.commands.first_ten"] = commands + + return data def _set_client_data(span, is_cluster, name, *args): From 7c10709c549e2015d90f98953755c89311b62fb2 Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 09:50:42 +0200 Subject: [PATCH 07/14] Rename _set_cache_data to _get_cache_data --- .../integrations/redis/_async_common.py | 4 ++-- sentry_sdk/integrations/redis/_sync_common.py | 4 ++-- .../integrations/redis/modules/caches.py | 22 +++++++++++-------- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/sentry_sdk/integrations/redis/_async_common.py b/sentry_sdk/integrations/redis/_async_common.py index 43de49fa3e..550cdf52fd 100644 --- a/sentry_sdk/integrations/redis/_async_common.py +++ b/sentry_sdk/integrations/redis/_async_common.py @@ -3,7 +3,7 @@ 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 ( @@ -99,7 +99,7 @@ async def _sentry_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) cache_span.__exit__(None, None, None) return value diff --git a/sentry_sdk/integrations/redis/_sync_common.py b/sentry_sdk/integrations/redis/_sync_common.py index 56aebb3ba4..343a5faf64 100644 --- a/sentry_sdk/integrations/redis/_sync_common.py +++ b/sentry_sdk/integrations/redis/_sync_common.py @@ -3,7 +3,7 @@ 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 ( @@ -105,7 +105,7 @@ def sentry_patched_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) cache_span.__exit__(None, None, None) return value diff --git a/sentry_sdk/integrations/redis/modules/caches.py b/sentry_sdk/integrations/redis/modules/caches.py index c6fc19f5b2..d93e729f2b 100644 --- a/sentry_sdk/integrations/redis/modules/caches.py +++ b/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 From 05569f18d6ff7f7440625852d25c1b8fa0d3756f Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 09:53:50 +0200 Subject: [PATCH 08/14] Rename _set_client_data to _get_client_data --- sentry_sdk/integrations/redis/_async_common.py | 4 ++-- sentry_sdk/integrations/redis/_sync_common.py | 4 ++-- sentry_sdk/integrations/redis/utils.py | 16 ++++++++++------ 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/sentry_sdk/integrations/redis/_async_common.py b/sentry_sdk/integrations/redis/_async_common.py index 550cdf52fd..7e012474c1 100644 --- a/sentry_sdk/integrations/redis/_async_common.py +++ b/sentry_sdk/integrations/redis/_async_common.py @@ -7,7 +7,7 @@ ) from sentry_sdk.integrations.redis.modules.queries import _compile_db_span_properties from sentry_sdk.integrations.redis.utils import ( - _set_client_data, + _get_client_data, _get_pipeline_data, ) from sentry_sdk.tracing import Span @@ -92,7 +92,7 @@ async def _sentry_execute_command(self, name, *args, **kwargs): db_span.__enter__() db_span_data = get_db_data_fn(self) - _set_client_data(db_span, is_cluster, name, *args) + db_client_span_data = _get_client_data(db_span, is_cluster, name, *args) value = await old_execute_command(self, name, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/_sync_common.py b/sentry_sdk/integrations/redis/_sync_common.py index 343a5faf64..d7cdeb5a28 100644 --- a/sentry_sdk/integrations/redis/_sync_common.py +++ b/sentry_sdk/integrations/redis/_sync_common.py @@ -7,7 +7,7 @@ ) from sentry_sdk.integrations.redis.modules.queries import _compile_db_span_properties from sentry_sdk.integrations.redis.utils import ( - _set_client_data, + _get_client_data, _get_pipeline_data, ) from sentry_sdk.tracing import Span @@ -98,7 +98,7 @@ def sentry_patched_execute_command(self, name, *args, **kwargs): db_span.__enter__() db_span_data = get_db_data_fn(self) - _set_client_data(db_span, is_cluster, name, *args) + db_client_span_data = _get_client_data(db_span, is_cluster, name, *args) value = old_execute_command(self, name, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/utils.py b/sentry_sdk/integrations/redis/utils.py index 7b7255a5bf..006601d926 100644 --- a/sentry_sdk/integrations/redis/utils.py +++ b/sentry_sdk/integrations/redis/utils.py @@ -128,16 +128,20 @@ def _get_pipeline_data( return data -def _set_client_data(span, is_cluster, name, *args): - # type: (Span, bool, str, *Any) -> None - span.set_tag("redis.is_cluster", is_cluster) +def _get_client_data(is_cluster, name, *args): + # type: (bool, str, *Any) -> dict[str, Any] + data = { + "redis.is_cluster": is_cluster, + } if name: - span.set_tag("redis.command", name) - span.set_tag(SPANDATA.DB_OPERATION, name) + data["redis.command"] = name + data[SPANDATA.DB_OPERATION] = name if name and args: name_low = name.lower() if (name_low in _SINGLE_KEY_COMMANDS) or ( name_low in _MULTI_KEY_COMMANDS and len(args) == 1 ): - span.set_tag("redis.key", args[0]) + data["redis.key"] = args[0] + + return data \ No newline at end of file From 544542781983d198bca916be3141c09f4b99933d Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 10:03:49 +0200 Subject: [PATCH 09/14] After gathering all the data, set it on the span --- .../integrations/redis/_async_common.py | 6 +++++- sentry_sdk/integrations/redis/_sync_common.py | 4 ++++ sentry_sdk/integrations/redis/utils.py | 19 +++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/sentry_sdk/integrations/redis/_async_common.py b/sentry_sdk/integrations/redis/_async_common.py index 7e012474c1..59092b3d80 100644 --- a/sentry_sdk/integrations/redis/_async_common.py +++ b/sentry_sdk/integrations/redis/_async_common.py @@ -9,6 +9,7 @@ from sentry_sdk.integrations.redis.utils import ( _get_client_data, _get_pipeline_data, + _update_span, ) from sentry_sdk.tracing import Span from sentry_sdk.utils import capture_internal_exceptions @@ -48,6 +49,7 @@ async def _sentry_execute(self, *args, **kwargs): 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) return await old_execute(self, *args, **kwargs) @@ -92,7 +94,8 @@ async def _sentry_execute_command(self, name, *args, **kwargs): db_span.__enter__() db_span_data = get_db_data_fn(self) - db_client_span_data = _get_client_data(db_span, is_cluster, name, *args) + db_client_span_data = _get_client_data(is_cluster, name, *args) + _update_span(db_span, db_span_data, db_client_span_data) value = await old_execute_command(self, name, *args, **kwargs) @@ -100,6 +103,7 @@ async def _sentry_execute_command(self, name, *args, **kwargs): if cache_span: 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 diff --git a/sentry_sdk/integrations/redis/_sync_common.py b/sentry_sdk/integrations/redis/_sync_common.py index d7cdeb5a28..aefbc1e49c 100644 --- a/sentry_sdk/integrations/redis/_sync_common.py +++ b/sentry_sdk/integrations/redis/_sync_common.py @@ -9,6 +9,7 @@ from sentry_sdk.integrations.redis.utils import ( _get_client_data, _get_pipeline_data, + _update_span, ) from sentry_sdk.tracing import Span from sentry_sdk.utils import capture_internal_exceptions @@ -50,6 +51,7 @@ def sentry_patched_execute(self, *args, **kwargs): is_transaction=False if is_cluster else self.transaction, command_stack=self.command_stack, ) + _update_span(span, span_data, pipeline_data) return old_execute(self, *args, **kwargs) @@ -99,6 +101,7 @@ def sentry_patched_execute_command(self, name, *args, **kwargs): db_span_data = get_db_data_fn(self) db_client_span_data = _get_client_data(db_span, is_cluster, name, *args) + _update_span(db_span, db_span_data, db_client_span_data) value = old_execute_command(self, name, *args, **kwargs) @@ -106,6 +109,7 @@ def sentry_patched_execute_command(self, name, *args, **kwargs): if cache_span: 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 diff --git a/sentry_sdk/integrations/redis/utils.py b/sentry_sdk/integrations/redis/utils.py index 006601d926..d60f365bad 100644 --- a/sentry_sdk/integrations/redis/utils.py +++ b/sentry_sdk/integrations/redis/utils.py @@ -16,6 +16,25 @@ from sentry_sdk.tracing import Span +TAG_KEYS = [ + "redis.commands", + "redis.is_cluster", + "redis.key", + "redis.transaction", + SPANDATA.DB_OPERATION, +] + + +def _update_span(span, *data_bags): + # type: (Span, *dict[str, Any]) -> None + for data in data_bags: + for key, value in data.items(): + if key in TAG_KEYS: + span.set_tag(key, value) + else: + span.set_data(key, value) + + def _get_safe_command(name, args): # type: (str, Sequence[Any]) -> str command_parts = [name] From fb7bfefe2d488b4cdc393c34eb399ab160e3dfdb Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 10:12:38 +0200 Subject: [PATCH 10/14] Linting --- sentry_sdk/integrations/redis/_async_common.py | 8 +++++--- sentry_sdk/integrations/redis/_sync_common.py | 7 +++---- sentry_sdk/integrations/redis/utils.py | 11 +++++------ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/sentry_sdk/integrations/redis/_async_common.py b/sentry_sdk/integrations/redis/_async_common.py index 59092b3d80..9474c1fdf0 100644 --- a/sentry_sdk/integrations/redis/_async_common.py +++ b/sentry_sdk/integrations/redis/_async_common.py @@ -26,7 +26,7 @@ def patch_redis_async_pipeline( 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 @@ -47,7 +47,9 @@ async def _sentry_execute(self, *args, **kwargs): 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, + command_stack=( + self._command_stack if is_cluster else self.command_stack + ), ) _update_span(span, span_data, pipeline_data) @@ -57,7 +59,7 @@ async def _sentry_execute(self, *args, **kwargs): def patch_redis_async_client(cls, is_cluster, get_db_data_fn): - # type: (Union[type[StrictRedis[Any]], type[RedisCluster[Any]]], bool, Callable[[Span, Any], None]) -> None + # 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 diff --git a/sentry_sdk/integrations/redis/_sync_common.py b/sentry_sdk/integrations/redis/_sync_common.py index aefbc1e49c..a41b30354f 100644 --- a/sentry_sdk/integrations/redis/_sync_common.py +++ b/sentry_sdk/integrations/redis/_sync_common.py @@ -27,7 +27,7 @@ def patch_redis_pipeline( get_command_args_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 @@ -37,7 +37,6 @@ def sentry_patched_execute(self, *args, **kwargs): if sentry_sdk.get_client().get_integration(RedisIntegration) is None: return old_execute(self, *args, **kwargs) - with sentry_sdk.start_span( op=OP.DB_REDIS, name="redis.pipeline.execute", @@ -59,7 +58,7 @@ def sentry_patched_execute(self, *args, **kwargs): def patch_redis_client(cls, is_cluster, get_db_data_fn): - # type: (Any, bool, Callable[[Span, Any], None]) -> None + # type: (Any, bool, Callable[[Any], dict[str, Any]]) -> None """ This function can be used to instrument custom redis client classes or subclasses. @@ -100,7 +99,7 @@ def sentry_patched_execute_command(self, name, *args, **kwargs): db_span.__enter__() db_span_data = get_db_data_fn(self) - db_client_span_data = _get_client_data(db_span, is_cluster, name, *args) + db_client_span_data = _get_client_data(is_cluster, name, *args) _update_span(db_span, db_span_data, db_client_span_data) value = old_execute_command(self, name, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/utils.py b/sentry_sdk/integrations/redis/utils.py index d60f365bad..b60a5ab7b7 100644 --- a/sentry_sdk/integrations/redis/utils.py +++ b/sentry_sdk/integrations/redis/utils.py @@ -124,14 +124,12 @@ def _parse_rediscluster_command(command): return command.args -def _get_pipeline_data( - is_cluster, get_command_args_fn, is_transaction, command_stack -): +def _get_pipeline_data(is_cluster, get_command_args_fn, is_transaction, command_stack): # type: (bool, Any, bool, Sequence[Any]) -> dict[str, Any] data = { "redis.is_cluster": is_cluster, "redis.transaction": is_transaction, - } + } # type: dict[str, Any] commands = [] for i, arg in enumerate(command_stack): @@ -151,7 +149,8 @@ def _get_client_data(is_cluster, name, *args): # type: (bool, str, *Any) -> dict[str, Any] data = { "redis.is_cluster": is_cluster, - } + } # type: dict[str, Any] + if name: data["redis.command"] = name data[SPANDATA.DB_OPERATION] = name @@ -163,4 +162,4 @@ def _get_client_data(is_cluster, name, *args): ): data["redis.key"] = args[0] - return data \ No newline at end of file + return data From 6e129b6ded9a174729a8480f32563fb0a812e2ae Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 10:25:03 +0200 Subject: [PATCH 11/14] Now create the breadcrumbs --- .../integrations/redis/_async_common.py | 3 +++ sentry_sdk/integrations/redis/_sync_common.py | 3 +++ sentry_sdk/integrations/redis/utils.py | 21 +++++++++++++++++++ 3 files changed, 27 insertions(+) diff --git a/sentry_sdk/integrations/redis/_async_common.py b/sentry_sdk/integrations/redis/_async_common.py index 9474c1fdf0..0c4873c1a2 100644 --- a/sentry_sdk/integrations/redis/_async_common.py +++ b/sentry_sdk/integrations/redis/_async_common.py @@ -7,6 +7,7 @@ ) from sentry_sdk.integrations.redis.modules.queries import _compile_db_span_properties from sentry_sdk.integrations.redis.utils import ( + _create_breadcrumb, _get_client_data, _get_pipeline_data, _update_span, @@ -52,6 +53,7 @@ async def _sentry_execute(self, *args, **kwargs): ), ) _update_span(span, span_data, pipeline_data) + _create_breadcrumb("redis.pipeline.execute", span_data, pipeline_data) return await old_execute(self, *args, **kwargs) @@ -98,6 +100,7 @@ async def _sentry_execute_command(self, name, *args, **kwargs): 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["op"], db_span_data, db_client_span_data) value = await old_execute_command(self, name, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/_sync_common.py b/sentry_sdk/integrations/redis/_sync_common.py index a41b30354f..be54f460e1 100644 --- a/sentry_sdk/integrations/redis/_sync_common.py +++ b/sentry_sdk/integrations/redis/_sync_common.py @@ -7,6 +7,7 @@ ) from sentry_sdk.integrations.redis.modules.queries import _compile_db_span_properties from sentry_sdk.integrations.redis.utils import ( + _create_breadcrumb, _get_client_data, _get_pipeline_data, _update_span, @@ -51,6 +52,7 @@ def sentry_patched_execute(self, *args, **kwargs): 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) @@ -101,6 +103,7 @@ def sentry_patched_execute_command(self, name, *args, **kwargs): 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["op"], db_span_data, db_client_span_data) value = old_execute_command(self, name, *args, **kwargs) diff --git a/sentry_sdk/integrations/redis/utils.py b/sentry_sdk/integrations/redis/utils.py index b60a5ab7b7..b9809d47e0 100644 --- a/sentry_sdk/integrations/redis/utils.py +++ b/sentry_sdk/integrations/redis/utils.py @@ -1,3 +1,4 @@ +import sentry_sdk from sentry_sdk.consts import SPANDATA from sentry_sdk.integrations.redis.consts import ( _COMMANDS_INCLUDING_SENSITIVE_DATA, @@ -27,6 +28,9 @@ def _update_span(span, *data_bags): # type: (Span, *dict[str, Any]) -> None + """ + Set tags and data on the given span to data from the given data bags. + """ for data in data_bags: for key, value in data.items(): if key in TAG_KEYS: @@ -35,6 +39,23 @@ def _update_span(span, *data_bags): span.set_data(key, value) +def _create_breadcrumb(message, *data_bags): + # type: (str, *dict[str, Any]) -> None + """ + Create a breadcrumb containing the data from the given data bags. + """ + combined_data = {} + for data in data_bags: + combined_data.update(data) + + sentry_sdk.add_breadcrumb( + message=message, + type="redis", + category="redis", + data=combined_data, + ) + + def _get_safe_command(name, args): # type: (str, Sequence[Any]) -> str command_parts = [name] From 6979215f5cbe5685a42c6a8970eb78d8a4ac2bbe Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 10:25:35 +0200 Subject: [PATCH 12/14] Do not create the redis breadcrumbs in tracing code (because this is done in the integration now --- sentry_sdk/tracing_utils.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/sentry_sdk/tracing_utils.py b/sentry_sdk/tracing_utils.py index f063897cb9..2c6e7f2a7a 100644 --- a/sentry_sdk/tracing_utils.py +++ b/sentry_sdk/tracing_utils.py @@ -157,14 +157,7 @@ def record_sql_queries( def maybe_create_breadcrumbs_from_span(scope, span): # type: (sentry_sdk.Scope, sentry_sdk.tracing.Span) -> None - if span.op == OP.DB_REDIS: - scope.add_breadcrumb( - message=span.description, - type="redis", - category="redis", - data=span._tags, - ) - elif span.op == OP.HTTP_CLIENT: + if span.op == OP.HTTP_CLIENT: scope.add_breadcrumb( type="http", category="httplib", From 9bfb8bc7b86a5d930bb62e043a80c1bbc9a91a6c Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 12:14:24 +0200 Subject: [PATCH 13/14] Fixed some tests --- sentry_sdk/integrations/redis/_async_common.py | 4 +++- sentry_sdk/integrations/redis/_sync_common.py | 4 +++- sentry_sdk/integrations/redis/utils.py | 12 +++++++----- tests/integrations/redis/test_redis_cache_module.py | 11 ++++++----- 4 files changed, 19 insertions(+), 12 deletions(-) diff --git a/sentry_sdk/integrations/redis/_async_common.py b/sentry_sdk/integrations/redis/_async_common.py index 0c4873c1a2..6a136fe29a 100644 --- a/sentry_sdk/integrations/redis/_async_common.py +++ b/sentry_sdk/integrations/redis/_async_common.py @@ -100,7 +100,9 @@ async def _sentry_execute_command(self, name, *args, **kwargs): 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["op"], 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) diff --git a/sentry_sdk/integrations/redis/_sync_common.py b/sentry_sdk/integrations/redis/_sync_common.py index be54f460e1..f4cb6ee1b8 100644 --- a/sentry_sdk/integrations/redis/_sync_common.py +++ b/sentry_sdk/integrations/redis/_sync_common.py @@ -103,7 +103,9 @@ def sentry_patched_execute_command(self, name, *args, **kwargs): 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["op"], 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) diff --git a/sentry_sdk/integrations/redis/utils.py b/sentry_sdk/integrations/redis/utils.py index b9809d47e0..9eb16c5bc4 100644 --- a/sentry_sdk/integrations/redis/utils.py +++ b/sentry_sdk/integrations/redis/utils.py @@ -18,7 +18,7 @@ TAG_KEYS = [ - "redis.commands", + "redis.command", "redis.is_cluster", "redis.key", "redis.transaction", @@ -42,17 +42,19 @@ def _update_span(span, *data_bags): def _create_breadcrumb(message, *data_bags): # type: (str, *dict[str, Any]) -> None """ - Create a breadcrumb containing the data from the given data bags. + Create a breadcrumb containing the tags data from the given data bags. """ - combined_data = {} + data = {} for data in data_bags: - combined_data.update(data) + for key, value in data.items(): + if key in TAG_KEYS: + data[key] = value sentry_sdk.add_breadcrumb( message=message, type="redis", category="redis", - data=combined_data, + data=data, ) diff --git a/tests/integrations/redis/test_redis_cache_module.py b/tests/integrations/redis/test_redis_cache_module.py index f118aa53f5..75455211ce 100644 --- a/tests/integrations/redis/test_redis_cache_module.py +++ b/tests/integrations/redis/test_redis_cache_module.py @@ -28,7 +28,7 @@ def test_no_cache_basic(sentry_init, capture_events): connection.get("mycachekey") (event,) = events - spans = event["spans"] + spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) assert len(spans) == 1 assert spans[0]["op"] == "db.redis" @@ -53,7 +53,7 @@ def test_cache_basic(sentry_init, capture_events): connection.mget("mycachekey1", "mycachekey2") (event,) = events - spans = event["spans"] + spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) assert len(spans) == 9 # no cache support for hget command @@ -96,7 +96,8 @@ def test_cache_keys(sentry_init, capture_events): connection.get("bl") (event,) = events - spans = event["spans"] + spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) + assert len(spans) == 6 assert spans[0]["op"] == "db.redis" assert spans[0]["description"] == "GET 'somethingelse'" @@ -133,7 +134,7 @@ def test_cache_data(sentry_init, capture_events): connection.get("mycachekey") (event,) = events - spans = event["spans"] + spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) assert len(spans) == 6 @@ -222,7 +223,7 @@ def test_cache_prefixes(sentry_init, capture_events): (event,) = events - spans = event["spans"] + spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) assert len(spans) == 13 # 8 db spans + 5 cache spans cache_spans = [span for span in spans if span["op"] == "cache.get"] From f4207dfc477b0871b74668507f23d7c05fb335ef Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 22 Oct 2024 13:35:00 +0200 Subject: [PATCH 14/14] Using render_span_tree --- tests/integrations/redis/test_redis.py | 76 ++++++++++------- .../redis/test_redis_cache_module.py | 84 +++++++++---------- .../redis/test_redis_cache_module_async.py | 60 ++++++------- 3 files changed, 115 insertions(+), 105 deletions(-) diff --git a/tests/integrations/redis/test_redis.py b/tests/integrations/redis/test_redis.py index aff5b3ec9b..f8225bed79 100644 --- a/tests/integrations/redis/test_redis.py +++ b/tests/integrations/redis/test_redis.py @@ -80,7 +80,7 @@ def test_redis_pipeline( } -def test_sensitive_data(sentry_init, capture_events): +def test_sensitive_data(sentry_init, capture_events, render_span_tree): # fakeredis does not support the AUTH command, so we need to mock it with mock.patch( "sentry_sdk.integrations.redis.utils._COMMANDS_INCLUDING_SENSITIVE_DATA", @@ -100,12 +100,16 @@ def test_sensitive_data(sentry_init, capture_events): ) # because fakeredis does not support AUTH we use GET instead (event,) = events - spans = event["spans"] - assert spans[0]["op"] == "db.redis" - assert spans[0]["description"] == "GET [Filtered]" + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="db.redis": description="GET [Filtered]"\ +""" + ) -def test_pii_data_redacted(sentry_init, capture_events): +def test_pii_data_redacted(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[RedisIntegration()], traces_sample_rate=1.0, @@ -120,15 +124,19 @@ def test_pii_data_redacted(sentry_init, capture_events): connection.delete("somekey1", "somekey2") (event,) = events - spans = event["spans"] - assert spans[0]["op"] == "db.redis" - assert spans[0]["description"] == "SET 'somekey1' [Filtered]" - assert spans[1]["description"] == "SET 'somekey2' [Filtered]" - assert spans[2]["description"] == "GET 'somekey2'" - assert spans[3]["description"] == "DEL 'somekey1' [Filtered]" + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="db.redis": description="SET 'somekey1' [Filtered]" + - op="db.redis": description="SET 'somekey2' [Filtered]" + - op="db.redis": description="GET 'somekey2'" + - op="db.redis": description="DEL 'somekey1' [Filtered]"\ +""" + ) -def test_pii_data_sent(sentry_init, capture_events): +def test_pii_data_sent(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[RedisIntegration()], traces_sample_rate=1.0, @@ -144,15 +152,19 @@ def test_pii_data_sent(sentry_init, capture_events): connection.delete("somekey1", "somekey2") (event,) = events - spans = event["spans"] - assert spans[0]["op"] == "db.redis" - assert spans[0]["description"] == "SET 'somekey1' 'my secret string1'" - assert spans[1]["description"] == "SET 'somekey2' 'my secret string2'" - assert spans[2]["description"] == "GET 'somekey2'" - assert spans[3]["description"] == "DEL 'somekey1' 'somekey2'" + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="db.redis": description="SET 'somekey1' 'my secret string1'" + - op="db.redis": description="SET 'somekey2' 'my secret string2'" + - op="db.redis": description="GET 'somekey2'" + - op="db.redis": description="DEL 'somekey1' 'somekey2'"\ +""" + ) -def test_data_truncation(sentry_init, capture_events): +def test_data_truncation(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[RedisIntegration()], traces_sample_rate=1.0, @@ -168,15 +180,17 @@ def test_data_truncation(sentry_init, capture_events): connection.set("somekey2", short_string) (event,) = events - spans = event["spans"] - assert spans[0]["op"] == "db.redis" - assert spans[0]["description"] == "SET 'somekey1' '%s..." % ( - long_string[: 1024 - len("...") - len("SET 'somekey1' '")], + assert ( + render_span_tree(event) + == f"""\ +- op="": description=null + - op="db.redis": description="SET 'somekey1' '{long_string[: 1024 - len("...") - len("SET 'somekey1' '")]}..." + - op="db.redis": description="SET 'somekey2' 'bbbbbbbbbb'"\ +""" ) - assert spans[1]["description"] == "SET 'somekey2' '%s'" % (short_string,) -def test_data_truncation_custom(sentry_init, capture_events): +def test_data_truncation_custom(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[RedisIntegration(max_data_size=30)], traces_sample_rate=1.0, @@ -192,12 +206,14 @@ def test_data_truncation_custom(sentry_init, capture_events): connection.set("somekey2", short_string) (event,) = events - spans = event["spans"] - assert spans[0]["op"] == "db.redis" - assert spans[0]["description"] == "SET 'somekey1' '%s..." % ( - long_string[: 30 - len("...") - len("SET 'somekey1' '")], + assert ( + render_span_tree(event) + == f"""\ +- op="": description=null + - op="db.redis": description="SET 'somekey1' '{long_string[: 30 - len("...") - len("SET 'somekey1' '")]}..." + - op="db.redis": description="SET 'somekey2' '{short_string}'"\ +""" ) - assert spans[1]["description"] == "SET 'somekey2' '%s'" % (short_string,) def test_breadcrumbs(sentry_init, capture_events): diff --git a/tests/integrations/redis/test_redis_cache_module.py b/tests/integrations/redis/test_redis_cache_module.py index 75455211ce..68f915c2e5 100644 --- a/tests/integrations/redis/test_redis_cache_module.py +++ b/tests/integrations/redis/test_redis_cache_module.py @@ -14,7 +14,7 @@ FAKEREDIS_VERSION = parse_version(fakeredis.__version__) -def test_no_cache_basic(sentry_init, capture_events): +def test_no_cache_basic(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[ RedisIntegration(), @@ -28,12 +28,16 @@ def test_no_cache_basic(sentry_init, capture_events): connection.get("mycachekey") (event,) = events - spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) - assert len(spans) == 1 - assert spans[0]["op"] == "db.redis" + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="db.redis": description="GET 'mycachekey'"\ +""" + ) -def test_cache_basic(sentry_init, capture_events): +def test_cache_basic(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[ RedisIntegration( @@ -53,31 +57,25 @@ def test_cache_basic(sentry_init, capture_events): connection.mget("mycachekey1", "mycachekey2") (event,) = events - spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) - assert len(spans) == 9 - - # no cache support for hget command - assert spans[0]["op"] == "db.redis" - assert spans[0]["tags"]["redis.command"] == "HGET" - - assert spans[1]["op"] == "cache.get" - assert spans[2]["op"] == "db.redis" - assert spans[2]["tags"]["redis.command"] == "GET" - - assert spans[3]["op"] == "cache.put" - assert spans[4]["op"] == "db.redis" - assert spans[4]["tags"]["redis.command"] == "SET" - - assert spans[5]["op"] == "cache.put" - assert spans[6]["op"] == "db.redis" - assert spans[6]["tags"]["redis.command"] == "SETEX" - - assert spans[7]["op"] == "cache.get" - assert spans[8]["op"] == "db.redis" - assert spans[8]["tags"]["redis.command"] == "MGET" + # no cache support for HGET command + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="db.redis": description="HGET 'mycachekey' [Filtered]" + - op="cache.get": description="mycachekey" + - op="db.redis": description="GET 'mycachekey'" + - op="cache.put": description="mycachekey1" + - op="db.redis": description="SET 'mycachekey1' [Filtered]" + - op="cache.put": description="mycachekey2" + - op="db.redis": description="SETEX 'mycachekey2' [Filtered] [Filtered]" + - op="cache.get": description="mycachekey1, mycachekey2" + - op="db.redis": description="MGET 'mycachekey1' [Filtered]"\ +""" + ) -def test_cache_keys(sentry_init, capture_events): +def test_cache_keys(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[ RedisIntegration( @@ -96,24 +94,18 @@ def test_cache_keys(sentry_init, capture_events): connection.get("bl") (event,) = events - spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) - - assert len(spans) == 6 - assert spans[0]["op"] == "db.redis" - assert spans[0]["description"] == "GET 'somethingelse'" - - assert spans[1]["op"] == "cache.get" - assert spans[1]["description"] == "blub" - assert spans[2]["op"] == "db.redis" - assert spans[2]["description"] == "GET 'blub'" - - assert spans[3]["op"] == "cache.get" - assert spans[3]["description"] == "blubkeything" - assert spans[4]["op"] == "db.redis" - assert spans[4]["description"] == "GET 'blubkeything'" - - assert spans[5]["op"] == "db.redis" - assert spans[5]["description"] == "GET 'bl'" + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="db.redis": description="GET 'somethingelse'" + - op="cache.get": description="blub" + - op="db.redis": description="GET 'blub'" + - op="cache.get": description="blubkeything" + - op="db.redis": description="GET 'blubkeything'" + - op="db.redis": description="GET 'bl'"\ +""" + ) def test_cache_data(sentry_init, capture_events): diff --git a/tests/integrations/redis/test_redis_cache_module_async.py b/tests/integrations/redis/test_redis_cache_module_async.py index d607f92fbd..a6ea06a973 100644 --- a/tests/integrations/redis/test_redis_cache_module_async.py +++ b/tests/integrations/redis/test_redis_cache_module_async.py @@ -21,7 +21,7 @@ @pytest.mark.asyncio -async def test_no_cache_basic(sentry_init, capture_events): +async def test_no_cache_basic(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[ RedisIntegration(), @@ -35,13 +35,17 @@ async def test_no_cache_basic(sentry_init, capture_events): await connection.get("myasynccachekey") (event,) = events - spans = event["spans"] - assert len(spans) == 1 - assert spans[0]["op"] == "db.redis" + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="db.redis": description="GET 'myasynccachekey'"\ +""" + ) @pytest.mark.asyncio -async def test_cache_basic(sentry_init, capture_events): +async def test_cache_basic(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[ RedisIntegration( @@ -57,15 +61,18 @@ async def test_cache_basic(sentry_init, capture_events): await connection.get("myasynccachekey") (event,) = events - spans = event["spans"] - assert len(spans) == 2 - - assert spans[0]["op"] == "cache.get" - assert spans[1]["op"] == "db.redis" + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="cache.get": description="myasynccachekey" + - op="db.redis": description="GET 'myasynccachekey'"\ +""" + ) @pytest.mark.asyncio -async def test_cache_keys(sentry_init, capture_events): +async def test_cache_keys(sentry_init, capture_events, render_span_tree): sentry_init( integrations=[ RedisIntegration( @@ -84,23 +91,18 @@ async def test_cache_keys(sentry_init, capture_events): await connection.get("abl") (event,) = events - spans = event["spans"] - assert len(spans) == 6 - assert spans[0]["op"] == "db.redis" - assert spans[0]["description"] == "GET 'asomethingelse'" - - assert spans[1]["op"] == "cache.get" - assert spans[1]["description"] == "ablub" - assert spans[2]["op"] == "db.redis" - assert spans[2]["description"] == "GET 'ablub'" - - assert spans[3]["op"] == "cache.get" - assert spans[3]["description"] == "ablubkeything" - assert spans[4]["op"] == "db.redis" - assert spans[4]["description"] == "GET 'ablubkeything'" - - assert spans[5]["op"] == "db.redis" - assert spans[5]["description"] == "GET 'abl'" + assert ( + render_span_tree(event) + == """\ +- op="": description=null + - op="db.redis": description="GET 'asomethingelse'" + - op="cache.get": description="ablub" + - op="db.redis": description="GET 'ablub'" + - op="cache.get": description="ablubkeything" + - op="db.redis": description="GET 'ablubkeything'" + - op="db.redis": description="GET 'abl'"\ +""" + ) @pytest.mark.asyncio @@ -122,7 +124,7 @@ async def test_cache_data(sentry_init, capture_events): await connection.get("myasynccachekey") (event,) = events - spans = event["spans"] + spans = sorted(event["spans"], key=lambda x: x["start_timestamp"]) assert len(spans) == 6