Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
Codecov Results 📊✅ 142 passed | Total: 142 | Pass Rate: 100% | Execution Time: 24.90s 📊 Comparison with Base Branch
All tests are passing successfully. ❌ Patch coverage is 7.37%. Project has 14189 uncovered lines. Files with missing lines (6)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 27.67% 33.67% +6%
==========================================
Files 190 190 —
Lines 21351 21391 +40
Branches 7066 7098 +32
==========================================
+ Hits 5908 7202 +1294
- Misses 15443 14189 -1254
- Partials 564 700 +136Generated by Codecov Action |
| db_span.__exit__(None, None, None) | ||
| if cache_span: | ||
| with capture_internal_exceptions(): | ||
| _set_cache_data(cache_span, self, cache_properties, value) |
There was a problem hiding this comment.
NameError raised when Redis command fails with exception
In sentry_patched_execute_command, when old_execute_command raises an exception, the value variable is never assigned. The finally block then attempts to use value in _set_cache_data(cache_span, self, cache_properties, value), causing a NameError. While capture_internal_exceptions() catches this, it results in unnecessary internal exception noise and obscures the real error from monitoring.
Verification
Read the full file at _sync_common.py, traced through the try/finally block logic. Verified that value is only assigned on line 152 inside the try block. If an exception is raised, execution jumps to finally where line 158 references value which would be undefined.
Identified by Warden code-review · XZX-J22
There was a problem hiding this comment.
Fix attempt detected (commit 032c7e2)
The commit reorganized code into a try-finally block and wrapped the problematic _set_cache_data call with capture_internal_exceptions(), but the core issue persists: if old_execute_command raises an exception, value remains undefined when accessed on line 158, causing the same NameError (just silently caught rather than fixed)
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
| ) -> None: | ||
| span.set_tag("redis.is_cluster", is_cluster) | ||
| span.set_tag("redis.transaction", is_transaction) | ||
| if isinstance(span, Span): |
There was a problem hiding this comment.
NameError: Span is only imported under TYPE_CHECKING but used at runtime
The Span class is imported only under TYPE_CHECKING (line 17), which means it's not available at runtime. However, isinstance(span, Span) is called at line 115, which will raise NameError: name 'Span' is not defined whenever _set_pipeline_data is called with any span type.
Verification
Read lines 1-17 of utils.py confirming Span is imported inside if TYPE_CHECKING: block. Verified no other imports of Span exist outside TYPE_CHECKING. The isinstance check at line 115 will fail at runtime.
Suggested fix: Import Span at runtime or invert the logic to check for StreamedSpan first (which is already imported at runtime on line 10)
| if isinstance(span, Span): | |
| if not isinstance(span, StreamedSpan): |
Identified by Warden code-review · VKW-3W2
Description
Support span streaming in the Redis integration(s).
Issues
Reminders
tox -e linters.feat:,fix:,ref:,meta:)