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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[guide section]: Error handling #191

Open
rcoh opened this issue Aug 16, 2021 · 14 comments
Open

[guide section]: Error handling #191

rcoh opened this issue Aug 16, 2021 · 14 comments
Labels
documentation This is a problem with documentation p2 This is a standard priority issue

Comments

@rcoh
Copy link
Contributor

rcoh commented Aug 16, 2021

Community Note

  • Please vote on this issue by adding a 馃憤 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue, please leave a comment

Tell us about your request
Add guide section around error handling best practices

  • handling unknown
  • extracting error messages
  • using is_xyz() error check methods
  • converting to top-level service errors
@rcoh rcoh added the documentation This is a problem with documentation label Aug 16, 2021
@underscore-nul
Copy link

I just was about to file a request to this vein. I found myself writing the following in order to test against whether a DynamoDB request failed due to the conditional request or not:

match query.send().await {
        ...
    Err(ServiceError {
        err:
            PutItemError {
                kind: PutItemErrorKind::ConditionalCheckFailedException(_),
                ..
            },
        raw: _,
    }) => {
        ...
    }
        ...
}

Clearly this isn't exactly pleasant nor legible. After noting the issue I did find a slightly better method but it's only slightly better.

Err(SdkError::ServiceError { err: e, raw: _ })
    if e.is_conditional_check_failed_exception() =>
{
    ...
}

I think the larger issue here is the errors don't "feel" Rust-like. It'd be my expectation that instead of being returned a complex nested series of enums and structs holding the error type, I should instead be able to write something as simple as

Err(PutItemError::ConditionCheckFailed) => {
    ...
}

And go about my day. This is just my own way of thinking though and it may be way off-base.

@rcoh
Copy link
Contributor Author

rcoh commented Jan 31, 2022

Hello! All errors actually impl Into<aws_sdk_blahblah::Error> which is a top-level error generated for each service. This combines the error variants of all operations into a single error to simplify error handling.

If you match against this error, it will look at lot more like what you expect: https://docs.rs/aws-sdk-dynamodb/0.6.0/aws_sdk_dynamodb/enum.Error.html

The primary intention here is that you might have something like:

fn handle_dynamo_errors(err: aws_sdk_dynamodb::Error) -> ... {
}

fn my_code() -> Result<HappyCase, aws_sdk_dynamodb::Error> {
  dynamo_client.list_tables().send().await?; // `?` can use the `Into` conversion to convert the operation specific error into the top level error
}

but you can also use this to simplify error handling in general when you don't care handling different failure modes

@Shahab96
Copy link

Shahab96 commented Sep 3, 2022

I think it would be nice to have complete examples of error handling as a part of the regular documentation. Right now the documentation doesn't cover any form of error handling at all.

@ymwjbxxq
Copy link

ymwjbxxq commented Sep 8, 2022

@Shahab96
I did it in this way

 let result = self
            .dynamo_db_client
            .put_item()
            .table_name(&self.table_name)
            .item("prop1", AttributeValue::S(request.prop1.to_string()))
            .condition_expression("attribute_not_exists(prop1)")
            .send()
            .await;
        if result.is_err() {
            let sdk_error = &result.as_ref().unwrap_err();
            if let SdkError::ServiceError { err, .. } = sdk_error {
                if err.is_conditional_check_failed_exception() {
                    return Ok(None);
                }
            }
            return Err(ApplicationError::SdkError(result.unwrap_err().to_string()));
        }

@fnordpig
Copy link
Contributor

fnordpig commented Nov 24, 2022

It is a bit shocking that absolutely no examples are easily discovered on the internet for how to interact with the SDK error handling. Example: when you use CloudWatch Logs Put Log Events API with an invalid key it returns the next valid sequence token in the error. This is the only way to retrieve the next sequence token if you've lost it. God bless you if you can navigate how to get a SdkError down into a form where you can retrieve the sequence token. I'm a relatively novice Rust programmer, but an extreme experience programmer, and of all the complexities of Rust the way errors are handled in the AWS SDK are beyond the most complex. Yet, not a single example. Which given how clearly extreme care was taken in forming the system you would have thought there would be at least one example in the examples folder that does anything more than ? error handling. I love the SDK, but the error system is inscrutable to anyone but who originally wrote it and a hand full of super crusty crusts. All I want to do is to be able to call expected_sequence_token - I've spent literally days on this, and I feel not one bit closer in that time to any form of understanding.

