fix actix match name bug #539
Conversation
There was a problem hiding this comment.
NOTE to self: Axum recently got something similar which we might want to support with a feature flag as well:
https://docs.rs/axum/latest/axum/extract/struct.MatchedPath.html
sentry-actix/src/lib.rs
Outdated
| }; | ||
| /// Extract a transaction name from the HTTP request | ||
| fn transaction_name_from_http(req: &ServiceRequest) -> String { | ||
| format!("{} {}", req.method(), req.uri()) |
There was a problem hiding this comment.
I believe req.uri() might have too high of a cardinality and might even contain PII.
I believe match_pattern is the thing we want, is it documented to give you /user/{id}/profile for example: https://docs.rs/actix-web/latest/actix_web/struct.HttpRequest.html#method.match_pattern
match_name seems to refer to the function used for logging? Not sure, there is no example.
Adding a req.method() prefix does seem like a very good thing indeed.
There was a problem hiding this comment.
Agree 100%.
I've changed the code to use request.match_pattern(), fixed the broken tests and added an extra one that does a POST with a pattern.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 70.73% 70.87% +0.14%
==========================================
Files 66 66
Lines 6601 6626 +25
==========================================
+ Hits 4669 4696 +27
+ Misses 1932 1930 -2 |
Issue
Actix
request.match_name()is buggy.It doesn't care about the HTTP verb. It only looks at the
path. Thus 2 routes with the samepathwill have the same match name.Proposal
Previously,
format!("{} {}", req.method(), req.uri())was already used as a fallback for the transaction name.These changes make it as the default transaction name.