fix(idempotency): Log nested exception message #1813
Conversation
pull-request-size
bot
added
the
size/XXL
|
1 similar comment
|
mploski
force-pushed
the
fix/idempotency-print-nested-exception
branch
from
01b8fa5 to
da141a6
Compare
pull-request-size
bot
added
size/XL
mploski
force-pushed
the
fix/idempotency-print-nested-exception
branch
from
da141a6 to
42acd50
Compare
pull-request-size
bot
added
size/M
| else: | ||
| super().__init__() | ||
|
|
||
|
|
There was a problem hiding this comment.
Hello @mploski! This is a huge improvement indeed. I remember the first time I needed to use the idempotency utility, I got an error due to permissions and spent a lot of time figuring out the reason for the "Failed to save record in progress to idempotency store" message. Creating a specific class to capture and return detailed errors makes life and adoption easier!!
I was wondering if we really need to instantiate the parent Exception class or if we could change this code to directly return the error message and make the code more clean. But it's just an idea to think about, I'm not sure and would just like to hear your side.
class BaseError(Exception):
"""
Base error class that overwrites the way exception and extra information is printed.
See https://github.com/awslabs/aws-lambda-powertools-python/issues/1772
"""
def __init__(self, *args: str):
self.message = args[0] if args else ""
self.args = args
def __str__(self):
"""
Return all arguments formatted or original message
"""
if self.args:
return f"{' - '.join(str(arg) for arg in self.args)}"
return self.messageThere was a problem hiding this comment.
Hey @leandrodamascena , thx for the feedback!
I like your proposal and I refactored my code to follow your suggestion i.e to modify how exception is printed in str dunder method. This approach also has additional benefit that we keep initial attributes ( in args list) in object itself so we can easily see how exception object was initialized during debugging if needed.
As for the super class calling. In theory it is suggested to call it as explained in Python Cookbook book: https://books.google.pl/books?id=f7wGeA71_eUC&pg=PA579&dq=To+illustrate+the+use+of+.args,+consider+this+interactive+session+with+the+built-in+RuntimeError+exception,+and+notice+how+any+number+of+arguments+can+be+used+with+the+raise+statement:&hl=en&sa=X&ved=2ahUKEwinm_Dy0bX8AhWMposKHQNHB3oQ6AF6BAgBEAI#v=onepage&q=To%20illustrate%20the%20use%20of%20.args%2C%20consider%20this%20interactive%20session%20with%20the%20built-in%20RuntimeError%20exception%2C%20and%20notice%20how%20any%20number%20of%20arguments%20can%20be%20used%20with%20the%20raise%20statement%3A&f=false.
I dove deep more to see what happens when we call Exception init method. It seems this behavior changed somewhere in newer python 3 releases and args list initialization was moved to new dunder method so all needed stuff is initialized either way even without calling superclass init method ( as oppose to python 2.x and 3.0 for example) . So I think we can safely omit it.
here is a c code for new method in exception class in python 3 main branch: https://github.com/python/cpython/blob/3.7/Objects/exceptions.c#L47 - you can see that we copy args to instance attribute
and in Python 3.0:
https://github.com/python/cpython/blob/v3.0/Objects/exceptions.c#L49 - you can see that this action is done only in init not in new
Can I ask you to review it once again? :-)
There was a problem hiding this comment.
Hello @mploski! Thank you very much for your feedback and all the links provided, it was very helpful to understand more deeply how the exception is handled and it arguments.
From my side everything is ok and we are ready to merge!
|
I did all the tests with this code and simulated several scenarios and in all I was successful. Really a big improvement this @mploski. I just commented one file and I will use your feedback to learn more about Python :)
|
Codecov ReportBase: 97.58% // Head: 97.59% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1813 +/- ##
========================================
Coverage 97.58% 97.59%
========================================
Files 141 141
Lines 6425 6434 +9
Branches 442 444 +2
========================================
+ Hits 6270 6279 +9
Misses 123 123
Partials 32 32
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| else: | ||
| super().__init__() | ||
|
|
||
|
|
There was a problem hiding this comment.
Hello @mploski! Thank you very much for your feedback and all the links provided, it was very helpful to understand more deeply how the exception is handled and it arguments.
From my side everything is ok and we are ready to merge!
rubenfonseca
changed the title
**Issue number: #1772
Summary
Raise
IdempotencyPersistenceLayerErrorexception with additional context taken from raised downstream exceptionChanges
IdempotencyPersistenceLayerErrorexception
Before this change:
After:
This change is not mandatory. I'm happy to hear your opinion whether we should reformat exception message or not and what format would be the best
User experience
Before then change all downstream exception details was hidden from user, making debugging harder.
Now downstream details are available
For details please follow: #1772
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
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.