erratic-pattern commented on issue #6377: URL: https://github.com/apache/arrow-rs/issues/6377#issuecomment-2412255655
I feel that cramming more error sources into the `Display` is a step in the wrong direction. Despite the half-baked edges of Rust error handling making this a tricky decision for library maintainers, following standard practices should lead to a better outcome for both application developers and library developers. As @crepererum pointed out, right now for a application developer, there is a confusing incompatibility in error messaging between Arrow ecosystem crates and (most?) other third-party crates. Using `Display` seems to "just work" for getting detailed information for Arrow crates, until the error goes into a third-party crate and then it doesn't. "So just use an error reporting library, "as the `reqwest` maintainers suggest. Well, no. Even if you follow "best practice" you're still hindered as an application developer because using any of the available error reporting crates with `arrow-rs` will produce very redundant and ugly error reports. To highlight this visually I've made [an example repo](https://github.com/erratic-pattern/arrow-error-reporting-examples) that creates a simple "application" using `object_store` with a popular error reporting crate called `color_eyre`. It attempts to connect to an S3 bucket that doesn't exist and then displays an error. Here is the output: ```console Error: 0: Generic S3 error: Error after 10 retries in 5.723990834s, max_retries:10, retry_timeout:180s, source:error sending request for url (http://169.254.169.254/latest/api/token) 1: Error after 10 retries in 5.723990834s, max_retries:10, retry_timeout:180s, source:error sending request for url (http://169.254.169.254/latest/api/token) 2: error sending request for url (http://169.254.169.254/latest/api/token) 3: client error (Connect) 4: tcp connect error: Host is down (os error 64) 5: Host is down (os error 64) Location: examples/object_store.rs:11 Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it. Run with RUST_BACKTRACE=full to include source snippets. ``` Maybe this is tolerable, but it's definitely ugly. You are at least getting as much information about the error chain as possible. Additionally, by cramming more information into the top-level error message, you're helping users in the default case to discover useful errors beyond just "Generic S3 error". So there is an argument to be made that the current state is acceptable, even if not ideal.. I think this discussion is beyond the scope of just "improve `reqwests` error display" and we should create an issue to discuss a more broad improvement to error messaging with both the default rust panic handler as well as the `color_eyre` (or similar crate) panic handler. If that sounds reasonable, I can write one up with points made from this discussion. I would also like to create a similar example repo for DataFusion to bring up the discussion there, since they also use the same approach to error messaging, but I think the discussion needs to happen here primarily since DataFusion is largely required to follow whatever convention `arrow-rs` takes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
