Closed heitorlessa closed 3 years ago
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.
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....
....
@heitorlessa i guess we need to get more feedback on what people want.
Yes. I'll primarily wait for @cakepietoast or @am29d
@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:
....
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.
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.
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
.
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
Example for handling all exceptions
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.