ref: Move HTTP client breadcrumbs to integrations (1) - #7130
Conversation
Codecov Results 📊✅ 99250 passed | ⏭️ 6479 skipped | Total: 105729 | Pass Rate: 93.87% | Execution Time: 347m 49s 📊 Comparison with Base Branch
➖ Removed Tests (1)View removed tests
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2508 uncovered lines. Files with missing lines (6)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 89.99% 89.99% —%
==========================================
Files 193 193 —
Lines 24974 25047 +73
Branches 9008 9036 +28
==========================================
+ Hits 22473 22539 +66
- Misses 2501 2508 +7
- Partials 1437 1443 +6Generated by Codecov Action |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| SPANDATA.HTTP_FRAGMENT: parsed_url.fragment, | ||
| } | ||
| ) | ||
| add_http_breadcrumb(rv.status_code, breadcrumb_data) | ||
|
|
There was a problem hiding this comment.
Bug: In non-streaming mode, two identical breadcrumbs are created for each HTTP request: one by add_http_breadcrumb and another by maybe_create_breadcrumbs_from_span when the span finishes.
Severity: MEDIUM
Suggested Fix
The maybe_create_breadcrumbs_from_span function should be modified to avoid creating a breadcrumb if one has already been created for the same HTTP client operation. Alternatively, the explicit call to add_http_breadcrumb in the httpx integration could be made conditional, checking if a breadcrumb for the span already exists.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: sentry_sdk/integrations/httpx.py#L142-L146
Potential issue: In non-streaming mode, the httpx integration creates two identical
breadcrumbs for each HTTP request. The request is wrapped in a `sentry_sdk.start_span`
with `op=OP.HTTP_CLIENT`. When this span finishes, `maybe_create_breadcrumbs_from_span`
is called, which unconditionally creates a breadcrumb. Immediately after, the new code
explicitly calls `add_http_breadcrumb`, resulting in a second, identical breadcrumb for
the same request. This leads to redundant data in Sentry events.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
The function will be removed in part 2 (#7131). I'll merge part 2 into this PR before merging into master.
![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 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f6a9c0c. Configure here.
| SPANDATA.HTTP_FRAGMENT: parsed_url.fragment, | ||
| } | ||
| ) | ||
| add_http_breadcrumb(rv.status_code, breadcrumb_data) |
There was a problem hiding this comment.
Breadcrumb skipped when no span is created
Medium Severity
In span streaming mode with no active parent span, httpx, httpx2, aiohttp and boto3 return early before reaching the new breadcrumb code, so no HTTP breadcrumb is recorded at all. pyreqwest places the call after the span context manager and does record one, making behaviour inconsistent and leaving breadcrumbs still coupled to span creation, which is what this change aims to remove.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit f6a9c0c. Configure here.
|
Will split this up further. I also noticed additional problems with it |
1 participant
Description
Move breadcrumbs created by HTTP client integrations to the integrations themselves, instead of relying on span data.
This PR has everything but stdlib in it.
Issues
Part 1/2 of #7067
Reminders
uv run ruff.feat:,fix:,ref:,meta:)