-
Notifications
You must be signed in to change notification settings - Fork 361
Error logging and examples #242
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
Conversation
Thanks for opening this PR! A decent chunk of these changes have been landed in this branch. Can you rebase your README.md changes + examples atop of this branch? Some of the comments are on your fork, by way, not on this PR. Can you comment on this PR in the future? |
@davidbarsky, I'll rebase once we have a resolution on |
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.
LGTM with a couple of points otherwise 👍
Co-authored-by: Veetaha <[email protected]>
# 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
…master`. * origin/davidbarsky/update-readme: Removed unnecessary + Sync on handler's future Added docs and missing _vars to reduce warnings Enforced Send+Sync for http Handler impl. Added some tracing!() to lib.rs. Fixed "Process exited" error in lib.rs. client.rs formatting fix Log panic as Debug+Display, improved examples Added err logging to lib.rs, consolidated examples Added error handling for #241, interim, broken. disable time on `tracing_subscriber` Add log and tracing examples; fix `tracing` dependency features. Create a reusable runtime struct + builder Cleanup docs; handle panics in lambda functions correctly. Add Cargo.lock to gitignore; whoops. don't depend on serde_derive directly Replace `genawaiter` with `async-stream` (#240) reorg
This looks good to me, but I wanted to be sure we addressed everything in previous reviews. @rimutaka @davidbarsky it appears there was some disagreement about removing tracing, but as far as I can tell tracing is still present. There was mention of a discussion in your fork, but I can't find it. Was this issue solved out of band? If so I'm comfortable approving and merging. |
I've got a branch up here that fixes all of the merge conflicts. It doesn't look like your branch is set up to accept commits from maintainers, so if I don't here from you in a few days I'll open a PR from that branch. |
This has been moved into: #284 The other PR just fixed merge conflicts and whatnot. Thanks for your contribution! |
Apologies for bumping a closed issue. What is the motivation for migrating from |
@seanpianka , the tracing is still there. https://github.com/awslabs/aws-lambda-rust-runtime/blob/master/lambda-runtime/src/lib.rs#L21. There was a discussion that it's overkill, but no action was taken to replace it with |
Issue #, if available: #241, #232, #216, #215
Description of changes:
tracing
withlog
simulated
feature from Cargo.tomlPlease, review the code carefully. I am new to Rust. Your comments and corrections are very welcome.
Important
I was unable to test this line from client.rs:
By submitting this pull request