feat(integrations): Add http.route attribute to server spans - #7183
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Results 📊✅ 118315 passed | ⏭️ 6732 skipped | Total: 125047 | Pass Rate: 94.62% | Execution Time: 413m 24s 📊 Comparison with Base Branch
➖ Removed Tests (1)View removed tests
All tests are passing successfully. ✅ Patch coverage is 98.08%. Project has 2485 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 90.24% 90.26% +0.02%
==========================================
Files 193 193 —
Lines 25474 25519 +45
Branches 9378 9402 +24
==========================================
+ Hits 22988 23034 +46
- Misses 2486 2485 -1
- Partials 1431 1431 —Generated by Codecov Action |
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
dddf367 to
b1363a5
Compare
…#7201) 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. Found while working on #7183.
…#7201) 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. Found while working on #7183.
b1363a5 to
1d15a6d
Compare
mjq
changed the title
http.route attribute to HTTP server spans
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1d15a6d. Configure here.
| name = _DEFAULT_TRANSACTION_NAME | ||
| source = TransactionSource.ROUTE | ||
| elif source == TransactionSource.ROUTE: | ||
| scope.set_segment_attribute(SPANDATA.HTTP_ROUTE, name) |
There was a problem hiding this comment.
Host routes set raw request path
Low Severity
For Starlette Host routes, _transaction_name_from_router falls back to the raw request path. That value is then stored as http.route, which is supposed to be a path template. High-cardinality paths can leak into span attributes and descriptions.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1d15a6d. Configure here.
There was a problem hiding this comment.
This was an existing bug. PR up to fix in #7266.
| for attribute, value in attributes.items(): | ||
| self.set_attribute(attribute, value) | ||
|
|
||
| def set_segment_attribute(self, key: str, value: "AttributeValue") -> None: |
There was a problem hiding this comment.
This seemed like a useful helper function to have (and avoids duplicating this code in every HTTP integration), but I would also understand if we don't want to expand the API surface with this: it makes the "segment" name a public thing which AIUI we are trying to avoid?
| name = _DEFAULT_TRANSACTION_NAME | ||
| source = TransactionSource.ROUTE | ||
| elif source == TransactionSource.ROUTE: | ||
| scope.set_segment_attribute(SPANDATA.HTTP_ROUTE, name) |
There was a problem hiding this comment.
This was an existing bug. PR up to fix in #7266.
1 participant
Description
Currently the HTTP path template is only available in the span name. Make it available as a semantic attribute as well (
http.route- definition in conventions).This will also let us use this in e.g. HTTP server span
descriptiongeneration, which will be necessary to maintain consistent descriptions between transactions and span streaming.Issues
http.routemissing from HTTP server spans #7182