-
Notifications
You must be signed in to change notification settings - Fork 41
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
Tweak logging of exceptions to exclude traceback if we have a default factory #119
base: master
Are you sure you want to change the base?
Conversation
I would have gladly updated the tests, but I have no idea of how I'm supposed to test this. |
@dlecocq What's your opinion on also switching from |
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.
Awesome! Thanks for going that extra mile, @PLPeeters ! I don't have merge powers on this repo anymore, but I think the Moz folks will have no issue merging after this approval. @lindseyreno - who at Moz should we ping about reppy
these days?
@PLPeeters - I support toning back the logging in the case of exceptions that are relatively common (like |
@dlecocq Alright, I'll add that in here then. Looking through the reppy exceptions, I figure all of them should be toned down if we have a default factory, given its presence implies we're expecting exceptions to happen? |
Sorry for the delay. Yeah, it seems sane to me that we'd want to tone down the logging for a lot of transient or common exceptions. |
@dlecocq I just generalised it to any |
@dlecocq and @PLPeeters I emailed my old team at Moz so hopefully they'll get to these PRs soon. |
@lindseyreno Did they get back to you by any chance? This repo is starting to look pretty dead, PRs and issues seem to be piling up. If the project is no longer maintained it would be nice to have an official answer stating so, that way it can be forked and maintained elsewhere. |
@PLPeeters They did and they let me know reppy is still on their radar and they'll look at the PRs soon. |
When using a default factory, it means we are expecting timeouts or have at least decided how to handle them. It therefore seems sensible not to log the whole stack trace when timeouts happen in this case, which is what this PR achieves.
Logs for timeouts with a default factory defined then become an info-level log. I suppose a case could be made for using warning and not info, but I do believe info makes the most sense.
I'm also wondering whether this should be extended to include
ConnectionException
; I think it could make sense.