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

Uncaught exception handling improvement #969

Closed
dmulter opened this issue Oct 23, 2018 · 6 comments
Closed

Uncaught exception handling improvement #969

dmulter opened this issue Oct 23, 2018 · 6 comments

Comments

@dmulter
Copy link
Contributor

dmulter commented Oct 23, 2018

Currently uncaught exceptions log a DEBUG level message (includes stack trace) and return this in the body of the HTTP error response if DEBUG is enabled. An InternalServerError is returned without any logging if DEBUG is not enabled.

Please consider the following alternate behavior:

  • Always log as ERROR level regardless of the DEBUG level of the app. This makes it much easier to monitor and alert based on these error messages.
  • Return the error plus stack trace via the HTTP response if DEBUG is enabled (existing behavior).
  • Return InternalServerError without stack trace if DEBUG is not enabled (existing behavior).

See this section of code:

if self.debug:

@jamesls
Copy link
Member

jamesls commented Oct 24, 2018

Thanks for the feedback. Just to clarify, given the last two points are existing behavior, the remaining task is to change the log level to ERROR on any uncaught exceptions? I think that seems reasonable to change.

@dmulter
Copy link
Contributor Author

dmulter commented Oct 24, 2018

Yes, just wanted to be very clear how all the other behaviors are great and I wouldn't want to lose them.

@dmulter
Copy link
Contributor Author

dmulter commented Oct 30, 2018

Oops, I guess my reply was actually wrong. I am also suggesting that the ERROR level log message is also generated when DEBUG is not enabled. I went to make the code change and realized my mistake. I'll submit a PR in a second so you can see what I'm thinking. Hopefully this change is also acceptable.

@dmulter
Copy link
Contributor Author

dmulter commented Feb 13, 2019

Here's the code diff to be clear, and hopefully to encourage progress on this issue ;-)

@fquinner
Copy link

fquinner commented Jun 5, 2019

Hi @dmulter - just changed this into a PR for the chalice project. This is a pretty big deal for me for operational monitoring too.

@stealthycoin
Copy link
Contributor

Merged with #1144

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

No branches or pull requests

4 participants