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

Add exception stack traces to logging #106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

plbertrand
Copy link
Contributor

Fixes #105

Copy link
Contributor

@HerrMuellerluedenscheid HerrMuellerluedenscheid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@HerrMuellerluedenscheid
Copy link
Contributor

Linter fails. Can you please have a look @plbertrand. Apart from that I think this looks great. Thanks

@plbertrand plbertrand force-pushed the exception-logging branch 2 times, most recently from 1614ac9 to 0e76cd3 Compare April 15, 2022 22:52
except ClientException as ce:
logger.error("Client exception: %s" % ce)
except ClientException:
logger.error("Client exception", exc_info=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.error("Client exception", exc_info=True)
logger.exception("Client exception")

As you suggested in #105 the exception shortcut is nice 👍 This could be an opportunity to move to it.

@FlorianLudwig
Copy link
Member

@plbertrand thanks for your contribution! Do you want to move to .exception from .error? If not I would merge this as is.

For some log statements, I wonder why we should include a traceback at all. For example when the connection aborts this is a network error - not a programming error.

Some log levels also seem wrong to me.

But both are out of scope of this MR. This is an improvement already as is!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add stack traces to logging
3 participants