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 📊✅ 104314 passed | ⏭️ 6677 skipped | Total: 110991 | Pass Rate: 93.98% | Execution Time: 363m 51s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 89.09%. Project has 2478 uncovered lines. Files with missing lines (2)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 90.15% 90.16% +0.01%
==========================================
Files 193 193 —
Lines 25147 25171 +24
Branches 9136 9146 +10
==========================================
+ Hits 22669 22693 +24
- Misses 2478 2478 —
- Partials 1431 1429 -2Generated by Codecov Action |
ericapisani
left a comment
There was a problem hiding this comment.
Non-blocking question, otherwise LGTM 🚀
| legacy_span.set_data("url", parsed_url.url) | ||
| legacy_span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query) | ||
| legacy_span.set_data(SPANDATA.HTTP_FRAGMENT, parsed_url.fragment) | ||
| breadcrumb.update( | ||
| { | ||
| SPANDATA.HTTP_QUERY: parsed_url.query, | ||
| SPANDATA.HTTP_FRAGMENT: parsed_url.fragment, | ||
| "url": parsed_url.url, | ||
| } |
There was a problem hiding this comment.
Legacy span path leaks unfiltered URL data into breadcrumbs
In the legacy span else: block, raw parsed_url.query and parsed_url.fragment are added to the breadcrumb unconditionally, bypassing should_send_default_pii/data_collection checks that the streaming path correctly applies.
Evidence
- The streaming span branch (lines 401–447) only adds query/fragment to
breadcrumbwhenhas_data_collection_enabledorshould_send_default_pii()is true. - The legacy
else:branch at line 460 addsbreadcrumb.update(...)insideif parsed_url is not None:with no PII guard at all. - Since
maybe_create_breadcrumbs_from_spanpreviously excludedauto.http.aiohttp, no breadcrumb was ever created for aiohttp; this PR adds breadcrumbs directly in the integration, meaning the unfiltered legacy path now leaks raw query strings to breadcrumbs. - The PR description explicitly requires breadcrumb data to be subject to PPI filtering, which the legacy path violates.
Identified by Warden · find-bugs · ZL9-MZY
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:)