fix: Use correct log level for fatal crashes [sns-1622] #97
Conversation
This will cause the sentry SDK, if initialized, to pick up the crash and send it to sentry. signals/KeyboardInterrupt shouldn't be caught by `except Exception:`
| @@ -132,7 +132,7 @@ def run(self) -> None: | |||
|
|
|||
| self._shutdown() | |||
| except Exception as error: | |||
| logger.info("Caught %r, shutting down...", error) | |||
| logger.exception("Caught exception, shutting down...") | |||
There was a problem hiding this comment.
We should not need to log this as an exception here as we are raising it again at line 145. What was the issue ?
There was a problem hiding this comment.
for some reason i don't fully understand, sys.excepthook is broken in the child process. i suspect that multiprocessing.Pool overwrites it to propagate the exception to the parent process.
for some other reason, the exception that makes the parent process shut down is also not reported.
now i can choose where i want to instrument: do I want to fix instrumentation in the parent process, where the exception stacktrace is broken and local variables are missing (because multiprocessing is pickling the exception to make it cross process boundaries), or in the child process where i risk losing errors if the consumer crash-loops in the parent process?
our answer for celery integration is that we instrument in the child process because there we get better contextual data.
I think we might want to instrument both processes just to be safe, but it would inherently mean duplicated error reports, unless we remove the raise in the child process
|
@fpacifici I investigated three parts wrt arroyo multiprocessing:
I would suggest we investigate double-sending of errors when it becomes a practical concern. In my experience of writing the celery integration, it is tricky to proactively suppress double-reporting of errors without risking losing some insight into what is going on. |
This will cause the sentry SDK, if initialized, to pick up the crash and
send it to sentry.
signals/KeyboardInterrupt shouldn't be caught by
except Exception: