feat(redis): Support streaming spans #6083
3 issues
code-review: Found 3 issues (3 medium)
Medium
Spans not properly closed if Redis command raises exception - `sentry_sdk/integrations/redis/_async_common.py:145-156`
In _sentry_execute_command, when old_execute_command raises an exception, db_span.__exit__() (line 152) and cache_span.__exit__() (line 156) will never be called because they are executed after the await statement without exception handling. This causes span leaks and corrupted span trees where spans remain open indefinitely. The same pattern exists in the sync version.
Also found at:
sentry_sdk/integrations/redis/utils.py:119-125
Span streaming test for pipeline misses redis.commands assertions - `tests/integrations/redis/cluster_asyncio/test_redis_cluster_asyncio.py:160-179`
In test_async_redis_pipeline, when span_streaming=True, the test does not verify the redis.commands data (count and first_ten commands). The expected_first_ten parameter from the parametrize decorator is unused in the streaming branch. This creates a test coverage gap where the pipeline command details are not validated for streaming spans.
Also found at:
tests/integrations/redis/test_redis.py:72-90tests/integrations/redis/cluster/test_redis_cluster.py:152-153
Test checks wrong span index for cache.hit assertion - `tests/integrations/redis/test_redis_cache_module_async.py:293`
Line 293 checks assert "cache.hit" not in spans[1]["data"] but spans[1] is the db.redis span (as stated in line 286), not the cache.put span. The assertion should check spans[2]["data"] since that's the cache.put span being validated in the surrounding assertions (lines 288-304). This test will pass but doesn't actually verify the intended behavior - that cache.put spans don't have a cache.hit attribute.
Also found at:
tests/integrations/redis/test_redis_cache_module.py:330
Duration: 3m 50s · Tokens: 2.6M in / 32.0k out · Cost: $4.20 (+merge: $0.00, +fix_gate: $0.00)
Annotations
Check warning on line 156 in sentry_sdk/integrations/redis/_async_common.py
sentry-warden / warden: code-review
Spans not properly closed if Redis command raises exception
In `_sentry_execute_command`, when `old_execute_command` raises an exception, `db_span.__exit__()` (line 152) and `cache_span.__exit__()` (line 156) will never be called because they are executed after the await statement without exception handling. This causes span leaks and corrupted span trees where spans remain open indefinitely. The same pattern exists in the sync version.
Check warning on line 125 in sentry_sdk/integrations/redis/utils.py
sentry-warden / warden: code-review
[AR8-4BE] Spans not properly closed if Redis command raises exception (additional location)
In `_sentry_execute_command`, when `old_execute_command` raises an exception, `db_span.__exit__()` (line 152) and `cache_span.__exit__()` (line 156) will never be called because they are executed after the await statement without exception handling. This causes span leaks and corrupted span trees where spans remain open indefinitely. The same pattern exists in the sync version.
Check warning on line 179 in tests/integrations/redis/cluster_asyncio/test_redis_cluster_asyncio.py
sentry-warden / warden: code-review
Span streaming test for pipeline misses redis.commands assertions
In `test_async_redis_pipeline`, when `span_streaming=True`, the test does not verify the `redis.commands` data (count and first_ten commands). The `expected_first_ten` parameter from the parametrize decorator is unused in the streaming branch. This creates a test coverage gap where the pipeline command details are not validated for streaming spans.
Check warning on line 90 in tests/integrations/redis/test_redis.py
sentry-warden / warden: code-review
[XLD-33B] Span streaming test for pipeline misses redis.commands assertions (additional location)
In `test_async_redis_pipeline`, when `span_streaming=True`, the test does not verify the `redis.commands` data (count and first_ten commands). The `expected_first_ten` parameter from the parametrize decorator is unused in the streaming branch. This creates a test coverage gap where the pipeline command details are not validated for streaming spans.
Check warning on line 153 in tests/integrations/redis/cluster/test_redis_cluster.py
sentry-warden / warden: code-review
[XLD-33B] Span streaming test for pipeline misses redis.commands assertions (additional location)
In `test_async_redis_pipeline`, when `span_streaming=True`, the test does not verify the `redis.commands` data (count and first_ten commands). The `expected_first_ten` parameter from the parametrize decorator is unused in the streaming branch. This creates a test coverage gap where the pipeline command details are not validated for streaming spans.
Check warning on line 293 in tests/integrations/redis/test_redis_cache_module_async.py
sentry-warden / warden: code-review
Test checks wrong span index for cache.hit assertion
Line 293 checks `assert "cache.hit" not in spans[1]["data"]` but `spans[1]` is the `db.redis` span (as stated in line 286), not the `cache.put` span. The assertion should check `spans[2]["data"]` since that's the cache.put span being validated in the surrounding assertions (lines 288-304). This test will pass but doesn't actually verify the intended behavior - that cache.put spans don't have a cache.hit attribute.
Check warning on line 330 in tests/integrations/redis/test_redis_cache_module.py
sentry-warden / warden: code-review
[XLS-Y8L] Test checks wrong span index for cache.hit assertion (additional location)
Line 293 checks `assert "cache.hit" not in spans[1]["data"]` but `spans[1]` is the `db.redis` span (as stated in line 286), not the `cache.put` span. The assertion should check `spans[2]["data"]` since that's the cache.put span being validated in the surrounding assertions (lines 288-304). This test will pass but doesn't actually verify the intended behavior - that cache.put spans don't have a cache.hit attribute.