[do not merge] feat: Span streaming & new span API #5551
5 issues
code-review: Found 5 issues (2 high, 2 medium, 1 low)
High
UnboundLocalError when Redis command fails with cache span active - `sentry_sdk/integrations/redis/_sync_common.py:158`
When old_execute_command raises an exception and a cache_span exists, the finally block attempts to access value in _set_cache_data(cache_span, self, cache_properties, value) before it's assigned. This causes an UnboundLocalError that replaces the original Redis exception, making it impossible to debug the actual failure cause.
Also found at:
sentry_sdk/integrations/redis/_async_common.py:145-147
StreamedSpan.set_status() method does not exist - will raise AttributeError at runtime - `sentry_sdk/integrations/sqlalchemy.py:102`
The code calls span.set_status(SpanStatus.ERROR) when the span is a StreamedSpan instance (line 102). However, the StreamedSpan class in sentry_sdk/traces.py does not have a set_status() method - it only has a status property setter. The legacy Span class has set_status() (line 600 in tracing.py), but StreamedSpan does not, so this will raise an AttributeError at runtime when handling SQL errors in streaming mode.
Medium
Missing test coverage for Celery integration with span streaming mode
The changes introduce significant branching logic to handle both legacy and span streaming modes in apply_async and _wrap_tracer functions. However, there are no tests in tests/integrations/celery/ that exercise the new span streaming code paths (checked by searching for trace_lifecycle and span_streaming patterns). Without integration tests covering the span streaming mode, regressions in Celery task tracing could go undetected.
HTTP status code attribute not set for StreamedSpan - `sentry_sdk/integrations/stdlib.py:175-177`
When handling HTTP responses with StreamedSpan, the code sets span.status and the reason attribute but does not set the HTTP status code attribute (SPANDATA.HTTP_STATUS_CODE). The legacy path correctly calls set_http_status() which sets http.response.status_code. This inconsistency means that HTTP response status codes will be missing from spans in streaming mode, reducing observability of HTTP client calls.
Low
Redundant Pattern import creates confusion and potential shadowing - `sentry_sdk/tracing_utils.py:13`
The code imports Pattern unconditionally from typing on line 13, then immediately tries to import it again from re in the try/except block on lines 15-19. This results in the re.Pattern import (when successful in Python 3.7+) shadowing the typing.Pattern import. While not a runtime error, this redundancy is confusing and the unconditional typing import on line 13 serves no purpose.
Duration: 12m 46s · Tokens: 9.2M in / 109.1k out · Cost: $13.03 (+extraction: $0.01, +merge: $0.00, +fix_gate: $0.01)
Annotations
Check failure on line 158 in sentry_sdk/integrations/redis/_sync_common.py
sentry-warden / warden: code-review
UnboundLocalError when Redis command fails with cache span active
When `old_execute_command` raises an exception and a `cache_span` exists, the `finally` block attempts to access `value` in `_set_cache_data(cache_span, self, cache_properties, value)` before it's assigned. This causes an `UnboundLocalError` that replaces the original Redis exception, making it impossible to debug the actual failure cause.
Check failure on line 147 in sentry_sdk/integrations/redis/_async_common.py
sentry-warden / warden: code-review
[FNQ-6ZX] UnboundLocalError when Redis command fails with cache span active (additional location)
When `old_execute_command` raises an exception and a `cache_span` exists, the `finally` block attempts to access `value` in `_set_cache_data(cache_span, self, cache_properties, value)` before it's assigned. This causes an `UnboundLocalError` that replaces the original Redis exception, making it impossible to debug the actual failure cause.
Check failure on line 102 in sentry_sdk/integrations/sqlalchemy.py
sentry-warden / warden: code-review
StreamedSpan.set_status() method does not exist - will raise AttributeError at runtime
The code calls `span.set_status(SpanStatus.ERROR)` when the span is a `StreamedSpan` instance (line 102). However, the `StreamedSpan` class in `sentry_sdk/traces.py` does not have a `set_status()` method - it only has a `status` property setter. The legacy `Span` class has `set_status()` (line 600 in tracing.py), but `StreamedSpan` does not, so this will raise an `AttributeError` at runtime when handling SQL errors in streaming mode.
Check warning on line 177 in sentry_sdk/integrations/stdlib.py
sentry-warden / warden: code-review
HTTP status code attribute not set for StreamedSpan
When handling HTTP responses with `StreamedSpan`, the code sets `span.status` and the `reason` attribute but does not set the HTTP status code attribute (`SPANDATA.HTTP_STATUS_CODE`). The legacy path correctly calls `set_http_status()` which sets `http.response.status_code`. This inconsistency means that HTTP response status codes will be missing from spans in streaming mode, reducing observability of HTTP client calls.