Closed borchero closed 5 months ago
Thanks a lot for implementing this, I know it was not an easy implementation. It might takes us a few days to review it to make sure nothing major really breaks, but I'm excited about this improvement!
Once you've had a look at the core changes of this PR: wdyt about including the OpenTelemetryLayer
into the lambda-runtime
package behind a opentelemetry
feature flag?
I'm not entirely sure why the remaining CI is newly failing on this PR, I'll have a look the next couple days (but would appreciate pointers if you have any since I'm not yet familiar with the entirety of the repo :smile:)
The main problem with this change is this:
The public interface hardly changes: user handlers must now be Send and 'static but that's it.
The recommended way to share resources between lambda invocations is by initializing them in the main
function, and then sharing the reference in the handler. Because handlers must be 'static
, all those references don't live long enough, which is going to be a pretty inconvenient breaking change.
If you look at the examples directory, all the examples should work, but a lot of them fail because of this problem. This error for example:
--> producer/src/main.rs:26:27
|
25 | let sqs_manager = SQSManager::new(aws_sdk_sqs::Client::new(&config), queue_url);
| ----------- binding `sqs_manager` declared here
26 | let sqs_manager_ref = &sqs_manager;
| ^^^^^^^^^^^^ borrowed value does not live long enough
...
30 | lambda_runtime::run(service_fn(handler_func_closure)).await?;
| ----------------------------------------------------- argument requires that `sqs_manager` is borrowed for `'static`
31 | Ok(())
32 | }
| - `sqs_manager` dropped here while still borrowed
Is caused because this code doesn't compile anymore: https://github.com/awslabs/aws-lambda-rust-runtime/blob/main/examples/advanced-sqs-multiple-functions-shared-data/producer/src/main.rs#L16-L32
I don't know if there is a way to remove the 'static
requirement. Otherwise, we'll need to find a way to make shared references work in a way that's still idiomatic, and plan some kind of migration for people to understand the changes, since it will break pretty much everyone that uses this runtime.
I agree. We should avoid such breaking changes for this refactoring effort.
Fair enough ๐
while working on this change, I already had a version running that didn't require 'static
but it requires a lot of desugaring (i.e. I don't think you can use BoxService
and BoxFuture
anymore). I didn't think the 'static
requirement was too restrictive since e.g. axum
also requires it.
That being said, I'm not sure if it is possible to get rid of the Send
requirement but let's see how it works out :smile:
Alright, Send
and 'static
bounds are both gone ;) it does look ugly though... if only Rust and tower
would be so far to define the service trait with an async fn call
method ๐ซฃ
Alright, Send and 'static bounds are both gone ;) it does look ugly though
Thanks a lot for being diligent with these changes, we really appreciate it.
if only Rust and
tower
would be so far to define the service trait with anasync fn call
method ๐ซฃ
This has been in my wish list for years ๐ญ
oh woow, all tests are passing, this is very awesome! We need a few days to review the full changes, I think it's looking really great so far.
For what is worth, I'd be ok bumping the MSRV value to something newer, like 1.70. I doubt many people still use a Rust version from Dec 2022.
As you suggested above, the latest commit also bumps the MSRV to 1.70 :)
Thanks again for all this work. I want to add some extra documentation before releasing this major change, but we don't have to wait for that to merge this change. I really appreciate all the effort to make this happen. ๐
Nice! Thanks for the swift reviews @calavera, happy to see this merged so quickly ๐
This is amazing! Thank you @borchero and @calavera for doing this!
Issue #, if available: #747, #691
Description of changes: This PR implements the layering RFC presented by @calavera in #747 with minor modifications to get the implementation running.
Notes about the current implementation:
Send
and'static
but that's it. Using the top-levelrun
function should result in functionally (almost) equivalent code (except for some logging statements).Runtime
type adds a few internal layers upon initialization to (1) catch panics in user handlers, (2) transform user handler responses into Runtime API requests, and (3) send completion/error requests to the Runtime API. Users can add their own layers on top: the service input is a customLambdaInvocation
type which provides request-specific information along with theConfig
of the Lambda environment for convenience.Clone
.I also added an example on how one could implement a custom layer to create OpenTelemetry spans and flush telemetry after each invocation, hence, this PR is also closely related to #691.
Thanks a lot for originally outlining the implementation @calavera, that helped a lot! Please regard this PR as a proposal for implementation, I'm happy to make both minor and major adjustments. I mainly wanted to share this to further #747.
By submitting this pull request