feat: Add metrics for using the Lambda authorizer feature by lucashuy · Pull Request #4942 · aws/aws-sam-cli

lucashuy · 2023-03-28T23:29:48Z

Which issue(s) does this change fix?

N/A

Why is this change necessary?

This adds a new event to our event tracker so that we can upload Lambda authorizer invocation events to our telemetry.

How does it address the issue?

Adds a new enum under UsedFeature and makes a call to the event tracking method to add that to the list of events to send once the command finishes execution.

This also adds a new property to the event metric: exception_name.
The exception name property will let us pass in the name of an exception that could have happened when using the feature.

This PR also updates the existing Lambda authorizer invocation tests to mock out the telemetry call and adds another one to specifically test if the event tracker was called with the correct parameters.

Lastly, this PR also now property catches the invalid response exception from the lambda authorizer.

What side effects does this change have?

Event related metrics will now have an exception_type property going forward.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

Add input/output type hints to new functions/methods
Write design document if needed (Do I need to write a design document?)
Write/update unit tests
Write/update integration tests
Write/update functional tests if needed
make pr passes
make update-reproducible-reqs if dependencies were changed
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lucashuy · 2023-04-05T22:02:04Z

Holding off on merging for now, pending further discussion on tracking this event.

…nto lambda_auth_metrics

…ssing

new changes to metrics

mildaniel

Thanks Lucas. Looks good overall and I like the change. Left a few minor questions.

mildaniel · 2023-04-10T21:20:54Z

            "event_value": self.event_value,
            "thread_id": self.thread_id,
            "time_stamp": self.time_stamp,
+            "exception_name": self.exception_name,


I assume this is the structure that gets published to Athena, right? Is it safe to publish None for the exception type and how does it appear in the database when we need to query it?

Yup you're correct that this is what gets published to the database. Publishing None is safe, this gets interpreted as null in the database ([{...., exception_name=null, ....}]), so a query would consist of a null check.

mildaniel · 2023-04-10T21:35:58Z

+        """
+        Get the session ID from click and save it locally.
+        """
+        if not EventTracker._session_id:


I don't see why there would be, but wanted to confirm that there won't be any collisions or race-conditions between retrieving and setting the session_id from click since it was moved from a previously locked state out.

This is a good catch. I can move the function call back to the locked state so that we don't accidentally undo anything that was caught before.

moelasmar

Can you create some task to update the other features that use the event tracking to include the new exception_name idea?

moelasmar · 2023-04-12T00:11:09Z

    @staticmethod
