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

CA-371419: Always log exceptions when responding with 500 Internal Error #4848

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

robhoes
Copy link
Member

@robhoes robhoes commented Nov 15, 2022

By default, logs from the http server are supressed, because it would log far too much. However, it would be valuable to always log the exception that has led to a 500 response, because this error condition is often due to a bug.

Logging for these cases is done under a separate name, which is enabled by default.

Signed-off-by: Rob Hoes rob.hoes@citrix.com

Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

How do the other logs get supressed?

@robhoes
Copy link
Member Author

robhoes commented Nov 16, 2022

@psafont This happens because of the following lines in /etc/xapi.conf, where logging for the http module is disabled:

# Disable logging for the following modules
disable-logging-for = http db_write redo_log api_readonly

By default, logs from the http server are supressed, because it would
log far too much. However, it would be valuable to always log the
exception that has led to a 500 response, because this error condition
is often due to a bug.

Logging for these cases is done under a separate name, which is enabled
by default.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
@@ -39,6 +39,8 @@ module D = Debug.Make (struct let name = "http" end)

open D

module E = Debug.Make (struct let name = "http_internal_errors" end)
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention so far is to use the module name name = __MODULE__

Copy link
Member Author

Choose a reason for hiding this comment

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

Here we explicitly want a different name.

let response_internal_error ?req ?extra s =
let response_internal_error ?req ?extra exc s =
E.error "Responding with 500 Internal Error due to %s" (Printexc.to_string exc) ;
E.log_backtrace () ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be sure that we still have the correct backtrace at this point? We had discussion in the past about lost back traces - obviously we will have some backtrace here but is it the correct one? That's why I would like to capture the backtrace first in an exception handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, but this is a catch-all situation. I guess it's better than nothing at this point.

@robhoes robhoes merged commit 34ec94f into xapi-project:master Nov 16, 2022
@robhoes robhoes deleted the log500 branch November 16, 2022 13:14
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.

3 participants