sentry-python icon indicating copy to clipboard operation
sentry-python copied to clipboard

SystemExit not capturable

Open MaximeLaurenty opened this issue 1 year ago • 5 comments

How do you use Sentry?

Sentry Saas (sentry.io)

Version

1.39.2

Steps to Reproduce

SystemExit (raised manually or through sys.exit) are not captured by sentry_sdk. We are not using any wsgi framework: sentry is used to monitor script (running directly through python myscript.py)

I found no way to specify that I didn't want them to be ignored after looking at the doc and the codebase. I only found this issue which had them handled in WSGI integration.

I understand some people might not want these to be captured, but not having the option to capture them seems like a bug to me.

We can manually trigger sentry when sys.exit is called in our code, but it'd be quite painful having to patch all potential packages raising sys.exit (like argparse).

minimal working example: Python 3.9.16

import atexit
import sys

import sentry_sdk


def before_send(event, hint):
    print("in before_send")
    return event


sentry_sdk.init(
    dsn="obfuscated",
    attach_stacktrace=True,  # attach stacktrace to logs in addition to errors
    environment="local",
    before_send=before_send,
    # doc: https://docs.sentry.io/platforms/python/configuration/options/
)
atexit.register(lambda: sentry_sdk.flush())  # removes error logs on shutdown


def myfunction():
    exit = 2 # tried 1 & 2

    print(f"running sys.exit({exit})")
    sys.exit(exit)  # not triggering before_send

    # print(f"raise systemexit {exit}")
    # raise SystemExit(exit) # not triggering before_send

    # raise BaseException("test maxime")  # triggers before_send

    raise Exception("test maxime")  # triggers before_send


myfunction()

Expected Result

All the above are captured by sentry (sys.exit(1), sys.exit(2), raise SystemExit(1), raise SystemExit(2))

Actual Result

None are captured by sentry (sys.exit(1), sys.exit(2), raise SystemExit(1), raise SystemExit(2)) :

(py39) ➜  repo git:(master) ✗ python test.py
running sys.exit(2)
(py39) ➜  repo git:(master) ✗ python test.py
running sys.exit(1)
(py39) ➜  repo git:(master) ✗ python test.py
raise systemexit 1
(py39) ➜  repo git:(master) ✗ python test.py
in before_send
Traceback (most recent call last):
(...)
BaseException

MaximeLaurenty avatar Jan 12 '24 10:01 MaximeLaurenty

Hi, thank you for reporting this issue.

It appears the reason that the SDK doesn't capture SystemExit exceptions is because we monitor uncaught exceptions by patching the sys.excepthook function, which the Python interpreter calls whenever any unhandled exception, except for SystemExit, is raised. So, we would likely have to implement an entirely different mechanism to capture SystemExit exemptions.

I will have to confer with the rest of the team to determine whether not capturing SystemExit exceptions is a bug or a deliberate design decision, and I will let you know once I have more information.

However, in the meantime, you should be able to work around this issue pretty easily by manually capturing the SystemExit exceptions. You could modify your minimal working example as follows to get SystemExit exceptions to be captured:

import sentry_sdk

sentry_sdk.init(
    ... 
)

def myfunction():
    ...

try:
    myfunction()
except SystemExit as e:
    sentry_sdk.capture_exception(e)  # Send the captured SystemExit exception to Sentry
    raise

szokeasaurusrex avatar Jan 12 '24 11:01 szokeasaurusrex

Thanks for the quick response. I already thought of capturing SystemExit myself like you suggest, however this isn't suitable for us because of 2 downsides:

  1. we'd need to update all our scripts to wrap all commands in it like
try:
  if __name__ == "__main__":
    do_1()
    do_2()
except SystemExit as e:
    sentry_sdk.capture_exception(e)  # Send the captured SystemExit exception to Sentry
  raise
  1. this would not capture errors during imports. Today we initialize sentry in the init.py of our package so it happens at the very beginning of any script, before any other import ; a try-except will only capture commands in its block ie after the imports (the alternative to wrap the whole content of all our files in a massive try-except would tackle this but seems completely unreasonable to me)

MaximeLaurenty avatar Jan 12 '24 14:01 MaximeLaurenty

Thank you for the clarification. I will see what we can do to support your use case

szokeasaurusrex avatar Jan 12 '24 15:01 szokeasaurusrex

The "best" solution I've thought about so far is to monkeypatch sys.exit itself (and hope very few packages raise SystemExit directly / without passing by sys.exit) after initializing the sdk with something like :

    _exit = sys.exit

    def patch_exit(__status=None) -> NoReturn:
        if __status and int(__status) > 0:
            sentry_sdk.capture_exception(SystemExit("Script exiting")) # to improve to include stack trace
        _exit(__status)

    sys.exit = patch_exit

but I'm pretty reluctant to patch such low-level python functions myself ...

MaximeLaurenty avatar Jan 12 '24 16:01 MaximeLaurenty

@MaximeLaurenty I have also thought of your solution, and of all the solutions I can think of, it appears to be the best.

I also considered whether we could maybe patch the SystemExit class's __init__ method; however, if the exception for whatever reason gets constructed without ever being raised, then there would be false positives in Sentry. So, patching SystemExit.__init__ is probably not a good solution. The ideal solution would be to somehow detect when SystemExit is raised anywhere in the program, but I am not sure if it is possible (at least, I have not found a way to do it).

Unless we can come up with a better idea, we could probably implement your proposed solution as a new integration named SysExitIntegration. The integration should probably be disabled by default, so that the SDK's default behavior remains unchanged, but anyone who would like to monitor sys.exit calls would be able to opt-in by enabling the integration.

szokeasaurusrex avatar Jan 15 '24 09:01 szokeasaurusrex

@MaximeLaurenty, we just merged a PR which will add a SysExitIntegration. It will be included in the next Python SDK release.

szokeasaurusrex avatar Aug 27 '24 13:08 szokeasaurusrex