fix(boto3): Finish StreamingBody span correctly - #7540
pabloDeputter wants to merge 5 commits into
Conversation
pabloDeputter
changed the title
StreamingBody span correctly
Codecov Results 📊✅ 129673 passed | ⏭️ 7165 skipped | Total: 136838 | Pass Rate: 94.76% | Execution Time: 430m 4s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 86.76%. Project has 2556 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## master #PR +/-##
==========================================
- Coverage 90.20% 90.19% -0.01%
==========================================
Files 194 197 +3
Lines 25888 26062 +174
Branches 9584 9666 +82
==========================================
+ Hits 23352 23506 +154
- Misses 2536 2556 +20
- Partials 1455 1449 -6Generated 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
| 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.
| streaming_span = sentry_sdk.traces.start_span( | ||
| name=span.name, | ||
| parent_span=span, | ||
| active=False, |
There was a problem hiding this comment.
Because this is an unusual decision (creating a span that we're marking as inactive) and then finishing it at a later point, I'd document why this was done for future readers.
It's not clear why this is happening if I were to read the code outside of this pull request.
There was a problem hiding this comment.
Yeah, I agree. I added a comment with some examples where it might fail when it's not initialized with active=False.
| 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.
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.