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

Feature request: Exception handling for Idempotency utility #10

Closed
heitorlessa opened this issue Mar 24, 2021 · 8 comments
Closed

Feature request: Exception handling for Idempotency utility #10

heitorlessa opened this issue Mar 24, 2021 · 8 comments
Assignees
Projects

Comments

@heitorlessa
Copy link
Contributor

Is your feature request related to a problem? Please describe.

As part of awslabs/aws-lambda-powertools-python#218 RFC, we had a brief discussion about exception handling but didn't implement in the first iteration as we weren't sure.

This is to enable a discussion on this before we make Idempotency G, or to agree on whether we should do this.

Describe the solution you'd like

A mechanism to allow an external function to handle individual or all exceptions raised as part of the Idempotency utility.

At present, customers using API Gateway might want to return a different response to their end customers if an operation is already in progress. This is not currently possible and requires a custom middleware - This brings operational and maintenance complexity.

Haven't put much thought on the UX yet hence why creating this to enable this discussion.

Example for handling a given exception

import os 

from aws_lambda_powertools.utilities.idempotency import (
    DynamoDBPersistenceLayer, IdempotencyConfig, idempotent
)
from aws_lambda_powertools.utilities.idempotency.exceptions import IdempotencyAlreadyInProgressError


persistence_layer = DynamoDBPersistenceLayer(table_name=os.getenv("TABLE_NAME"))
config =  IdempotencyConfig(
    event_key_jmespath="body",
    use_local_cache=True,
    exception_handler=my_custom_exception_handler,
    exceptions_list=[IdempotencyAlreadyInProgressError]
)


def my_custom_exception_handler(event: Dict[str, Any], exception: BaseException):
    ...


@idempotent(config=config, persistence_store=persistence_layer)
def handler(event, context):
    ...

Example for handling all exceptions

import os 

from aws_lambda_powertools.utilities.idempotency import (
    DynamoDBPersistenceLayer, IdempotencyConfig, idempotent
)

persistence_layer = DynamoDBPersistenceLayer(table_name=os.getenv("TABLE_NAME"))
config =  IdempotencyConfig(
    event_key_jmespath="body",
    use_local_cache=True,
    exception_handler=my_custom_exception_handler,
)


def my_custom_exception_handler(event: Dict[str, Any], exception: BaseException):
    ...


@idempotent(config=config, persistence_store=persistence_layer)
def handler(event, context):
    ...

Describe alternatives you've considered

Create a custom middleware using middleware factory utility.

Additional context

I don't think we need to change how we remove idempotency keys from the store when exceptions are raised. This is mostly to give customers an option to handle exceptions as they see fit.

FAQ

Does this include exceptions raised in your own handler, or only for the Idempotent related parts?

I'd vote for Idempotent related only, though I also see the point of catching all of them as it can quickly become confusing. I'm on two minds here - Either building a separate scoped utility to handle exceptions as a whole, or handle Idempotency exceptions only to give customers a chance to provide a non-error response when needed.

I like the former because it allows other utilities like the future Circuit Breaker, integration with Error Tracking systems like Sentry, and a clean intent with batteries included (e.g. retry on X exceptions, etc.).

Would the idempotent library have standard error responses defined for API Gateway proxy requests?

No. This could get out of hand quickly, as the next would be Circuit Breaker, or any other utility we might want to provide.

If it is the result of the handler, then if it does not re-raise the error, does this result then get saved in the idempotent persistence layer?

That's where I think it's tricky and I'd like to not touch the current Idempotency mechanics - It's a complex sequence already, and an additional branch flow could make this harder to maintain and reason.

@michaelbrewer
Copy link
Contributor

I'd vote for Idempotent related only, though I also see the point of catching all of them as it can quickly become confusing.

Yes, it would get confusing and would remind me of old java code:

try {
  try {
    try {
       doSomething();
    } catch (Exception e) {
       // ...
    }
    doSomethingElse();
   } catch (Exception e) {
      // ...
   }
   // etc ..

Coz if i wanted exception handling i would it done in one place.

@heitorlessa
Copy link
Contributor Author

I'm thinking here... one of the ways we also solve these in other utilities is to provide a non-decorator option - be that a stand-alone function, context manager, etc.

What do you think?

try:
    with idempotency(....): 
except....
    ....

@michaelbrewer
Copy link
Contributor

@heitorlessa i guess we need to get more feedback on what people want.

@heitorlessa
Copy link
Contributor Author

Yes. I'll primarily wait for @cakepietoast or @am29d

@michaelbrewer
Copy link
Contributor

@heitorlessa i am not sure if this is needed for 1.14.0 using lambda_handler_decorator works for me

@lambda_handler_decorator(trace_execution=True)
def custom_exception_handler(
    handler: Callable[[Dict, LambdaContext], Dict],
    event: Dict,
    context: LambdaContext,
) -> Dict:
    """Utility to convert errors to api gateway response"""
    try:
        return handler(event, context)
    except IdempotencyAlreadyInProgressError:
        return build_response(400, {"message": "Transaction in progress, please try again later"})
    except IdempotencyKeyError:
        return build_response(400, {"message": "Request is missing an idempotent key"})
    except IdempotencyPersistenceLayerError:
        return build_response(503, {"message": "Failure with idempotency persistent layer"})
    except AuthenticationError:
        return build_response(403, {"message": "Unauthorized"})
    except SchemaValidationError:
        return build_response(400, {"message": "Request Validation Error"})
    except Exception:
        return build_response(503, {"message": "Something went wrong"})


@custom_exception_handler
@tracer.capture_lambda_handler
@idempotent(
    config=IdempotencyConfig(
        event_key_jmespath='[requestContext.authorizer.claims."ds:account", powertools_json(body).orderKey]',
        use_local_cache=True,
        raise_on_no_idempotency_key=True,
    ),
    persistence_store=DynamoDBPersistenceLayer(table_name=IDEMPOTENT_TABLE_NAME),
)
def lambda_handler(event: dict, _) -> dict:
     ....

@am29d
Copy link

am29d commented Apr 16, 2021

I agree on both options tbh: dedicated exception handler and context manager. While it is really useful to have an option for handling all exceptions in one method, I think it might be an overload for simple cases.

@heitorlessa
Copy link
Contributor Author

Thinking a bit more on this... I believe we could change idempotency to offer a standalone function so customers can make any other function within their code idempotent not only the Lambda handler.

This would also be an opportunity to make @idempotent decorator to accept any other function/method.

@heitorlessa heitorlessa transferred this issue from aws-powertools/powertools-lambda-python May 13, 2021
@heitorlessa heitorlessa added this to Ideas in Roadmap Jun 4, 2021
@heitorlessa
Copy link
Contributor Author

We have now launched @idempotency_function which can provide a better mechanism not only for any Python function (beyond Lambda env) but more control for exception handling if you consider the handler not using @idempotent.

Docs: https://awslabs.github.io/aws-lambda-powertools-python/latest/utilities/idempotency/#idempotent_function-decorator

@heitorlessa heitorlessa moved this from Ideas to Shipped in Roadmap Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Roadmap
Shipped
Development

No branches or pull requests

4 participants