fix(boto3): Finish StreamingBody span correctly - #7540
pabloDeputter merged 5 commits into
Conversation
pabloDeputter
changed the title
StreamingBody span correctly
Codecov Results 📊✅ 132259 passed | ⏭️ 7214 skipped | Total: 139473 | Pass Rate: 94.83% | Execution Time: 427m 1s 📊 Comparison with Base Branch
➖ Removed Tests (60)View removed tests
All tests are passing successfully. ✅ Patch coverage is 86.76%. Project has 2554 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## master #PR +/-##
==========================================
+ Coverage 90.30% 90.30% —%
==========================================
Files 195 198 +3
Lines 26261 26322 +61
Branches 9798 9804 +6
==========================================
+ Hits 23714 23768 +54
- Misses 2547 2554 +7
- Partials 1485 1484 -1Generated by Codecov Action |
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| orig_read = body.read | ||
| orig_close = body.close | ||
| raw_stream = body._raw_stream # type: ignore[attr-defined] | ||
| orig_raw_close = raw_stream.close | ||
| finished = False | ||
|
|
There was a problem hiding this comment.
Streaming span leaks if setup fails before guarded try
Move _raw_stream access (and the other pre-try setup) into the existing try/except so finish() still runs if instrumentation setup raises after the child span is created.
Evidence
_instrument_streaming_body()createsstreaming_spanbefore setup completes.body._raw_stream/raw_stream.closerun outside the latertrythat callsfinish()on failure._sentry_after_call()invokes_instrument_streaming_body()undercapture_internal_exceptions(), so a setup exception is swallowed and the child span is never finished.
Identified by Warden · code-review, find-bugs · HDS-MQ3
There was a problem hiding this comment.
I'll check this out in the next PR, cause I don't want to have merge issues
There was a problem hiding this comment.
Fix attempt detected (commit ff199b0)
The commit clearly attempts to harden streaming span finalization, but _raw_stream access and other setup still occur before the guarded try, so an exception there can still leak the child span.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
There was a problem hiding this comment.
Fix attempt detected (commit 11ea5b8)
The change clearly attempts to harden streaming span finalization, but body._raw_stream and related setup still execute before the try/except that calls finish_span, so a setup exception can still leak the child span.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
There was a problem hiding this comment.
Fix attempt detected (commit 65dfa57)
The change clearly attempts to harden streaming-span finalization, but accesses to body._raw_stream and raw_stream.close still occur before the try/except, so setup failures there can still leak the child span.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
There was a problem hiding this comment.
Fix attempt detected (commit 4892911)
The change adds guarded cleanup for instrumentation assignments, but _raw_stream access and other setup remain before the try, so an exception there still leaks the child span.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
There was a problem hiding this comment.
Fix attempt detected (commit d7a3959)
The change clearly attempts to finalize the streaming span on setup failures, but accesses to body.read, body.close, body._raw_stream, and raw_stream.close remain before the guarded try, so an exception there still leaks the span.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
There was a problem hiding this comment.
Fix attempt detected (commit 09a038e)
The change clearly targets streaming-span finalization and adds cleanup for failures during wrapper assignment, but accessing body.read, body.close, and body._raw_stream.close still occurs before the guarded try and before finish_span is defined, so a setup exception can still leak the child span.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
There was a problem hiding this comment.
Fix attempt detected (commit 364f547)
The change clearly hardens streaming-span finalization, but _raw_stream access and other setup still occur before the guarded try, so a setup exception can leave the child span unfinished.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
| span: "Union[Span, StreamedSpan]", parsed: "Dict[str, Any]" | ||
| ) -> bool: | ||
| if isinstance(span, NoOpStreamedSpan): | ||
| return False |
There was a problem hiding this comment.
Why return a boolean here when we're not using the result?
There was a problem hiding this comment.
I probably messed up when splitting the large PR in smaller ones; in the next PR of the stack, we'll use this return value to check whether there was 1. a StreamingBody as response and 2. it was instrumented; since we only want to delay closing the boto span if both conditions are met.
I'll document this better in the next PR.
| ret = orig_read(*args, **kwargs) | ||
| if ret: | ||
| return ret | ||
|
|
||
| if isinstance(streaming_span, StreamedSpan): | ||
| streaming_span.end() | ||
| else: | ||
| streaming_span.finish() | ||
| with capture_internal_exceptions(): | ||
| amount = args[0] if args else kwargs.get("amt") |
There was a problem hiding this comment.
Couple of variable name suggestions to improve readability:
ret => read_return_value
amount => amount_of_bytes_requested
| orig_raw_close = raw_stream.close | ||
| finished = False | ||
|
|
||
| def finish(error: "Optional[BaseException]" = None) -> None: |
There was a problem hiding this comment.
Not specific to your changes and more of a general note on why I'm suggesting this (and other) renames:
We have more than a few vague names within the SDK - both within the "working code" and our tests. This makes it difficult to understand what exactly is being invoked when calling a method, what data is represented by a variable, or what behaviour we're looking to test.
I'd like us to try and move a bit more in the direction of being more specific so that someone reading this in the future doesn't have to jump to function definitions or ask a clanker what pieces of data mean or what's being tested (at least as often as we may have to now).
| def finish(error: "Optional[BaseException]" = None) -> None: | |
| def finish_span(error: "Optional[BaseException]" = None) -> None: |
There was a problem hiding this comment.
good to know, I'll try to keep this in mind in the future.
| sentry_init( | ||
| traces_sample_rate=1.0, | ||
| integrations=[Boto3Integration()], | ||
| trace_lifecycle="stream" if span_streaming else "static", |
There was a problem hiding this comment.
When these changes land, we're going to have to make sure that we remove the branching on span_streaming in the major/3.0 branch 😅
There was a problem hiding this comment.
Yupp, there are quite a lot of these cases 😬
dedab66 to
5383b90
Compare
66d4665 to
54c499e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 54c499e. Configure here.
ff199b0 to
05d5050
Compare
11ea5b8 to
65dfa57
Compare
65dfa57 to
4892911
Compare
4892911 to
d7a3959
Compare
d7a3959 to
09a038e
Compare
09a038e to
364f547
Compare
2 participants
Description
Finish boto streaming spans when a
StreamingBodyis consumed, closed or fails. Previously, streaming spans where only finished when a read returned no data orStreamingBody.close()was called.