-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework error handling example #2382
Conversation
.get::<MatchedPath>() | ||
.map(|matched_path| matched_path.as_str()); | ||
|
||
tracing::debug_span!("request", %method, %uri, matched_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As-is, the uri (and method) won't be in quotes but the matched path will, as the comment at the start of the file shows correctly. I find this pretty weird. Maybe use let uri = req.uri().to_string();
above instead, and remove the %
before uri
here? For the method, it probably makes sense for it not to be in quotes.
AppError::TimeError(err) => { | ||
// Because `TraceLayer` wraps each request in a span that contains the request | ||
// method, uri, etc we don't need to include those details here | ||
tracing::error!(%err, "error from time_library"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe controversial, but I find the err=
bit in the output a bit weird. IMHO tracing fields are much more useful for fields whose exact value you'd want to match when searching logs, rather than error messages that are generally just searched for specific keywords.
tracing::error!(%err, "error from time_library"); | |
tracing::error!("error from time_library: {err}"); |
Not sure what the chances are of me stumbling upon links to the deleted example just as this merges, haha. I just wanted to highlight these references to the deleted/renamed file:
Not familiar with contributing, so let me know if an issue would be preferable. :) |
Thanks for noticing! I've filed an issue #2464 |
This reworks the ye olde error-handling-and-dependency-injection. I've removed all the dependency injection bits because I think they're distracting and deserve their own example.
Hopefully this new version is easier to follow and can serve as a template for people to build upon.