Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 66 additions & 10 deletions sentry_sdk/integrations/pyreqwest.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from sentry_sdk.traces import StreamedSpan
from sentry_sdk.tracing import BAGGAGE_HEADER_NAME
from sentry_sdk.tracing_utils import (
add_http_breadcrumb,
add_http_request_source,
add_sentry_baggage_to_headers,
has_span_streaming_enabled,
Expand Down Expand Up @@ -67,15 +68,19 @@ def _patch_builder_method(cls: type, method_name: str, middleware: "Any") -> Non
original_method = getattr(cls, method_name)

def sentry_patched_method(self: "Any", *args: "Any", **kwargs: "Any") -> "Any":
if not getattr(self, "_sentry_instrumented", False):
integration = sentry_sdk.get_client().get_integration(PyreqwestIntegration)
if integration is not None:
self.with_middleware(middleware)
try:
self._sentry_instrumented = True
except (TypeError, AttributeError):
# In case the instance itself is immutable or doesn't allow extra attributes
pass
integration = sentry_sdk.get_client().get_integration(PyreqwestIntegration)

if getattr(self, "_sentry_instrumented", False) or integration is None:
return original_method(self, *args, **kwargs)

self.with_middleware(middleware)
Comment thread
sentrivana marked this conversation as resolved.

try:
self._sentry_instrumented = True
except (TypeError, AttributeError):
# In case the instance itself is immutable or doesn't allow extra attributes
pass

return original_method(self, *args, **kwargs)

setattr(cls, method_name, sentry_patched_method)
Expand Down Expand Up @@ -151,11 +156,20 @@ def _sentry_pyreqwest_span(request: "Request") -> "Generator[Any, None, None]":


async def sentry_async_middleware(
request: "Request", next_handler: "Next"
request: "Request",
next_handler: "Next",
) -> "Response":
if sentry_sdk.get_client().get_integration(PyreqwestIntegration) is None:
return await next_handler.run(request)

method = request.method
parsed_url = None
with capture_internal_exceptions():
# This needs to be done early because the URL is no longer accessible
# after the request has been sent
parsed_url = parse_url(str(request.url), sanitize=False)

response = None
with _sentry_pyreqwest_span(request) as span:
response = await next_handler.run(request)
if isinstance(span, StreamedSpan):
Expand All @@ -167,6 +181,23 @@ async def sentry_async_middleware(
elif span is not None:
span.set_http_status(response.status)

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(
Comment on lines +184 to +191

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

{
"url": parsed_url.url,
SPANDATA.HTTP_QUERY: parsed_url.query,
SPANDATA.HTTP_FRAGMENT: parsed_url.fragment,
}
)

add_http_breadcrumb(response.status, breadcrumb_data)

return response


Expand All @@ -176,6 +207,14 @@ def sentry_sync_middleware(
if sentry_sdk.get_client().get_integration(PyreqwestIntegration) is None:
return next_handler.run(request)

method = request.method
parsed_url = None
with capture_internal_exceptions():
# This needs to be done early because the URL is no longer accessible
# after the request has been sent
parsed_url = parse_url(str(request.url), sanitize=False)

response = None
with _sentry_pyreqwest_span(request) as span:
response = next_handler.run(request)
if isinstance(span, StreamedSpan):
Expand All @@ -187,4 +226,21 @@ def sentry_sync_middleware(
elif span is not None:
span.set_http_status(response.status)

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(
{
"url": parsed_url.url,
SPANDATA.HTTP_QUERY: parsed_url.query,
SPANDATA.HTTP_FRAGMENT: parsed_url.fragment,
}
)

add_http_breadcrumb(response.status, breadcrumb_data)

return response
5 changes: 4 additions & 1 deletion sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,10 @@ def add_http_breadcrumb(status_code: "Optional[int]", data: "dict[str, Any]") ->
def maybe_create_breadcrumbs_from_span(
scope: "sentry_sdk.Scope", span: "sentry_sdk.tracing.Span"
) -> None:
if span.op == OP.HTTP_CLIENT and span.origin not in ("auto.http.aiohttp",):
if span.op == OP.HTTP_CLIENT and span.origin not in (
"auto.http.aiohttp",
"auto.http.pyreqwest",
):
level = None
status_code = span._data.get(SPANDATA.HTTP_STATUS_CODE)
if status_code:
Expand Down
246 changes: 244 additions & 2 deletions tests/integrations/pyreqwest/test_pyreqwest.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
from pyreqwest.simple.sync_request import pyreqwest_get as sync_pyreqwest_get

import sentry_sdk
from sentry_sdk import start_transaction
from sentry_sdk import capture_message, start_transaction
from sentry_sdk.consts import MATCH_ALL, SPANDATA
from sentry_sdk.integrations.pyreqwest import PyreqwestIntegration
from tests.conftest import get_free_port
from tests.conftest import ApproxDict, get_free_port


class PyreqwestMockHandler(BaseHTTPRequestHandler):
Expand Down Expand Up @@ -956,3 +956,245 @@ def fake_start_span(*args, **kwargs):
assert SPANDATA.CODE_NAMESPACE in data
assert SPANDATA.CODE_FILEPATH in data
assert SPANDATA.CODE_FUNCTION in data


@pytest.mark.parametrize("send_default_pii", [True, False])
@pytest.mark.parametrize("span_streaming", [True, False])
def test_crumb_capture(
sentry_init,
capture_events,
server_port,
send_default_pii,
span_streaming,
):
def before_breadcrumb(crumb, hint):
crumb["data"]["extra"] = "foo"
return crumb

sentry_init(
integrations=[PyreqwestIntegration()],
before_breadcrumb=before_breadcrumb,
send_default_pii=send_default_pii,
trace_lifecycle="stream" if span_streaming else "static",
)

url = f"http://localhost:{server_port}/hello?q=test#frag"

events = capture_events()

client = SyncClientBuilder().build()
response = client.get(url).build().send()
assert response.status == 200

capture_message("Testing!")

(event,) = events

crumb = event["breadcrumbs"]["values"][0]
assert crumb["type"] == "http"
assert crumb["category"] == "httplib"

expected = {
SPANDATA.HTTP_METHOD: "GET",
SPANDATA.HTTP_STATUS_CODE: 200,
"extra": "foo",
}
if send_default_pii:
expected["url"] = f"http://localhost:{server_port}/hello"
expected[SPANDATA.HTTP_QUERY] = "q=test"
expected[SPANDATA.HTTP_FRAGMENT] = "frag"

assert crumb["data"] == ApproxDict(expected)


@pytest.mark.asyncio
@pytest.mark.parametrize("send_default_pii", [True, False])
async def test_async_crumb_capture(
sentry_init,
capture_events,
server_port,
send_default_pii,
):
sentry_init(
integrations=[PyreqwestIntegration()],
send_default_pii=send_default_pii,
)

url = f"http://localhost:{server_port}/hello?q=test#frag"

events = capture_events()

# Ensure the isolation scope contextvar is set before pyreqwest spawns
# its middleware on a separate asyncio Task. Without this, the child task
# lazily creates its own isolation scope, and breadcrumbs added there
# don't propagate back to this task's context.
sentry_sdk.get_isolation_scope()

with sentry_sdk.start_transaction():
async with ClientBuilder().build() as client:
response = await client.get(url).build().send()
assert response.status == 200

capture_message("Testing!")

(event,) = events

crumb = event["breadcrumbs"]["values"][0]
assert crumb["type"] == "http"
assert crumb["category"] == "httplib"

expected = {
SPANDATA.HTTP_METHOD: "GET",
SPANDATA.HTTP_STATUS_CODE: 200,
}
if send_default_pii:
expected["url"] = f"http://localhost:{server_port}/hello"
expected[SPANDATA.HTTP_QUERY] = "q=test"
expected[SPANDATA.HTTP_FRAGMENT] = "frag"

assert crumb["data"] == ApproxDict(expected)


@pytest.mark.asyncio
@pytest.mark.parametrize("send_default_pii", [True, False])
async def test_async_crumb_capture_span_streaming(
sentry_init,
capture_events,
server_port,
send_default_pii,
):
sentry_init(
integrations=[PyreqwestIntegration()],
send_default_pii=send_default_pii,
trace_lifecycle="stream",
)

url = f"http://localhost:{server_port}/hello?q=test#frag"

events = capture_events()

with sentry_sdk.traces.start_span(name="segment"):
async with ClientBuilder().build() as client:
response = await client.get(url).build().send()
assert response.status == 200

capture_message("Testing!")

(event,) = events

crumb = event["breadcrumbs"]["values"][0]
assert crumb["type"] == "http"
assert crumb["category"] == "httplib"

expected = {
SPANDATA.HTTP_METHOD: "GET",
SPANDATA.HTTP_STATUS_CODE: 200,
}
if send_default_pii:
expected["url"] = f"http://localhost:{server_port}/hello"
expected[SPANDATA.HTTP_QUERY] = "q=test"
expected[SPANDATA.HTTP_FRAGMENT] = "frag"

assert crumb["data"] == ApproxDict(expected)


@pytest.mark.parametrize(
"status_code,level",
[
(200, None),
(301, None),
(403, "warning"),
(405, "warning"),
(500, "error"),
],
)
def test_crumb_capture_client_error(
sentry_init,
capture_events,
server_port,
status_code,
level,
):
sentry_init(
integrations=[PyreqwestIntegration()],
)

url = f"http://localhost:{server_port}/status/{status_code}"

events = capture_events()

with sentry_sdk.start_transaction():
client = SyncClientBuilder().build()
response = client.get(url).build().send()
assert response.status == status_code

capture_message("Testing!")

(event,) = events

crumb = event["breadcrumbs"]["values"][0]
assert crumb["type"] == "http"
assert crumb["category"] == "httplib"

if level is None:
assert "level" not in crumb
else:
assert crumb["level"] == level

assert crumb["data"] == ApproxDict(
{
SPANDATA.HTTP_METHOD: "GET",
SPANDATA.HTTP_STATUS_CODE: status_code,
}
)


@pytest.mark.parametrize(
"status_code,level",
[
(200, None),
(301, None),
(403, "warning"),
(405, "warning"),
(500, "error"),
],
)
def test_crumb_capture_client_error_span_streaming(
sentry_init,
capture_events,
server_port,
status_code,
level,
):
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.


url = f"http://localhost:{server_port}/status/{status_code}"

events = capture_events()

with sentry_sdk.traces.start_span(name="segment"):
client = SyncClientBuilder().build()
response = client.get(url).build().send()
assert response.status == status_code

capture_message("Testing!")

(event,) = events

crumb = event["breadcrumbs"]["values"][0]
assert crumb["type"] == "http"
assert crumb["category"] == "httplib"

if level is None:
assert "level" not in crumb
else:
assert crumb["level"] == level

assert crumb["data"] == ApproxDict(
{
SPANDATA.HTTP_METHOD: "GET",
SPANDATA.HTTP_STATUS_CODE: status_code,
}
)
Loading