Skip to content
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

feat(event_handler): Add new EventHandler for Async Lambda #5799

Draft
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

sinofseven
Copy link

Issue number: #5627

Summary

Add AsyncTriggerResolver.

Changes

Please provide a summary of what's being changed

Added EventHandler for asynchronous Lambda execution.

User experience

Please share what the user experience looks like before and after this change

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

Copy link

boring-cyborg bot commented Dec 27, 2024

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 27, 2024
@boring-cyborg boring-cyborg bot added the tests label Jan 5, 2025
@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 5, 2025
@anafalcao
Copy link
Collaborator

Hi @sinofseven! Thank you so much for opening this PR. Do you need any help here?

@sinofseven
Copy link
Author

sinofseven commented Jan 14, 2025

Hi, @anafalcao!

Thanks very much for your suggestion!

I have a few problems at the moment.
I am hoping to discuss these with you.

  • What must be done in tests other than unit tests
  • Regarding the test case for the unit test.

What must be done in tests other than unit tests

For the EventHandler implementation I am creating, I believe that the event samples provided for unit testing of data_classes are sufficient for testing.

However, the EventHandler test for API Gateway implements E2E testing.
I am wondering if it is necessary to implement E2E tests for the EventHandler I am creating.

Regarding the test case for the unit test

The unit test attempted to check all cases, including correct and incorrect values for all arguments of each Route class, as well as the presence or absence of arguments.

Therefore, in creating the test, I wrote all the patterns of the test case in a Google Spreadsheet.

https://docs.google.com/spreadsheets/d/1eioW7rALlFdkv2Q0y8ERZnMnJuJK3QWoPfhoJMTlqaA/edit?usp=sharing

Following this, I implemented a unit test in tests/unit/event_handler/_async_execution/_routes/test_aws_config_rule.py.
(The other tests were created before we properly managed the test cases.)

Here is where I ran into a problem: I had a very large number of test cases in having many arguments Route class.
(For example, we found that the S3Route class with 9 arguments requires just under 20,000 test cases.)

Therefore, I am wondering how far to describe the test cases.

@github-actions github-actions bot added the feature New feature or functionality label Jan 20, 2025
Copy link
Contributor

⚠️Large PR detected⚠️

Please consider breaking into smaller PRs to avoid significant review delays. Ignore if this PR has naturally grown to this size after reviews.

@leandrodamascena
Copy link
Contributor

leandrodamascena commented Jan 20, 2025

Hi @sinofseven, first of all, sorry for the late reply, but I was super busy these days. I was reading the code and it covers most of the aspects we expect from this utility, such as: routing per key, detail, object, event name and others. I must say we have a SUPER SUPER nice work here, thank you very much for that.

I have a proposal to follow with this PR and please let me know what do you think.

Creating a RFC

I'm going to start an RFC with what this implementation should cover. Powertools is available in 4 different languages and the idea is that this RFC can cover the design of this implementation, features that will be supported and resolvers that we will support in the first version. Don't worry we have people to implement in different languages. I need a week to have a v1 of this RFC and then you can contribute.

Keeping this PR open

I will keep this PR open while we discuss the RFC, which will probably take something like 1 month, because we need to hear from the community, from you, from the other maintainers and we need to agree on the implementation. I will also run local tests with this PR to get more insight into the codebase, changes we need to make and other things.

Integrations

We have a utility called EventSource Data Class, where we map the event that comes from any service to Lambda, we can integrate this utility with this one and create an experience like:

from aws_lambda_powertools.async_resolver import S3Resolver

app = S3Resolver()

@app.resolve_by_name(object_name="powertools.txt")
def resolve_by_name():
    print(f"Event time {app.event_time}") # We can access the event properties

We also have a utility called Parser where we use Pydantic templates to do data parsing and validation against S3 Events. We can think of an idea how to integrate with that.

My personal opinion

While I like the idea of ​​having multiple resolvers like S3, EventBridge, SNS, SES, it might make sense to step back and agree to the following:

1 - Make the base router strong and consistent enough to support many types of resolvers
2 - Implement 2 initial resolvers like S3 and EventBridger, because I think events in this case are simpler to interpret and route
3 - Create tests that can be reused for many resolvers.
4 - As soon as we validate the 2 initial resolvers, we can add more.

I would love to hear your feedback and work together on this RFC to design this utility API and add this feature to Powertools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge event_handlers feature New feature or functionality need-rfc size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tests
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Feature request: Add EventHandler for triggering asynchronous execution
4 participants