Skip to content

ref(pyreqwest): Move crumbs to integration - #7148

Open
sentrivana wants to merge 6 commits into
ivana/move-http-crumbs-1from
ivana/move-http-crumbs-2
Open

ref(pyreqwest): Move crumbs to integration#7148
sentrivana wants to merge 6 commits into
ivana/move-http-crumbs-1from
ivana/move-http-crumbs-2

Conversation

@sentrivana

@sentrivana sentrivana commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Description

Create HTTP client breadcrumbs directly in the pyreqwest integration instead of using maybe_create_breadcrumbs_from_span.

Additionally:

  • refactor to use an early return in the wrapper to avoid nesting
  • add breadcrumb tests to the pyreqwest test suite

Issues

Reminders

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Codecov Results 📊

104422 passed | ⏭️ 6677 skipped | Total: 111099 | Pass Rate: 93.99% | Execution Time: 364m 48s

📊 Comparison with Base Branch

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

All tests are passing successfully.

✅ Patch coverage is 96.55%. Project has 2487 uncovered lines.
❌ Project coverage is 90.13%. Comparing base (base) to head (head).

Files with missing lines (1)
File Patch % Lines
sentry_sdk/integrations/pyreqwest.py 96.43% ⚠️ 1 Missing and 3 partials
Coverage diff
@@            Coverage Diff             @@
##          main       #PR       +/-##
==========================================
- Coverage    90.15%    90.13%    -0.02%
==========================================
  Files          193       193         —
  Lines        25147     25191       +44
  Branches      9136      9152       +16
==========================================
+ Hits         22669     22704       +35
- Misses        2478      2487        +9
- Partials      1431      1431         —

Generated by Codecov Action

@sentrivana
sentrivana marked this pull request as ready for review August 10, 2026 09:16
@sentrivana
sentrivana requested a review from a team as a code owner August 10, 2026 09:16
Comment thread sentry_sdk/integrations/pyreqwest.py Outdated
Comment thread sentry_sdk/integrations/pyreqwest.py Outdated
Comment thread sentry_sdk/integrations/pyreqwest.py Outdated
@sentrivana
sentrivana marked this pull request as draft August 10, 2026 09:53
@sentrivana
sentrivana marked this pull request as ready for review August 10, 2026 10:21
Comment thread sentry_sdk/integrations/pyreqwest.py

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e31a3aa. Configure here.

):
sentry_init(
integrations=[PyreqwestIntegration()],
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Streaming crumb test not streaming

Low Severity

test_crumb_capture_client_error_span_streaming is named and structured for span streaming, but sentry_init omits trace_lifecycle="stream". With the default lifecycle, traces.start_span yields a no-op span, so this case never exercises breadcrumb levels under streaming the way the matching async streaming test does.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e31a3aa. Configure here.

Comment on lines +184 to +191
if response is not None:
breadcrumb_data = {
SPANDATA.HTTP_METHOD: method,
SPANDATA.HTTP_STATUS_CODE: response.status,
}

if parsed_url and should_send_default_pii():
breadcrumb_data.update(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The pyreqwest integration only adds the url to breadcrumbs when should_send_default_pii() is true, unlike the aiohttp integration which always includes it.
Severity: MEDIUM

Suggested Fix

Move the breadcrumb_data.update({"url": parsed_url.url}) call outside of the if should_send_default_pii(): block. The query and fragment should remain inside the conditional block, but the base URL should always be added to the breadcrumb data, similar to how the aiohttp integration handles it.

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/pyreqwest.py#L184-L191

Potential issue: The `pyreqwest` integration's breadcrumb creation is inconsistent with
the reference `aiohttp` integration. The base URL is only added to the `breadcrumb_data`
if `should_send_default_pii()` returns true. However, the base URL, as returned by
`parse_url`, does not contain sensitive query parameters or fragments and should always
be included for context. This results in breadcrumbs missing the `url` field when PII is
disabled, reducing their utility compared to other HTTP integrations like `aiohttp`
which always include the base URL.

Also affects:

  • sentry_sdk/integrations/pyreqwest.py:229~236

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants