feat(redis): Support streaming spans #6083
3 issues
code-review: Found 3 issues (1 high, 1 medium, 1 low)
High
Spans leak without cleanup when Redis command raises exception - `sentry_sdk/integrations/redis/_sync_common.py:151-160`
If old_execute_command() on line 151 raises an exception, db_span.__exit__() and cache_span.__exit__() are never called. This leaves spans in an unclosed state, causing resource leaks in span tracking and missing span data. The spans should be wrapped in try/finally to ensure proper cleanup regardless of exceptions.
Also found at:
sentry_sdk/integrations/redis/_async_common.py:147-148
Medium
Test assertion checks wrong span index - always passes vacuously - `tests/integrations/redis/test_redis_cache_module_async.py:293`
Line 293 asserts "cache.hit" not in spans[1]["data"] but should be checking spans[2]. The surrounding code (lines 288-294) is testing the cache.put span at index 2, and spans[1] is the db.redis span (per line 286 comment). Since db.redis spans never have cache.hit, this assertion always passes regardless of whether the cache.put span incorrectly contains cache.hit. The test fails to validate the actual requirement.
Also found at:
tests/integrations/redis/test_redis_cache_module.py:330
Low
Streaming span test does not verify redis.commands data - `tests/integrations/redis/cluster/test_redis_cluster.py:182-189`
In test_rediscluster_pipeline, the expected_first_ten parameter (which validates redis.commands.first_ten with potentially filtered PII) is only used in the non-streaming path (line 207). The streaming path (lines 182-189) doesn't verify that command data is properly included or filtered based on send_default_pii. This may result in insufficient test coverage for PII filtering in streaming mode.
Duration: 7m 47s · Tokens: 2.7M in / 35.6k out · Cost: $4.13 (+extraction: $0.01, +merge: $0.00, +fix_gate: $0.01)
Annotations
Check failure on line 160 in sentry_sdk/integrations/redis/_sync_common.py
sentry-warden / warden: code-review
Spans leak without cleanup when Redis command raises exception
If `old_execute_command()` on line 151 raises an exception, `db_span.__exit__()` and `cache_span.__exit__()` are never called. This leaves spans in an unclosed state, causing resource leaks in span tracking and missing span data. The spans should be wrapped in try/finally to ensure proper cleanup regardless of exceptions.
Check failure on line 148 in sentry_sdk/integrations/redis/_async_common.py
sentry-warden / warden: code-review
[UF9-SNY] Spans leak without cleanup when Redis command raises exception (additional location)
If `old_execute_command()` on line 151 raises an exception, `db_span.__exit__()` and `cache_span.__exit__()` are never called. This leaves spans in an unclosed state, causing resource leaks in span tracking and missing span data. The spans should be wrapped in try/finally to ensure proper cleanup regardless of exceptions.
Check warning on line 293 in tests/integrations/redis/test_redis_cache_module_async.py
sentry-warden / warden: code-review
Test assertion checks wrong span index - always passes vacuously
Line 293 asserts `"cache.hit" not in spans[1]["data"]` but should be checking `spans[2]`. The surrounding code (lines 288-294) is testing the cache.put span at index 2, and spans[1] is the db.redis span (per line 286 comment). Since db.redis spans never have cache.hit, this assertion always passes regardless of whether the cache.put span incorrectly contains cache.hit. The test fails to validate the actual requirement.
Check warning on line 330 in tests/integrations/redis/test_redis_cache_module.py
sentry-warden / warden: code-review
[H4B-NSH] Test assertion checks wrong span index - always passes vacuously (additional location)
Line 293 asserts `"cache.hit" not in spans[1]["data"]` but should be checking `spans[2]`. The surrounding code (lines 288-294) is testing the cache.put span at index 2, and spans[1] is the db.redis span (per line 286 comment). Since db.redis spans never have cache.hit, this assertion always passes regardless of whether the cache.put span incorrectly contains cache.hit. The test fails to validate the actual requirement.