Improve error message specificity for AWS initialization errors #10146
Conversation
Previously the purification step would fail with a message that contained just "Unable to build connector." Which doesn't explain why we're trying to build a connector, what kind of connector it is, or provide resolution steps. With this, we know that it's failing to validate, it will say something like "Access Denied" if it's not possible to access STS, etc.
| let sts_client = mz_aws_util::sts::client(&config)?; | ||
| let _ = sts_client.get_caller_identity().send().await?; | ||
| let _ok = mz_aws_util::sts::client(&config) | ||
| .map_err(|e| { |
There was a problem hiding this comment.
looks like client's error return type is anyhow::Error -- do you think it'd make sense to do mz_aws_util::sts::client(&config).context("Unable to build STS client to validate AWS credentials") instead of manually mapping the error?
Not a biggie but seems a little more best practices for using anyhow?
There was a problem hiding this comment.
Yeah anyhow's causes aren't printed unless you use the alternative format specifier ({:#} instead of just {}). We currently almost always use plain display, see my other comment here, so IMO the only way we can make context work correctly is to change all our error messages to not display their causes, and instead to always use the alternate form when formatting user-visible error messages. Which would be better, but is IMO out of scope for this PR.
use anyhow::{anyhow, Context};
fn err() -> anyhow::Result<()> {
Err(anyhow!("oh no!"))
}
fn main() {
let result = err().context("this is helpful").unwrap_err();
println!("basic: {}", result);
println!("expanded: {:#}", result);
}prints:
basic: this is helpful
expanded: this is helpful: oh no!
There was a problem hiding this comment.
Ahh TIL -- thanks for the explanation and playground link!
There was a problem hiding this comment.
IMO the only way we can make context work correctly is to change all our error messages to not display their causes, and instead to always use the alternate form when formatting user-visible error messages. Which would be better, but is IMO out of scope for this PR.
Yes. We should do this! We've been putting it off for way too long. I would much rather spend a few hours fixing this once and for all than continue to accumulate tech debt like this. I'm going to work on a PR for that.
There was a problem hiding this comment.
I checked, and we already correctly use the {:#} specifier when printing the cause for this error, so we can use .context here like @cjubb39 suggested.
| let connector = mz_http_proxy::hyper::connector().map_err(|e| anyhow!("{}", e))?; | ||
| pub fn connector(service: &str) -> Result<DynConnector, anyhow::Error> { | ||
| let connector = mz_http_proxy::hyper::connector() | ||
| .map_err(|e| anyhow!("Unable to build AWS {} HTTP connector: {}", service, e))?; |
There was a problem hiding this comment.
This seems like the wrong place to add service-specific context to me. There's nothing service-specific about this connector.
There was a problem hiding this comment.
Right, it's just used by the various other methods in the aws-util crate. Originally I had added this method as a map_err to all of them, but that was pretty silly duplicated code. If you'd prefer I can bring that back.
Originally all these clients were generated from a macro that interpolated the service name in error messages, this seemed like the least intrusive change that still supports good error messages.
|
Ok, so I looked into this more, and we can make AWS client construction infallible to match upstream. I sent #10161 for that with justification. |
Previously the purification step would fail with a message that contained just
"Unable to build connector." Which doesn't explain why we're trying to build a
connector, what kind of connector it is, or provide resolution steps.
With this, we know that it's failing to validate, it will say something like
"Access Denied" if it's not possible to access STS, etc.