fix(boto3): Trace the complete botocore client-call lifecycle - #7538
pabloDeputter wants to merge 7 commits into
Conversation
Codecov Results 📊✅ 127217 passed | ❌ 59 failed | ⏭️ 7171 skipped | Total: 134447 | Pass Rate: 94.62% | Execution Time: 439m 56s 📊 Comparison with Base Branch
➕ New Tests (59)View new tests
➖ Removed Tests (1)View removed tests
❌ Failed Tests
|
| File | Patch % | Lines |
|---|---|---|
| sentry_sdk/integrations/boto3/_instrumentation.py | 94.51% | |
| sentry_sdk/integrations/boto3/_client.py | 96.23% | |
| sentry_sdk/integrations/boto3/_context.py | 96.30% |
Coverage diff
@@ Coverage Diff @@
## master #PR +/-##
==========================================
+ Coverage 90.22% 90.27% +0.05%
==========================================
Files 194 198 +4
Lines 25997 26159 +162
Branches 9662 9698 +36
==========================================
+ Hits 23454 23614 +160
- Misses 2543 2545 +2
- Partials 1451 1455 +4Generated by Codecov Action
53a8717 to
4efce4b
Compare
4efce4b to
05501d2
Compare
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
ericapisani
left a comment
ericapisani
left a comment
There was a problem hiding this comment.
Minor things but overall looking good. Will take a look at tests on my 2nd pass
| from sentry_sdk.integrations.boto3 import Boto3Integration | ||
| @contextmanager | ||
| def _activate_client_span(span: "StreamedSpan") -> "Iterator[StreamedSpan]": | ||
| """Temporarily activate an inactive boto span without ending it.""" |
There was a problem hiding this comment.
Worth adding context on why we're doing this.
| # use unknown if `service_id_hyphenized` so span name can still be created. | ||
| # e.g. "aws.unkown.GetObject" | ||
| service_name = ctx.service_id_hyphenized or "unknown" | ||
| span_name = "aws.%s.%s" % (service_name, ctx.operation_name) |
There was a problem hiding this comment.
f-strings are generally the preferred way to construct strings like this in modern python these days
| span_name = "aws.%s.%s" % (service_name, ctx.operation_name) | |
| span_name = f"aws.{service_name}.{ctx.operation_name}" |
There was a problem hiding this comment.
yupp, I took over the old code, but I usually also use f-strings.
| SPANDATA.SENTRY_ORIGIN: span_origin, | ||
| } | ||
| if ctx.service_id: | ||
| attributes[SPANDATA.RPC_METHOD] = "%s/%s" % ( |
| "sentry.op": OP.HTTP_CLIENT_STREAM, | ||
| "sentry.origin": Boto3Integration.origin, | ||
| SPANDATA.SENTRY_OP: OP.HTTP_CLIENT_STREAM, | ||
| SPANDATA.SENTRY_ORIGIN: Boto3Integration.origin, |
| if isinstance(span, StreamedSpan) and ( | ||
| span.get_attributes().get(SPANDATA.SENTRY_ORIGIN) != Boto3Integration.origin | ||
| ): |
There was a problem hiding this comment.
Because of the length of this conditional, I think it'd be a bit cleaner to pull this into a variable
| if isinstance(span, StreamedSpan) and ( | |
| span.get_attributes().get(SPANDATA.SENTRY_ORIGIN) != Boto3Integration.origin | |
| ): | |
| is_span_origin_from_boto = span.get_attributes().get(SPANDATA.SENTRY_ORIGIN) == Boto3Integration.origin | |
| if isinstance(span, StreamedSpan) and is_span_origin_from_boto: |
There was a problem hiding this comment.
I agree, but then you get the issue that the legacy span doesn't support get_attributes(); I fixed it like this:
# an ignored streamed span is not activated; avoid enriching its parent.
if isinstance(span, StreamedSpan):
if not (span.get_attributes().get(SPANDATA.SENTRY_ORIGIN) == ORIGIN):
return| headers["foo"] = "old" | ||
| headers["foo"] = "new" | ||
| produces two fields: {"foo": "old", "foo": "new"}. So delete existing | ||
| fields before assigning replacement. |
There was a problem hiding this comment.
Great comment, thanks for adding this 👍🏻 🙏🏻
There was a problem hiding this comment.
yeah it's really weird behavior 😆
| }, | ||
| # boto3 integration owns span's lifecycle; keep child inactive so it | ||
| # can't restore boto3 span later on. | ||
| active=not is_inactive_boto3_span, |
There was a problem hiding this comment.
Apologies for this slight nitpick, but can we add a space after the = and before the not?
The "not" almost blends in with the =, especially because the syntax highlighting of Github and IDEs use the same colour for the two. 😭
There was a problem hiding this comment.
I agree, but the linter doesn't really like that 🤣 I fixed it by adding fmt: off and fmt: on for that specific block; I haven't seen it being used anywhere else in the codebase, so not sure whether this is correct.
05501d2 to
421da22
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
c3e8f5b to
66d4665
Compare
fe64ae9 to
add0913
Compare
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 177d277. Configure here.
f649477 to
4423557
Compare
4423557 to
c622458
Compare
c622458 to
3e4fcfd
Compare
This branch has not been deployed
2 participants
Description
Move boto3 span creation from individual HTTP request attempts to full botocore client-call lifecycle.
Previously, the client span was created from the botocore
request-createdevent; since botocore creates a newAwsRequestevent for every retry, these spans represent individual retries rather than the full event. Following OTel (https://opentelemetry.io/docs/specs/semconv/rpc/rpc-spans/#rpc-client-span) the span should cover the entire call lifecycle including all retries. Wrapping_make_api_call()covers all retries performed by botocore, including serialization, endpoint resolution, the final response or failures (https://github.com/boto/botocore/blob/develop/botocore/client.py:999).BaseClient._make_api_call()so one span represents a single boto operation; this span is also kept active across every retry attempt.request-createdis kept for breadcrumbs, HTTP request attributes, and trace propagation.StreamingBodyare kept open until body is consumed or closed.Issues
Resolves #7474