-
Notifications
You must be signed in to change notification settings - Fork 361
Added error handling for #241, interim, broken #244
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
Added error handling for #241, interim, broken #244
Conversation
Co-authored-by: Veetaha <[email protected]>
lambda/examples/error-handling.rs
Outdated
use simple_error; | ||
use simple_logger; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use simple_error; | |
use simple_logger; | |
use simple_error; | |
use simple_logger; |
This will generate a clippy warning: https://rust-lang.github.io/rust-clippy/master/#single_component_path_imports
@@ -31,3 +31,5 @@ async-stream = "0.2" | |||
tracing-subscriber = "0.2" | |||
once_cell = "1.4.0" | |||
simple_logger = "1.6.0" | |||
log = "0.4" | |||
simple-error = "0.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using anyhow
instead of simple-error
. simple-error
has only 17 stars and was last updated a year ago...
@@ -131,7 +131,7 @@ impl<F, A, B, Error, Fut> Handler<A, B> for HandlerFn<F> | |||
where | |||
F: Fn(A, Context) -> Fut, | |||
Fut: Future<Output = Result<B, Error>> + Send + Sync, | |||
Error: Into<Box<dyn std::error::Error + Send + Sync + 'static>> + fmt::Debug, | |||
Error: Into<Box<dyn std::error::Error + Send + Sync + 'static>> + fmt::Display, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why we have + fmt::Display/Debug
here at all. std::error::Error
requires Debug + Display
as supertraits.
cc @davidbarsky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if fmt:Display is removed as you suggested, rustc complains over error!("{}", e);
and format!("{}", e)
with message
`<F as Handler<A, B>>::Error` doesn't implement `std::fmt::Display`
the trait `std::fmt::Display` is not implemented for `<F as Handler<A, B>>::Error`
in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
required by `std::fmt::Display::fmt`rustc(E0277)
I read something about tracing macro allowing something like error!("{}", %e);
to force Display over Debug, but can't find it any more. Even if that worked It may be too risky to rely on that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it seems is the Error type is Into
an Error
. You may need error!("{}", e.into());
You most definitely get Display with Error. You also got Debug with the fmt string. Again I think the display message should be Display. Debug is not for display |
That's my bad—I see the test failures. I'll work on fixing that and let you know when I've pushed the fix.
Two questions:
|
@davidbarsky David, looks like it logs from use lambda::{handler_fn, Context};
use serde_json::Value;
use tracing::{error, info, debug};
type Error = Box<dyn std::error::Error + Send + Sync + 'static>;
#[tokio::main]
async fn main() -> Result<(), Error> {
let func = handler_fn(func);
simple_logger::init_with_level(log::Level::Info); // ::init().unwrap();
error!("Error from main!");
info!("Info from main!");
debug!("Debug from main!");
lambda::run(func).await?;
Ok(())
}
async fn func(event: Value, _: Context) -> Result<Value, Error> {
error!("Error from func!");
info!("Info from func!");
debug!("Debug from func!");
//Ok(event)
Err(simple_error::SimpleError::new("simple_logger::init_with_level(log::Level::Info);").into())
} Expected actionCloudWatch log entries should contain output from: Test input:
Output: The output is as expected.
Cloudwatch log output: Only
The other problem here is ...The error message produced by the handler was not logged by the runtime's lib.rs in Using
|
OK, I think I understand where my confusion comes from now. I expected
to be logged in CloudWatch and returned to the caller. It is returned to the caller, but is not logged in CloudWatch. Inserting Sorry for wasting your time. I'll make a commit with the fix in a few hours. |
Oustanding:
|
lambda/src/lib.rs
Outdated
request_id, | ||
diagnostic: Diagnostic { | ||
error_type: type_name_of_val(&err).to_owned(), | ||
error_message: format!("{:?}", err), | ||
error_message: "Lambda panicked!".to_owned(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not lose the why here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point! I wouldn't want my stack trace and all to go to the front end, though. How about a compromise: Display -> Response, Debug -> CloudWatch? Open to suggestions, but would wait and see how it pans out in production with different error types.
Err(err) if err.is_panic() => {
error!("{:?}", err); // inconsistent with other log record formats - to be reviewed
EventErrorRequest {
request_id,
diagnostic: Diagnostic {
error_type: type_name_of_val(&err).to_owned(),
error_message: format!("Lambda panicked: {}", err),
},
}
.into_req()
}
Response
{
"errorType": "Runtime.ExitError",
"errorMessage": "RequestId: 2d579019-07f7-409a-a6e6-af7725253307 Error: Runtime exited with error: exit status 101"
}
CloudWatch:
START RequestId: 2d579019-07f7-409a-a6e6-af7725253307 Version: $LATEST
thread 'main' panicked at 'explicit panic', lambda/examples/error-handling.rs:87:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
END RequestId: 2d579019-07f7-409a-a6e6-af7725253307
REPORT RequestId: 2d579019-07f7-409a-a6e6-af7725253307 Duration: 43.40 ms Billed Duration: 100 ms Memory Size: 128 MB Max Memory Used: 27 MB Init Duration: 23.15 ms
RequestId: 2d579019-07f7-409a-a6e6-af7725253307 Error: Runtime exited with error: exit status 101
Runtime.ExitError
@@ -131,7 +131,7 @@ impl<F, A, B, Error, Fut> Handler<A, B> for HandlerFn<F> | |||
where | |||
F: Fn(A, Context) -> Fut, | |||
Fut: Future<Output = Result<B, Error>> + Send + Sync, | |||
Error: Into<Box<dyn std::error::Error + Send + Sync + 'static>> + fmt::Debug, | |||
Error: Into<Box<dyn std::error::Error + Send + Sync + 'static>> + fmt::Display, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it seems is the Error type is Into
an Error
. You may need error!("{}", e.into());
lambda/examples/basic.rs
Outdated
/// to be serialized into json. The runtime pays no attention | ||
/// to the contents of the response payload. | ||
#[derive(Serialize)] | ||
struct Response { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability can we keep the type defs at the top of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error!("{}", e.into()); kept throwing errors no matter what I tried. I do not understand it enough to make a meaningful fix. Left as is for now.
lambda/examples/error-handling.rs
Outdated
/// The actual handler of the Lambda request. | ||
pub(crate) async fn func(event: Value, ctx: lambda::Context) -> Result<Value, Error> { | ||
// convert the JSON request to a struct | ||
let req = serde_json::from_value::<Request>(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protip. You can use serde_json::from_value::<Request>(event)?;
above and remove one level of unpacking here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match serde_json::from_value::(event)?.event_type {
EventType::SimpleError => {
I'm no expert here, but it looks like exiting fn main causes this Process exited before completing request error. Should we wrap the handler invocation in a loop, somewhere in lib.rs after the runtime has been initialized so that it never exits? I looped it inside main for a test and the Process exited... errors stopped. async fn main() -> Result<(), Error> {
loop {
let func = handler_fn(func);
lambda::run(func).await?;
}
} It actually worked OK on master branch, somehow ... |
|
Sure. What confused me was that the polling protocol doesn't seem to work because I see it does not even try to execute the loop inside |
Outstanding:
|
@davidbarsky, @softprops , will you get a chance to look into the failed compilation of lambda-http this week? If not, I might give it a crack on Fri. Not sure I'm skilled enough to fix it, though. |
I might be able to this weekend. My ask here is really make sure the breaking changes are worth it. |
@softprops Doug, I spent all afternoon going in circles, but I cannot find the source of Sync/Send problem with the HTTP crate. Can you give me any pointers? |
Sorry about that. I've been busy with some family stuff recently. Is there any way to break up this pr to make it smaller to understand the changes? |
@softprops, I think I fixed it. Yay! :) Will tidy up the code and make a commit shortly. |
@softprops , @davidbarsky , @Veetaha, can you review my latest changes? |
# Conflicts resolved in favour of davidbarsky/update-readme: # Cargo.lock # lambda/Cargo.toml # lambda/examples/README.md # lambda/examples/basic.rs # lambda/examples/error-handling.rs # lambda/src/client.rs # lambda/src/lib.rs
I've just run into a new issue - we cannot use any async fns inside the handler unless they implement
(fixed in rimutaka@19e6881#diff-d9a13770c705340766ae33d16aac07afR133) |
Err(err) => EventErrorRequest { | ||
}, | ||
Err(err) if err.is_panic() => { | ||
error!("{:?}", err); // inconsistent with other log record formats - to be reviewed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of changes in this diff, but it's wasn't this actually the original intended changeset: to make this a Display-able message like other runtimes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was like 3 lines of code on top of master branch. Most other changes were fixes to the unfinished work of @davidbarsky.
@@ -331,7 +343,7 @@ where | |||
.expect("Unable create runtime"); | |||
|
|||
let client = &runtime.client; | |||
let incoming = incoming(client).take(1); | |||
let incoming = incoming(client); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take(1) was breaking the loop that resulted in an error message that the function failed.
Why was Thank you for your work on this pull request but it would be awesome if we could break this pull up into chunks that solve specific problems. I see rustdocs added for existing to types, readme additions, breaking api changes, and new examples all in the same pull. perhaps if we revisited this pull into pulls that addressed each of those we can make some movement on this. |
Someone added + Sync somewhere and then it proliferated itself. I'll try to remove it. Had to add docs to keep the formatter happy. Agree the pull is too big. What I can do is just move the error handling changes back to master, which is like 3 lines of code and let @davidbarsky to finish the work he started. |
👍 |
I think this PR was superseded by PR #284. Shall we close it? |
@rimutaka Good call, closing it now. Thanks! |
Issue #, if available: #241, relates to PR #242
Description of changes:
Added my changes from #242 on top of David's changes in this branch.
For example, hello-without-macro-tracing.rs has these 2 lines:
so it should at least log Hello. It doesn't.
This line returns an error for a test:
Err(simple_error::SimpleError::new("simple error").into())
.Output:
Trace in CloudWatch:
As you can see, neither
info!("Hello!")
norsimple error
messages are there.I am not sure what other changes are intended for lib.rs and what state it is in, so it's better if @davidbarsky takes a look first.
TODO
By submitting this pull request