Skip to content
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

use Debug impl for generic Reqwest errors to provide more info #6518

Conversation

erratic-pattern
Copy link
Contributor

Closes #6377

This should help in the interim to identify specific causes of, sometimes confusing, network related errors. I think this change should be temporary until we can find a better way to improve error messages.

@github-actions github-actions bot added the object-store Object Store Interface label Oct 6, 2024
see apache#6377

This should help in the interim to identify specific causes of, sometimes confusing, network
related errors. I think this change should be temporary until we can find a better way to
improve error messages.
@tustvold
Copy link
Contributor

tustvold commented Oct 6, 2024

I think this runs the risk of inadvertently exposing details that shouldn't be rendered in logs, I therefore think we should instead look to improve the logging in a more targeted fashion.

@erratic-pattern erratic-pattern marked this pull request as draft October 7, 2024 13:11
@erratic-pattern
Copy link
Contributor Author

I think this runs the risk of inadvertently exposing details that shouldn't be rendered in logs, I therefore think we should instead look to improve the logging in a more targeted fashion.

So I think there's two ways to go about this:

  1. move away from snafu and do explicit Display impls that show more information about the reqwest error.

  2. convert reqwest::Error to a new internal wrapper type which classifies the errors based on category and includes information like cause, and provides its own snafu-based Display implementation so that we can customize how reqwest errors are displayed.

Do you have any preference here? I think the latter is possibly a cleaner option. I'd just like to get some kind of permission before I go mucking around with the existing error conventions in the library.

@tustvold
Copy link
Contributor

tustvold commented Oct 8, 2024

I would recommend taking a step back and working out what it is we are wanting to expose beyond what we do currently, and getting consensus on the ticket. As it stands I am a little unsure what we're aiming to achieve here, and therefore struggle to advise on how best to achieve it.

The reqwest maintainer states that users should walk the error chain if they want more detailed error information - seanmonstar/reqwest#2373 (comment). There is therefore a reasonable argument that this custom logic doesn't even belong in this crate, and users wanting more detailed errors should look to display the error chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve reqwests error display
2 participants