Skip to content

show crates.io search errors to the user without logging them #2480

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

Open
syphar opened this issue Apr 4, 2024 · 4 comments · May be fixed by #2751
Open

show crates.io search errors to the user without logging them #2480

syphar opened this issue Apr 4, 2024 · 4 comments · May be fixed by #2751
Assignees
Labels
A-backend Area: Webserver backend E-easy Effort: Should be easy to implement and would make a good first PR mentor This has instructions for getting started P-low Low priority issues

Comments

@syphar
Copy link
Member

syphar commented Apr 4, 2024

We are using the crates.io search API to power our own crate-search.

Sometimes the crates.io API returns 4xx or 5xx errors because of wrong queries or timeouts.

These errors can be shown to the user, but there is no need to report them to sentry or log them as errors.

We could add a new metric to track the amount of errors.

@syphar syphar added mentor This has instructions for getting started E-easy Effort: Should be easy to implement and would make a good first PR A-backend Area: Webserver backend labels Apr 4, 2024
@syphar
Copy link
Member Author

syphar commented Apr 4, 2024

instructions:

  • web::releases::get_search_results fetches the search results, it's used in web::releases::search_handler
  • our logging config would send everything that is logged with tracing::error! to sentry, and also every server error
  • so either the handler or the method have to be changed so we don't generate a server error any more, but just a warning.

@syphar syphar added the P-low Low priority issues label Apr 4, 2024
@byfnoel
Copy link

byfnoel commented Sep 4, 2024

@rustbot claim

byfnoel added a commit to byfnoel/docs.rs that referenced this issue Feb 28, 2025
When queries to `crates.io` API return errors, we will display them
instead of logging them as errors.

Closes rust-lang#2480
@byfnoel
Copy link

byfnoel commented May 20, 2025

cc: @syphar, @GuillaumeGomez

After a second take, I believe the ? bubble up errors that eventually convert to AxumNope::InternalError, which reports to Sentry and also logs as errors.

I was thinking of creating a custom error response based on how it is constructed in registry_api.rs.
See:

if let Some(errors) = response.errors {

           let messages: Vec<_> = errors.into_iter().map(|e| e.detail).collect();

           bail!("got error from crates.io: {}", messages.join("\n"));

       }

My propose solution (This goes inside src/web/releases.rs):

        match get_search_results(&mut conn, &registry, query_params).await {  
            Ok(result) => result,  
            Err(error) => { 
                // if we get an error from crates.io search api
                if error.to_string().contains("got error from crates.io:") {  
                    // Create a custom error response. Note: This would not report to Sentry 
                    
                    };  
                    return Ok(custom_error.into_response());  
                }  
                // For other errors, propagate as usual  
                return Err(AxumNope::InternalError(error));  
            }  
        }  

@syphar
Copy link
Member Author

syphar commented May 22, 2025

@byfnoel thank you for working on this!

generally: differentating between error types should never be done via text matching.

The cleanest way would be to introduce a new error type (enum with thiserror), that is returned by get_search_result which would enable us to differentiate between a crates.io-error and the other errors. Then you could also have the crates.io error variant contain a nice error message with whatever is coming from the crates.io API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend Area: Webserver backend E-easy Effort: Should be easy to implement and would make a good first PR mentor This has instructions for getting started P-low Low priority issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants