Skip to content
Closed
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
21 changes: 21 additions & 0 deletions sentry_sdk/integrations/aiohttp.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
TransactionSource,
)
from sentry_sdk.tracing_utils import (
add_http_breadcrumb,
add_http_request_source,
has_span_streaming_enabled,
should_propagate_trace,
Expand Down Expand Up @@ -504,6 +505,26 @@ async def on_request_end(
with capture_internal_exceptions():
add_http_request_source(span)

with capture_internal_exceptions():
parsed_url = parse_url(str(params.url), sanitize=False)
breadcrumb_data = {
SPANDATA.HTTP_METHOD: params.method.upper(),
SPANDATA.HTTP_STATUS_CODE: status,
"reason": params.response.reason,
}
if parsed_url:
breadcrumb_data.update(
{
"url": parsed_url.url,
SPANDATA.HTTP_QUERY: parsed_url.query,
SPANDATA.HTTP_FRAGMENT: parsed_url.fragment,
}
)
add_http_breadcrumb(
status,
breadcrumb_data,
)

trace_config = TraceConfig()

trace_config.on_request_start.append(on_request_start)
Expand Down
16 changes: 15 additions & 1 deletion sentry_sdk/integrations/boto3.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from sentry_sdk.scope import should_send_default_pii
from sentry_sdk.traces import StreamedSpan
from sentry_sdk.tracing import Span
from sentry_sdk.tracing_utils import has_span_streaming_enabled
from sentry_sdk.tracing_utils import add_http_breadcrumb, has_span_streaming_enabled
from sentry_sdk.utils import (
capture_internal_exceptions,
parse_url,
Expand Down Expand Up @@ -112,18 +112,32 @@ def _sentry_request_created(
# request.context is an open-ended data-structure
# where we can add anything useful in request life cycle.
request.context["_sentrysdk_span"] = span
request.context["_sentrysdk_breadcrumb_data"] = {
SPANDATA.HTTP_METHOD: request.method,
"url": request.url,
}


def _sentry_after_call(
context: "Dict[str, Any]", parsed: "Dict[str, Any]", **kwargs: "Any"
) -> None:
span: "Optional[Union[Span, StreamedSpan]]" = context.pop("_sentrysdk_span", None)
breadcrumb_data: "Optional[Dict[str, Any]]" = context.pop(
"_sentrysdk_breadcrumb_data", None
)

# Span could be absent if the integration is disabled.
if span is None:
return
span.__exit__(None, None, None)

with capture_internal_exceptions():
status_code = parsed.get("ResponseMetadata", {}).get("HTTPStatusCode")
data = breadcrumb_data or {}
if status_code is not None:
data[SPANDATA.HTTP_STATUS_CODE] = status_code
add_http_breadcrumb(status_code, data)

body = parsed.get("Body")
if not isinstance(body, StreamingBody):
return
Expand Down
31 changes: 31 additions & 0 deletions sentry_sdk/integrations/httpx.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from sentry_sdk.integrations import DidNotEnable, Integration
from sentry_sdk.scope import should_send_default_pii
from sentry_sdk.tracing_utils import (
add_http_breadcrumb,
add_http_request_source,
has_span_streaming_enabled,
propagate_trace_headers,
Expand Down Expand Up @@ -128,6 +129,21 @@ def send(self: "Client", request: "Request", **kwargs: "Any") -> "Response":
with capture_internal_exceptions():
add_http_request_source(span)

breadcrumb_data = {
SPANDATA.HTTP_METHOD: request.method,
SPANDATA.HTTP_STATUS_CODE: rv.status_code,
"reason": rv.reason_phrase,
}
if parsed_url:
breadcrumb_data.update(
{
"url": parsed_url.url,
SPANDATA.HTTP_QUERY: parsed_url.query,
SPANDATA.HTTP_FRAGMENT: parsed_url.fragment,
}
)
add_http_breadcrumb(rv.status_code, breadcrumb_data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f6a9c0c. Configure here.


Comment on lines +142 to +146

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: 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.

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 function will be removed in part 2 (#7131). I'll merge part 2 into this PR before merging into master.

return rv

Client.send = send # type: ignore
Expand Down Expand Up @@ -220,6 +236,21 @@ async def send(
with capture_internal_exceptions():
add_http_request_source(span)

breadcrumb_data = {
SPANDATA.HTTP_METHOD: request.method,
SPANDATA.HTTP_STATUS_CODE: rv.status_code,
"reason": rv.reason_phrase,
}
if parsed_url:
breadcrumb_data.update(
{
"url": parsed_url.url,
SPANDATA.HTTP_QUERY: parsed_url.query,
SPANDATA.HTTP_FRAGMENT: parsed_url.fragment,
}
)
add_http_breadcrumb(rv.status_code, breadcrumb_data)

return rv

AsyncClient.send = send # type: ignore
31 changes: 31 additions & 0 deletions sentry_sdk/integrations/httpx2.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from sentry_sdk.integrations import DidNotEnable, Integration
from sentry_sdk.scope import should_send_default_pii
from sentry_sdk.tracing_utils import (
add_http_breadcrumb,
add_http_request_source,
has_span_streaming_enabled,
propagate_trace_headers,
Expand Down Expand Up @@ -129,6 +130,21 @@ def send(self: "Client", request: "Request", **kwargs: "Any") -> "Response":
with capture_internal_exceptions():
add_http_request_source(span)

breadcrumb_data = {
SPANDATA.HTTP_METHOD: request.method,
SPANDATA.HTTP_STATUS_CODE: rv.status_code,
"reason": rv.reason_phrase,
}
if parsed_url:
breadcrumb_data.update(
{
"url": parsed_url.url,
SPANDATA.HTTP_QUERY: parsed_url.query,
SPANDATA.HTTP_FRAGMENT: parsed_url.fragment,
}
)
add_http_breadcrumb(rv.status_code, breadcrumb_data)

return rv

Client.send = send # type: ignore
Expand Down Expand Up @@ -222,6 +238,21 @@ async def send(
with capture_internal_exceptions():
add_http_request_source(span)

breadcrumb_data = {
SPANDATA.HTTP_METHOD: request.method,
SPANDATA.HTTP_STATUS_CODE: rv.status_code,
"reason": rv.reason_phrase,
}
if parsed_url:
breadcrumb_data.update(
{
"url": parsed_url.url,
SPANDATA.HTTP_QUERY: parsed_url.query,
SPANDATA.HTTP_FRAGMENT: parsed_url.fragment,
}
)
add_http_breadcrumb(rv.status_code, breadcrumb_data)

return rv

AsyncClient.send = send # type: ignore
48 changes: 48 additions & 0 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 @@ -156,6 +157,14 @@ async def sentry_async_middleware(
if sentry_sdk.get_client().get_integration(PyreqwestIntegration) is None:
return await next_handler.run(request)

method = request.method

# If we want to access request.url, we need to do it early. It can't be
# retrieved after the request has been sent
parsed_url = None
with capture_internal_exceptions():
parsed_url = parse_url(str(request.url), sanitize=False)

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

breadcrumb_data = {
SPANDATA.HTTP_METHOD: method,
SPANDATA.HTTP_STATUS_CODE: response.status,
}
if parsed_url:
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


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

method = request.method

# If we want to access request.url, we need to do it early. It can't be
# retrieved after the request has been sent
parsed_url = None
with capture_internal_exceptions():
parsed_url = parse_url(str(request.url), sanitize=False)

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

breadcrumb_data = {
SPANDATA.HTTP_METHOD: method,
SPANDATA.HTTP_STATUS_CODE: response.status,
}

if parsed_url:
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
16 changes: 16 additions & 0 deletions sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,22 @@ def record_sql_queries(
yield span


def add_http_breadcrumb(status_code, data):
# type: (Optional[int], dict[str, Any]) -> None
level = None
if status_code:
if 500 <= status_code <= 599:
level = "error"
elif 400 <= status_code <= 499:
level = "warning"

kwargs: "dict[str, Any]" = {"type": "http", "category": "httplib", "data": data}
if level:
kwargs["level"] = level

sentry_sdk.add_breadcrumb(**kwargs)
Comment thread
sentrivana marked this conversation as resolved.


def maybe_create_breadcrumbs_from_span(
scope: "sentry_sdk.Scope", span: "sentry_sdk.tracing.Span"
) -> None:
Expand Down
Loading