Conversation
github-actions
bot
added
the
S-waiting-on-review
|
Looks good to me, thanks! I'll let @syphar take a look as well. |
|
Thank you for the review @GuillaumeGomez. Let's see if @syphar wants anything changed. |
syphar
left a comment
syphar
left a comment
There was a problem hiding this comment.
Thank you for the good work! I'm very happy to see this coming.
src/web/releases.rs
Outdated
| && let Some(status) = registry_request_error.status() | ||
| && (status.is_client_error() || status.is_server_error()) | ||
| { | ||
| return Err(SearchError::CratesIo(format!( |
There was a problem hiding this comment.
looking at RegistryApi::search, I can see other errors using bail! that might also be interesting for the user.
what do you think? should we change RegistryApi::search to return SearchError too?
There was a problem hiding this comment.
Great suggestion. However, I have great reserve about this because RegistryApi might introduce new problems such as limited reusability, and how we might want to handle errors in different part of the codebase.
There was a problem hiding this comment.
kk, understood.
I think I wouldn't have the worry about limited reusablility for a thing that is only used once currently.
But I'm fine leaving it as-is for now.
Caveat: there are errors happening in RegistryApi::search using bail!, which now would still end up as server errors & reported to sentry, while they could be shown to the user without any error logging.
byfnoel
force-pushed
the
categorize-errors
branch
from
c697b6b to
5f81817
Compare
Co-authored-by: Denis Cornehl <denis@cornehl.org>
src/web/releases.rs
Outdated
| .await | ||
| .get("/releases/search?query=anything_goes_here") | ||
| .await?; | ||
| assert_eq!(response.status(), 503); |
There was a problem hiding this comment.
one point of the whole story also was to show the error that we get from crates.io directly to the user.
Where is that tested?
( feel free to drop me a line if you're not sure how, other tests parse the HTML and test if parts exist / contain certain text)
|
feel free to re-request a review when you're ready again |
|
Thank you! Let’s do it, @syphar 👍. Please feel free to point me to a better solution if needed. I appreciate the feedback. |
byfnoel
force-pushed
the
categorize-errors
branch
from
7d82b95 to
3bf347e
Compare
byfnoel
force-pushed
the
categorize-errors
branch
from
5f86d24 to
3bf347e
Compare
syphar
left a comment
syphar
left a comment
There was a problem hiding this comment.
sorry for the delay, my dayjob & family took too much time
| )); | ||
| } | ||
|
|
||
| // Errors from bail!() in RegistryApi::search() |
There was a problem hiding this comment.
I don't see why we would match strings differentiate errors coming from a function in the same codebase.
I remember we already had a partial discussion about this.
I'm open to other approaches, that don't involve string matching
syphar
added
S-waiting-on-author
|
Thank you for the work until here! Since it became more pressing, I solved a subset in #3196 , feel free to improve further, or close this PR. |
3 participants
This PR improves error handling for queries to the
crates.ioAPI by categorizing error responses and updating how they are surfaced to users. Instead of logging 4xx or 5xx errors and reporting them to sentry, now we simply show them directly to the user.Previously, all errors from the crates.io API were logged and reported to Sentry. By surfacing these errors to users, we get to reduce noise in the tracking system and provide more descriptive responses.
Closes #2480