-
Notifications
You must be signed in to change notification settings - Fork 361
Lambda Extensions #376
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
Lambda Extensions #376
Conversation
Extract the HTTP client logic into a new crate. This allows other crates to share the same logic to interact with the Runtime API. Signed-off-by: David Calavera <[email protected]>
This new crate encapsulates the logic to create Lambda Extensions. It includes reference examples. Signed-off-by: David Calavera <[email protected]>
Thanks a lot for taking the time to create this PR @calavera ! I'll take a look at it tomorrow. 😄 |
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.
Hey @calavera!
Thanks a lot for the time and efforts in creating this PR! Overall it's pretty solid and I'm for the idea of splitting the API client into its separate crate.
In term of potential improvements before merging this, there are three main points for me:
- I'm not too keen on adding a dependency on
async_trait
, as it would diverge from how theHandler
trait works at the moment. - I think we should push error handling under the hood as much as possible by catching the
Err
s and sending the right API call on behalf of the developer. - Right now,
lambda-extension
listens to both INVOKE and SHUTDOWN events. While doing an Ok(()) is not a big deal, I think it would be nice to be able to configure what events to listen to, and then get an enum as payload.
Once upon a time, there was a crate called `lambda-runtime-client`. We don't want people to mistake this new crate with the old one. Signed-off-by: David Calavera <[email protected]>
- Remove async_trait dependency. - Use a similar handler api than the runtime. - Allow to register the extension for only certain events. Signed-off-by: David Calavera <[email protected]>
@nmoutschen I've updated this PR to address your comments. Can you take a look at the examples now and let me know what you think? |
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.
Thanks a lot for the update @calavera!
I really like the changes you made! The main change I think would help here before I can approve is on the Extension
trait signature, by making self mutable to be in line with recent changes in this repository (but not published yet).
I also have some other small remarks/nitpicks, but up to you to decide if you agree with me or not.
- Remove extension ID from call signature. - Make extension mutable. - Add example showing how to implement an extension with a struct. Signed-off-by: David Calavera <[email protected]>
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 from an implementation POV!
Signed-off-by: David Calavera <[email protected]>
Signed-off-by: David Calavera <[email protected]>
@nmoutschen I've added documentation where it was necessary and opened the PR for review. Thanks a lot for your feedback. |
Signed-off-by: David Calavera <[email protected]>
I still have to update the README with information about this new crate. |
Signed-off-by: David Calavera <[email protected]>
Signed-off-by: David Calavera <[email protected]>
@nmoutschen I was not sure where to add information to the main readme, so I opted for creating two specific readmes inside the new crate directories. Let me know what you think. |
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! Just 2 small details on libc for various OS versions, and on how users probably won't use the runtime client directly.
Signed-off-by: David Calavera <[email protected]>
@nmoutschen I decided to remove the paragraph about targets explicitly. I think it was confusing and hard to explain. I'd expect that people that want to write extensions are already familiar with functions, and how the runtime works. It's still worth mentioning that the target must match the architecture that functions run on. |
@nmoutschen what are the next steps to get this PR merged? |
Hey @calavera ! Not much on your side. I wanted to run a complete test by building an extension and running on Lambda before merging this just to make sure. I'm on sick leave at the moment, but I should be able to do it next week. |
Thanks for letting me know, I wasn't sure if there was anything missing. Take care |
lambda-extension/src/lib.rs
Outdated
Some(name) => name.into(), | ||
None => { | ||
let args: Vec<String> = std::env::args().collect(); | ||
args[0].clone() |
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.
After doing some tests on Lambda, this causes an issue with the Lambda Extensions API. Because this value contains slashes (e.g. /opt/extension/my-extension
), the Lambda Extension API will fail to process it correctly, and assumes that this is an internal extension.
Because of this, it will fail to register the extension if it contains/starts with (not sure which one) a slash.
However, taking the last part of the path works. E.g. instead of /opt/extension/my-extension
, my-extension
works.
(Note: this looks like an odd behavior and is not documented, so I'm raising this with the Lambda team, but would still be better to make that 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.
good catch! I've updated this code to cleanup the executable name.
Hey @calavera ! I just found a bug when running a sample Lambda function with a layer, but otherwise everything else works when I changed the full extension path for its basename. Once this is fixed, I'll do another pass, but we should be good to merge. |
Cleanup the path from the executable when it takes the name from arg[0]. Signed-off-by: David Calavera <[email protected]>
@nmoutschen I pushed a new commit that should fixed that problem 🙌 |
All good now! Merged the PR! Thank again for your contribution here! 🙌 |
Issue #, if available:
Fixes #261
Description of changes:
This PR introduces two new creates into this project to be able to implement Lambda Extensions in Rust. The work is not completed, and I wanted to share it with you all now that I have functional examples to get people's feedback on the direction I'm taking.
The first crate is
lambda-runtime-client
. I've extracted this crate from the current runtime. The idea with this is to be able to reuse clients for different APIs. This brings some guarantees about client behaviors, like having a User-Agent header by default.The second crate is
lambda-extension
. This crate implements the Lambda Extensions API, which is the main point of this PR. I've included two examples in this new crate so I can get feedback about the API exposed to developers that want to create extensions in Rust.Basic Example
Example that subscribes to specific events
There are a few things missing to get this PR in a mergeable state, mostly documentation, but I'll appreciate your input sooner rather than later so I can make these changes better.
By submitting this pull request