-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
lending.py: Add logging to exceptions #3350
lending.py: Add logging to exceptions #3350
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions about some of the new non-exception logs; otherwise lgtm.
Could you resolve the conflicts on this branch? I tried to but I can't quite figure out what it's supposed to be; it looks like there were some big changes in the test-py3 file. Otherwise code lgtm; I just need to toss on dev to test borrowing 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested borrowing works from homepage; lgtm!
Also tested https://openlibrary.org/account/loans works, and return book works.
Already seeing some errors logged!
|
Note to self (and @mekarpeles ); let's keep on eye on this during deploy to make sure it doesn't explode the size of our log files. |
|
https://github.com/internetarchive/openlibrary/blob/master/openlibrary/core/lending.py#L165-L166 is the problem because |
Were there many of these in the logs? How do we hunt down why this is happening? |
Fix.
openlibrary/core/lending.py
contains many bare exceptions which make it difficult to spot the problems that are causingpytest openlibrary/plugins/openlibrary/tests/test_home.py
to fail on Python 3. This PR logs the exceptions in lending.py usinghttps://docs.python.org/3/library/logging.html#logging.Logger.exception
Technical
Testing
Yes.
Evidence
Stakeholders