Skip to content

ref(httpx,httpx2): Move crumbs to integrations - #7149

Merged
sentrivana merged 41 commits into
masterfrom
ivana/move-http-crumbs-3
Aug 12, 2026
Merged

ref(httpx,httpx2): Move crumbs to integrations#7149
sentrivana merged 41 commits into
masterfrom
ivana/move-http-crumbs-3

Conversation

@sentrivana

@sentrivana sentrivana commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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:

  • Running the async breadcrumb tests with pytest-asyncio, as that simulates how the scopes behave live better than setting up an ad-hoc event loop.
  • Split the crumb tests into span streaming/not span streaming (because of the difference in send_default_pii behavior).

Issues

Reminders

"reason": rv.reason_phrase,
}

if parsed_url and (not is_span_streaming_enabled or should_send_default_pii()):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Codecov Results 📊

112111 passed | ⏭️ 6749 skipped | Total: 118860 | Pass Rate: 94.32% | Execution Time: 401m 48s

📊 Comparison with Base Branch

Metric Change
Total Tests 📈 +648
Passed Tests 📈 +648
Failed Tests
Skipped Tests

All tests are passing successfully.

✅ Patch coverage is 100.00%. Project has 2487 uncovered lines.
✅ Project coverage is 90.19%. Comparing base (base) to head (head).

Coverage diff
@@            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        -1

Generated by Codecov Action

sentrivana added a commit that referenced this pull request Aug 10, 2026
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.
@sentrivana
sentrivana marked this pull request as ready for review August 10, 2026 15:06
@sentrivana
sentrivana requested a review from a team as a code owner August 10, 2026 15:07
Base automatically changed from ivana/move-http-crumbs-2 to master August 12, 2026 12:07
@sentrivana
sentrivana enabled auto-merge (squash) August 12, 2026 12:09
@sentrivana
sentrivana merged commit 8177739 into master Aug 12, 2026
139 checks passed
@sentrivana
sentrivana deleted the ivana/move-http-crumbs-3 branch August 12, 2026 12:21
Comment on lines +253 to +255
)

add_http_breadcrumb(rv.status_code, breadcrumb_data)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants