ref(httpx,httpx2): Move crumbs to integrations - #7149
Conversation
Sorry, something went wrong.
Move subprocess breadcrumb creation from the centralized `maybe_create_breadcrumbs_from_span` hook into the stdlib integration's `Popen.__init__` wrapper. This makes breadcrumbs work for both legacy spans and streamed spans, and removes the dependency on span internals.
…-redis-breadcrumbs-to-integration
| "reason": rv.reason_phrase, | ||
| } | ||
|
|
||
| if parsed_url and (not is_span_streaming_enabled or should_send_default_pii()): |
There was a problem hiding this comment.
The is_not_span_streaming_enabled part is there for continuity in transaction mode, where we don't care about should_send_default_pii before setting breadcrumb data.
Sorry, something went wrong.
Codecov Results 📊✅ 112111 passed | ⏭️ 6749 skipped | Total: 118860 | Pass Rate: 94.32% | Execution Time: 401m 48s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2487 uncovered lines. @@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 90.17% 90.19% +0.02%
==========================================
Files 193 193 —
Lines 25337 25353 +16
Branches 9286 9294 +8
==========================================
+ Hits 22846 22866 +20
- Misses 2491 2487 -4
- Partials 1429 1428 -1Generated by Codecov Action |
Sorry, something went wrong.
Instead of parametrizing on sync/async httpx(2) client, split each test case into a sync and async variant, with the async variant as a proper `async def` function with `@pytest.mark.parametrize`. I did this because the tests routinely fail for me locally, and switching to using `pytest-asyncio` fixes that. The other reason is that for #7149, the breadcrumb tests don't simulate async very well, which leads to some scope problems and ultimately breadcrumbs not appearing on events in tests. I know this PR is not ideal since with the existing test duplication on span streaming/transaction tracing, we already have a LOT of test cases, and now many of them get an additional variant. But it does make them more resilient to random local (and I believe also CI) failures and we'll get rid of half of them on the new major branch.
| ) | ||
|
|
||
| add_http_breadcrumb(rv.status_code, breadcrumb_data) |
There was a problem hiding this comment.
HTTPX breadcrumbs are silently skipped when the request raises
When real_send raises an exception, the new add_http_breadcrumb call after the span block is unreachable, so no HTTP breadcrumb is recorded for failed requests. Previously Span.__exit__ created this crumb during cleanup regardless of exceptions.
Evidence- Span.__exit__ in tracing.py unconditionally calls self.finish(), which in turn calls maybe_create_breadcrumbs_from_span, so the old span-based mechanism always emitted a crumb—even when the request raised.
- The new code places breadcrumb_data = { ... } and add_http_breadcrumb(...) after the if/else span blocks, so any exception propagated by real_send bypasses it entirely.
- This means network errors, timeouts, or any other HTTP library exception inside the span block silently drop the breadcrumb, losing telemetry context that was previously available.
Identified by Warden · code-review · B7U-CFR
Sorry, something went wrong.
Description
Move breadcrumbs from the centralized, span-powered maybe_create_breadcrumbs_from_span directly to the HTTPX and HTTPX2 integrations. (Put the two together in one PR since they're the same changeset.)
Additional changes and context:
Issues
Reminders