@fnordpig
Copy link
Contributor

fnordpig commented Nov 24, 2022

As these sorts of things always work, using the bread crumbs of syntax in this discussion I stumbled into something that works and a very tenuous understanding of why. Again, these really need to be documented and some of these things were not ultra clear even from reading the rust books available on how match can let you bind structs. I have to be honest I still don't quite grok WHY this works - but I can accept that it does.


    let result = put_log_events.send().await;
    match result {
        Ok(_) => (),
        Err(aws_sdk_cloudwatchlogs::types::SdkError::ServiceError { err: PutLogEventsError { kind: aws_sdk_cloudwatchlogs::error::PutLogEventsErrorKind::InvalidSequenceTokenException(err),.. },.. }) => {
            println!("{:?}", err.expected_sequence_token());
        },
        _ => ()
    };

Specifically, the kind: section boggles my mind a bit as to why it works, but the only thing I can guess is it somehow hints to the compiler that err is in fact an InvalidSequenceTokenException, so it's ok to call functions associated with it. I don't think that's necessarily the AWS SDK's job to teach these concepts except that they're not what you encounter in learning rust through the standard paths, and while apparently a very clever way to handle things (I did read the sdk source code in the course of this to see how the errors are handled and generated), it's probably useful to clearly write out the various cases for handling errors given the error system richness and cleverness. Cleverness without documentation or examples is another word for obfuscation.

@fnordpig
Copy link
Contributor

(note I just realized you can also get the token from DescribeLogStreams - doh)

@jmklix jmklix added p2 This is a standard priority issue and removed p2 This is a standard priority issue labels Nov 28, 2022
@jdisanti
Copy link
Contributor

@fnordpig - Thank you for the feedback! I agree with what you're saying, and we definitely have room to improve here. The next SDK release will at least improve the error handling situation slightly, which you can read about in #657. We're also thinking about combining the Error and ErrorKind types to further simplify error handling, which you can read about and provide feedback for in this RFC.

@fnordpig
Copy link
Contributor

fnordpig commented Dec 1, 2022

Thanks! By the way, I love the SDK. Thanks for all the work you guys are doing!

@skeet70
Copy link

skeet70 commented Dec 20, 2022

This is really necessary. I converted a simple KMS project from Rusoto in 30 min, then spent a full day+ trying to track down all the error variants I already had mapped in Rusoto. The splits between SdkError, sdk::Error, and authentication/CredentialsError are really hard to follow and use. At the very least the patterns need to be manually documented, or there should be an example consumer that handles all the common errors for an SDK linked from the dev guide (which there may be, but I certainly can't find it!).

@josb
Copy link

josb commented Dec 21, 2022

I agree with this sentiment. A comprehensive error handling guide with examples would be very helpful.

@fnordpig
Copy link
Contributor

fnordpig commented Jan 7, 2023

@jdisanti finally read the rfc, sorry for the delay. It speaks a lot about the internals of the sdk at a level I don't have a lot of context on. It seems like a thorough and careful analysis of how a lot of stuff can be simplified, as well as the error handling auto generation reducing the sprawl of error stuff. That seems like a good idea. It's very complex as the comments here say.

I would say that just providing a handful of thoughtful examples of what can be done today will be:

  • Fast
  • Easy (for you!)
  • profoundly helpful to your customers of today
  • Provides a pivot point to demonstrate how your RFC improves things

That probably is what hinders my ability to provide a more detailed response on the rfc. Without what's in your head I can't understand how or why your proposal would make our lives easier, other than the assertions in the rfc around simplification and reduction. Worked examples would help the aws sdk customers of today be delighted, and will make them expectant of the proposed changes when they're compared against how you propose to improve things.

Looking forward to seeing the rfc manifested and reach my keyboard!

@jdisanti
Copy link
Contributor

jdisanti commented Jan 9, 2023

@fnordpig - My apologies, I think I must have split out the error simplification part of that RFC into a separate RFC by the time you got to it. Appreciate you taking a look!

@fnordpig
Copy link
Contributor

fnordpig commented Jan 9, 2023

@jdisanti Yes the proposal makes sense. I learned something about rust from your examples provided as well. You guys should provide more examples in the documentation for how to handle errors! Like a cookbook. But reading through your first example it showed me how they're intended to be used, which makes a lot more sense and would have saved a lot of trouble.

@jmklix jmklix added the p2 This is a standard priority issue label Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

9 participants