-    def track_event(event_name: str, event_value: str):
+    def track_event(
+        event_name: str, event_value: str, session_id: Optional[str] = None, exception_name: Optional[str] = None


why do we need to send the session_id is there any case that this parameter will be different than the session_id retrieved from the command context ?

Its more so that the session_id from the click context might not actually exist in some cases. The Flask app that gets created, starts on its own thread. Each request that gets made to Flask is processed inside of another thread, which does not have access to the original click context.

moelasmar · 2023-04-12T00:13:34Z

        self._dict_of_routes: Dict[str, Route] = {}
        self.stderr = stderr

+        self._click_session_id = None


I think this session_id should be similar to the default session_id we already use in the Event Tracker. can we remove this part here, and just depend on the default session_id used in the event tracker

Addressed in above comment thread ^^

* chore: Merge from develop into feat/apigw-lambda-auth (#4642) * feat: List Command (#4587) * feat: Add table view and rich table (#3851) * Add table view and rich table * Black reformat * Update reproducible reqs * Comment out table comparison assertions * Make rich table class members protected * feat: Adding base commands and help messages for sam list (#3912) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Added the stack-outputs command implementation and tests (#3947) Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Refactored stack outputs command to the producer mapper consumer design pattern (#3980) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests * Added implementation and tests for the stack-outputs command * Added test skips for integration tests, added unit tests, removed redundant init_client call * commit to retrigger appveyor tests * Commmit to trigger appveyor * Modified client source, made fixes based on comments * Made fixes based on comments * Combined get_stack_info and stack_exists, and modified unit tests * Empty-Commit * Empty-Commit * Empty-Commit * fixed tests based on comments * reformatted file * Refactored stack outputs command to the producer mapper consumer design pattern * Fixed formatting * Moved interfaces, made changes based on comments * Made fixes based on comments * Made fixes based on comments * Empty commit * Made changes based on comments, added new exceptions * Fixed format * Fixed return type declaration * Fixed return type declaration * Changed return type to list * Fixed error Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Local transform and resource collection (#4020) Co-authored-by: Andrew Zhan <zhandr@amazon.com> * feat: Adding cloud resources to sam list resources output (#4056) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests * Added implementation and tests for the stack-outputs command * Added test skips for integration tests, added unit tests, removed redundant init_client call * commit to retrigger appveyor tests * Commmit to trigger appveyor * Modified client source, made fixes based on comments * Made fixes based on comments * Combined get_stack_info and stack_exists, and modified unit tests * Empty-Commit * Empty-Commit * Empty-Commit * fixed tests based on comments * reformatted file * Refactored stack outputs command to the producer mapper consumer design pattern * Fixed formatting * Moved interfaces, made changes based on comments * Made fixes based on comments * Made fixes based on comments * Empty commit * Made changes based on comments, added new exceptions * Fixed format * Fixed return type declaration * Fixed return type declaration * Changed return type to list * Fixed error * Implementation of the local transform and resource collection * Empty-Commit * Added section to avoid unused variable * Refactored common code * Added tests, modified PR * Fixed formatting * Made fixes based on PR comments * Fixed formatting * Fixed typing errors * Reverted typing * Fixed error with typing * Made changes to handling optional params * Fixes to typing errors * Made edits based on comments * Fixed error * Changed return type * Reverted return type due to make pr error * Added change to fix make pr error * Removed translate_utils.py file * Added cloud resources to sam list resources output * Empty commit * modified test format * Modified tests * Modified test Co-authored-by: Andrew Zhan <zhandr@amazon.com> * feat: Adding the sam list testable resources command, tests, and table output format support for all sam list commands (#4081) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests * Added implementation and tests for the stack-outputs command * Added test skips for integration tests, added unit tests, removed redundant init_client call * commit to retrigger appveyor tests * Commmit to trigger appveyor * Modified client source, made fixes based on comments * Made fixes based on comments * Combined get_stack_info and stack_exists, and modified unit tests * Empty-Commit * Empty-Commit * Empty-Commit * fixed tests based on comments * reformatted file * Refactored stack outputs command to the producer mapper consumer design pattern * Fixed formatting * Moved interfaces, made changes based on comments * Made fixes based on comments * Made fixes based on comments * Empty commit * Made changes based on comments, added new exceptions * Fixed format * Fixed return type declaration * Fixed return type declaration * Changed return type to list * Fixed error * Implementation of the local transform and resource collection * Empty-Commit * Added section to avoid unused variable * Refactored common code * Added tests, modified PR * Fixed formatting * Made fixes based on PR comments * Fixed formatting * Fixed typing errors * Reverted typing * Fixed error with typing * Made changes to handling optional params * Fixes to typing errors * Made edits based on comments * Fixed error * Changed return type * Reverted return type due to make pr error * Added change to fix make pr error * Removed translate_utils.py file * Added cloud resources to sam list resources output * Empty commit * modified test format * Modified tests * Modified test * Adding the sam list testable resources command, tests, and table output format support for all sam list commands * Changed table and made changes based on pr comments * Fixed integration test expected outputs * Fixed table heading * Added docstring and re-arranged the testable resources producer to reduce if-elses within a single function * Fixed format Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Renaming the 'testable-resources' command to 'endpoints' (#4116) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests * Added implementation and tests for the stack-outputs command * Added test skips for integration tests, added unit tests, removed redundant init_client call * commit to retrigger appveyor tests * Commmit to trigger appveyor * Modified client source, made fixes based on comments * Made fixes based on comments * Combined get_stack_info and stack_exists, and modified unit tests * Empty-Commit * Empty-Commit * Empty-Commit * fixed tests based on comments * reformatted file * Refactored stack outputs command to the producer mapper consumer design pattern * Fixed formatting * Moved interfaces, made changes based on comments * Made fixes based on comments * Made fixes based on comments * Empty commit * Made changes based on comments, added new exceptions * Fixed format * Fixed return type declaration * Fixed return type declaration * Changed return type to list * Fixed error * Implementation of the local transform and resource collection * Empty-Commit * Added section to avoid unused variable * Refactored common code * Added tests, modified PR * Fixed formatting * Made fixes based on PR comments * Fixed formatting * Fixed typing errors * Reverted typing * Fixed error with typing * Made changes to handling optional params * Fixes to typing errors * Made edits based on comments * Fixed error * Changed return type * Reverted return type due to make pr error * Added change to fix make pr error * Removed translate_utils.py file * Added cloud resources to sam list resources output * Empty commit * modified test format * Modified tests * Modified test * Adding the sam list testable resources command, tests, and table output format support for all sam list commands * Changed table and made changes based on pr comments * Fixed integration test expected outputs * Fixed table heading * Added docstring and re-arranged the testable resources producer to reduce if-elses within a single function * Fixed format * Renamed command from testable resources to endpoints Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Cleanup integration tests * Cleanup tests, address comments * Move boto3 imports, update unit tests * Add comments, use constants for resources * Update unit test mocking type * Add missing parameters * Add additional comments * Fix spelling, minor updates Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: andrewzhan <andrewzhan8@gmail.com> Co-authored-by: Andrew Zhan <zhandr@amazon.com> * feat: Add warning about not providing stack name option (#4624) * fix: Fix failing list tests on Windows (#4623) * fix: Fix failing list tests on Windows * Black reformat * Add event tracking for sam validate --lint metrics (#4612) * Update lint helpand output message * Add event tracking for sam validate --lint metrics * Add unit test for tracking --------- Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * Use safe yaml parse in list producer (#4632) * Revert an integration test change related to permission change revert (#4633) --------- Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: andrewzhan <andrewzhan8@gmail.com> Co-authored-by: Andrew Zhan <zhandr@amazon.com> Co-authored-by: David <114027923+cdavidxu-hub@users.noreply.github.com> Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com> * feat: Collect Lambda authorizers in swagger definition (#4641) * Initial suppport to gather lambda authorizers in swagger * Added more unit tests * Made it clear that tests are Lambda auth related * Added function docstring * Added missed case where identity sources differ depending on API Gateway version * Changed some values to constants * Addressed comments * Added empty check to other security definition check * Updated log messages to change some info to warnings, and no auth info to debug * feat: Collect Lambda Authorizers found in Cloudformation resources (#4668) * Initial suppport to gather lambda authorizers in swagger * Added more unit tests * Made it clear that tests are Lambda auth related * Added function docstring * Added missed case where identity sources differ depending on API Gateway version * Changed some values to constants * Addressed comments * Added empty check to other security definition check * Added collection of Lambda authorizers for CFN resources * Addressed comments by moving validation logic to its own methods * feat: Collect Lambda Authorizers under the Auth property for Serverless resources (#4654) * Initial suppport to gather lambda authorizers in swagger * Added more unit tests * Made it clear that tests are Lambda auth related * Added function docstring * Added missed case where identity sources differ depending on API Gateway version * Added parsing Authorizers inside of Auth properties for Serverless resources * Added unit tests * Added test for the HTTP API extraction method * Changed some values to constants * Changed some variables to constants * Addressed comments * Added empty check to other security definition check * Correctly name identity sources * Addressed some comments * Changed LOGs to debugs to reduce spam --------- Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> * feat: Added identity source validation and removed empty string state (#4683) * Added identity source validation and removed empty string state * make black reformat * Moved identity source validator into validators folder * Fixed linting errors * Run make black * Compile regular expressions * feat: Added identity source validation in request handling (#4762) * Added identity source validation in request handling * Removed call to create flask app * Added context to id validator * Add route type check for operation_name * Convert to dictionary to avoid typing issue * Addressed comments and moved Route class to it's own module * feat: Event construction refactor (#4798) * Refactored LocalApigwService by moving some event generation logic out * Cleaned operation name generation * Addressed comments * feat: Event constructors for authorizers (#4807) * Refactored LocalApigwService by moving some event generation logic out * Cleaned operation name generation * Addressed comments * Added tests * Fixed typing for identity getter functions * feat: Invoke Lambda authorizer (#4840) * Added initial invocation logic for Lambda authorizer * Added response validation * Removed unused method * Added tests * Updated principalId get logic * Changed typing to be correct * Addressed comments * Format * Moved lambda auth invocation to its own method * Addressed comments * feat: Integration testing of local Lambda authorizers in serverless properties (#4872) * Added initial invocation logic for Lambda authorizer * Added response validation * Removed unused method * Added tests * Updated principalId get logic * Changed typing to be correct * Addressed comments * Format * Moved lambda auth invocation to its own method * Addressed comments * Initial integration testing setup and work * Change test class name and add comment to make it more clear that this is a bad test case * make black * feat: Validate headers against validation expression property (#4910) * Added identity validation expression check * Added additional test cases * fix: Fix APIGW V2 context passing (#4916) * Updated context passing logic to consider V2 payloads * Addressed comments by adding checking for API event * feat: Integration testing of Lambda authorizers defined as CFN resources (#4917) * Added identity validation expression check * Updated context passing logic to consider V2 payloads * Initial CFN authorizer resource testing * Added tests to validate template validation * fix: Added missing checks for Swagger parsing of Lambda authorizers and fixed some existing ones (#4938) * Added missing check for simple responses and fixed validation string check * Added missing check for simple responses * Added checks for payload version * make format * Addressed comments * feat: Integration testing of Lambda authorizers defined under the Swagger document (#4939) * Added identity validation expression check * Updated context passing logic to consider V2 payloads * Added missing check for simple responses and fixed validation string check * Initial swagger parsing integration testing * Added missing check for simple responses * Added checks for payload version * make format * Added template validation for swagger * make format * fix: Only print console message if enableSimpleResponses is defined (#4994) * feat: Add usage disclaimer when starting API with authorizers (#4968) * Added disclaimer for authorizer usage and updated log message for undefined authorizers * Populated message * Updated message * Updated message * feat: Add metrics for using the Lambda authorizer feature (#4942) * Added event tracking for using Lambda authorizers * Added and update unit tests * Moved event tracker to after invocation logic and added session ID passing * Updated event tracker to accept exceptions * Added tests * Updated doc string to include exception message * Addressed comments and removed old test * Added exception to __repr__ and __eq__ * chore: Removed old test file (#5006) --------- Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: andrewzhan <andrewzhan8@gmail.com> Co-authored-by: Andrew Zhan <zhandr@amazon.com> Co-authored-by: David <114027923+cdavidxu-hub@users.noreply.github.com> Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>

Added event tracking for using Lambda authorizers

da14e4c

github-actions Bot added pr/internal area/local/invoke sam local invoke command area/local/start-api sam local start-api command area/local/start-invoke labels Mar 28, 2023

Merge branch 'feat/apigw-lambda-auth' into lambda_auth_metrics

b13f995

lucashuy commented Apr 3, 2023

View reviewed changes

Comment thread samcli/local/apigw/local_apigw_service.py Outdated

lucashuy marked this pull request as ready for review April 3, 2023 17:59

lucashuy requested a review from a team as a code owner April 3, 2023 17:59

lucashuy requested review from moelasmar and qingchm April 3, 2023 17:59

Added and update unit tests

6e78f93

lucashuy requested a review from hawflau April 4, 2023 22:31

mildaniel previously approved these changes Apr 4, 2023

View reviewed changes

moelasmar reviewed Apr 5, 2023

View reviewed changes

Comment thread samcli/local/apigw/local_apigw_service.py Outdated

moelasmar previously approved these changes Apr 5, 2023

View reviewed changes

lucashuy added 4 commits April 6, 2023 10:16

Merge branch 'feat/apigw-lambda-auth' of github.com:aws/aws-sam-cli i…

b8fc46a

…nto lambda_auth_metrics

Moved event tracker to after invocation logic and added session ID pa…

bba8d2b

…ssing

Updated event tracker to accept exceptions

03da14d

Added tests

764be6c

lucashuy requested review from mildaniel and moelasmar April 10, 2023 17:55

Updated doc string to include exception message

a25367c

mildaniel reviewed Apr 10, 2023

View reviewed changes

Addressed comments and removed old test

747014c

lucashuy requested a review from mildaniel April 11, 2023 17:51

Merge branch 'feat/apigw-lambda-auth' into lambda_auth_metrics

7e9f151

Merge branch 'feat/apigw-lambda-auth' into lambda_auth_metrics

802bad6

moelasmar reviewed Apr 12, 2023

View reviewed changes

Added exception to __repr__ and __eq__

82e98f9

moelasmar approved these changes Apr 12, 2023

View reviewed changes

mildaniel approved these changes Apr 12, 2023

View reviewed changes

lucashuy merged commit 908f379 into aws:feat/apigw-lambda-auth Apr 12, 2023

Navigation Menu

Uh oh!

feat: Add metrics for using the Lambda authorizer feature - #4942

feat: Add metrics for using the Lambda authorizer feature#4942
lucashuy merged 12 commits into
aws:feat/apigw-lambda-authfrom
lucashuy:lambda_auth_metrics

lucashuy commented Mar 28, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

lucashuy commented Apr 5, 2023

Uh oh!

mildaniel left a comment

Uh oh!

Uh oh!

mildaniel Apr 10, 2023

Uh oh!

lucashuy Apr 10, 2023

Uh oh!

mildaniel Apr 10, 2023

Uh oh!

lucashuy Apr 10, 2023

Uh oh!

moelasmar left a comment

Uh oh!

Uh oh!

moelasmar Apr 12, 2023

Uh oh!

lucashuy Apr 12, 2023

Uh oh!

moelasmar Apr 12, 2023

Uh oh!

lucashuy Apr 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

lucashuy commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Mandatory Checklist

Uh oh!

Uh oh!

Uh oh!

lucashuy commented Apr 5, 2023

Uh oh!

mildaniel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moelasmar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lucashuy commented Mar 28, 2023 •

edited

Loading