ref(aiohttp): Move crumbs to integration - #7135
Conversation
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
| span: "Union[Span, StreamedSpan, None]" | ||
| if has_span_streaming_enabled(client.options): | ||
| if sentry_sdk.traces.get_current_span() is None: | ||
| span = None |
There was a problem hiding this comment.
This had to be moved after the PII redaction, because even if we don't want to create a span, we do want to create a breadcrumb, and we need to apply the same PII redacting logic to breadcrumbs.
| scope: "sentry_sdk.Scope", span: "sentry_sdk.tracing.Span" | ||
| ) -> None: | ||
| if span.op == OP.HTTP_CLIENT: | ||
| if span.op == OP.HTTP_CLIENT and span.origin not in ("auto.http.aiohttp",): |
There was a problem hiding this comment.
This is just here to make sure we're not creating breadcrumbs the old way in transaction-based tracing anymore. Once all HTTP client integrations have been migrated, the whole function will go away
Codecov Results 📊✅ 99595 passed | ⏭️ 6479 skipped | Total: 106074 | Pass Rate: 93.89% | Execution Time: 356m 8s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 89.09%. Project has 2503 uncovered lines. Files with missing lines (2)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 89.99% 89.99% —%
==========================================
Files 193 193 —
Lines 24991 25015 +24
Branches 9012 9022 +10
==========================================
+ Hits 22490 22512 +22
- Misses 2501 2503 +2
- Partials 1437 1436 -1Generated by Codecov Action |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| if trace_config_ctx.span is None: | ||
| status = int(params.response.status) | ||
|
|
||
| breadcrumb = trace_config_ctx._sentry_breadcrumb |
There was a problem hiding this comment.
Bug: Accessing trace_config_ctx._sentry_breadcrumb in on_request_end may raise an AttributeError if it wasn't set in on_request_start due to the integration being disabled.
Severity: MEDIUM
Suggested Fix
In on_request_end, guard the access to trace_config_ctx._sentry_breadcrumb. You can use getattr(trace_config_ctx, "_sentry_breadcrumb", None) or a try...except AttributeError block to prevent the crash when the attribute is missing.
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/aiohttp.py#L503
Potential issue: In the `on_request_end` function, `trace_config_ctx._sentry_breadcrumb`
is accessed without first checking if it has been set. If the `on_request_start`
function returns early (for example, if the AioHTTP integration is disabled after a
`ClientSession` is created), `_sentry_breadcrumb` is never assigned. This leads to an
`AttributeError` when `on_request_end` attempts to access it, which can crash the
request handler. This can occur when a `ClientSession` is created while the integration
is enabled, Sentry is then re-initialized without the integration, and a request is
subsequently made using the original session.
ericapisani
left a comment
ericapisani
left a comment
There was a problem hiding this comment.
Non-blocking question, otherwise LGTM 🚀
| def add_http_breadcrumb(status_code, data): | ||
| # type: (Optional[int], dict[str, Any]) -> None |
There was a problem hiding this comment.
Is there something preventing this type definition from living inline like the following?
| def add_http_breadcrumb(status_code, data): | |
| # type: (Optional[int], dict[str, Any]) -> None | |
| def add_http_breadcrumb(status_code: "Optional[int]", data: "dict[str,Any]") -> "None": |
Or is it in a comment as a matter of personal preference?
2 participants
Description
Create HTTP client breadcrumbs directly in the aiohttp integration instead of using
maybe_create_breadcrumbs_from_span.👀 Best reviewed with Hide whitespace on.
This turned out to be a bigger change than expected:
send_default_pii/data_collection. So e.g. if parts of the query have been redacted on a span, they should not appear verbatim in the breadcrumb.Additionally:
spanon the aiohttp trace context to_sentry_spanIssues
Reminders
uv run ruff.feat:,fix:,ref:,meta:)