feat: Implement RFC for layering of runtime

[borchero]

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:

• The public interface hardly changes: user handlers must now be Send and 'static but that's it. Using the top-level run function should result in functionally (almost) equivalent code (except for some logging statements).
• The now-public 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 custom LambdaInvocation type which provides request-specific information along with the Config of the Lambda environment for convenience.
• Some services could probably be implemented a little easier if one would accept that any services wrapped in the runtime implement 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

• I confirm that my contribution is made under the terms of the Apache 2.0 license.
• I confirm that I've made a best effort attempt to update all relevant documentation.

[calavera]

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!

[borchero]

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?

[borchero]

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 😄)

[calavera]

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.

[bnusunny]

I agree. We should avoid such breaking changes for this refactoring effort.

[borchero]

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 😄

[borchero]

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 🫣

[calavera]

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 an async fn call method 🫣

This has been in my wish list for years 😭

[calavera]

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.

[calavera]

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.

[calavera]

This is looking really good. I have a few comments to add a few smaller things, but I've been testing this branch today and I'm very happy with the result so far.

[calavera]

Can you add a src/main.rs file with a lambda function that shows how to use this library?

[borchero]

I added a dummy implementation that logs traces to stdout. Unfortunately, OpenTelemetry setup in Rust isn't very trivial, hence, there are a bunch more dependencies in the Cargo.toml of the example. I attempted to clearly indicate which dependencies are required by the library portion of the example.

[calavera]

thanks!

[calavera]

should the TracingLayer always be included like we do with the other ones? If we leave it here, maybe we need to add some documentation in Runtime::new that indicates that this layer needs to be added when you use the Runtime struct directly to initialize the runtime.

[borchero]

I'd argue that it should not be included by default. If someone was to add their custom tracing layer, this would essentially create duplicate traces. I added a comment to the docstring of Runtime::new.

[calavera]

that's a good point. Thanks!

[borchero]

As you suggested above, the latest commit also bumps the MSRV to 1.70 :)

[calavera]

Thanks. This change looks good to me!

[calavera]

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. 👏

[borchero]

Nice! Thanks for the swift reviews @calavera, happy to see this merged so quickly 😁

[benbpyle]

This is amazing! Thank you @borchero and @calavera for doing this!