fix(starlette): Set transaction name on current scope in sync handler - #7201
Conversation
The sync request/response handler passed the _isolation_ scope to `_set_transaction_name_and_source`, but the transaction/segment span lives on the _current_ scope. As a result the route-resolved name never reached the span for sync endpoints, which were instead named by the raw URL from the ASGI middleware (`transaction_info.source` of `url` rather than `route`). Async handlers already used the current scope and were unaffected. Pass the current scope (already computed above) so sync and async handlers behave identically: - streaming: the segment name / `sentry.segment.name.source` are route-based - static: the transaction event name / source are route-based For parametrized routes this also removes high-cardinality URL transaction names for sync endpoints.
e043a2f to
b6107ba
Compare
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b6107ba. Configure here.
Codecov Results 📊✅ 117639 passed | ⏭️ 6732 skipped | Total: 124371 | Pass Rate: 94.59% | Execution Time: 423m 38s 📊 Comparison with Base Branch
➖ Removed Tests (1)View removed tests
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2488 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 90.25% 90.22% -0.03%
==========================================
Files 193 193 —
Lines 25433 25433 —
Branches 9346 9346 —
==========================================
+ Hits 22952 22945 -7
- Misses 2481 2488 +7
- Partials 1436 1436 —Generated by Codecov Action |
ericapisani
left a comment
There was a problem hiding this comment.
Great catch, and thanks for making these changes. Very minor suggestion for the span streaming test, but at the core this LGTM.
Thanks for also separating span streaming enabled and the transactions approach into 2 separate tests - will make it easier when we remove the transactions code 🔥
e7bbe5e to
4b5d7f3
Compare
The sync request/response handler passed the isolation scope to
_set_transaction_name_and_source, but the transaction/segment span lives on the current scope. As a result the route-resolved name never reached the span for sync endpoints, which were instead named by the raw URL from the ASGI middleware (transaction_info.sourceofurlrather thanroute). Async handlers already used the current scope and were unaffected.Pass the current scope (already computed above) so sync and async handlers behave identically:
sentry.segment.name.sourceare route-basedFor parametrized routes this also removes high-cardinality URL transaction names for sync endpoints.
Found while working on #7183.