-
-
Notifications
You must be signed in to change notification settings - Fork 167
fix(actix): capture only server errors #877
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #877 +/- ##
==========================================
+ Coverage 73.13% 73.29% +0.16%
==========================================
Files 64 64
Lines 7366 7407 +41
==========================================
+ Hits 5387 5429 +42
+ Misses 1979 1978 -1 🚀 New features to boost your workflow:
|
let req = TestRequest::get().uri("/test").to_request(); | ||
let res = app.call(req).await; | ||
assert!(res.is_err()); | ||
assert!(res.unwrap_err().error_response().status().is_client_error()); |
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.
[nit] I would check the exact status code, not just that it is a client error here
let req = TestRequest::get().uri("/test").to_request(); | ||
let res = app.call(req).await; | ||
assert!(res.is_err()); | ||
assert!(res.unwrap_err().error_response().status().is_server_error()); |
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.
[nit] also would check exact status code here
looks good, couple of small (mostly optional) suggestions |
Before this change, if a middleware processing the request after the Sentry one returned an error, we would capture it unconditionally.
With this change, we capture it only if it's a server error.
(note that the behavior was correct for responses returned by services (i.e. request handler, not middleware), as at line 373 we check if it's a server error).
This is one of the possible ways to tackle this, and I think this is the one that makes most sense for the following reasons:
It must be said that there is a small possibility that someone misses this in the changelog who was actually relying on these errors to show up in Sentry.
Close #876