airbrake / airbrake-python

Airbrake Python
MIT License
51 stars 24 forks source link

Notice.py initialiser should use str in message #79

Open zachgoldstein opened 7 years ago

zachgoldstein commented 7 years ago

From https://github.com/airbrake/airbrake-python/pull/76#discussion_r113081044

I think what I'm really trying to say is, if I did something silly/wrong like pass a string to Notice as the exception argument, then I would expect it to either 1) fail or 2) end up as the error message (or _somewhere) in airbrake.

When passed a string, I think the initialiser should use that in the error message. More generally, we could probably clean up __init__ in notice.py so that it's more clear what it's doing.

sirdodger commented 7 years ago

I have another problem with Notice.init, where it attempts to fetch the last exc_info() to get a traceback, regardless if it is currently in an exception handler or not. The result is that if do something like:

try:
    raise Exception('blah')
except:
    pass

airbrake('Write unrelated error to airbrake')

Then my message gets overwritten with 'blah' and the stack trace points to the handled exception.

It would be more useful if init only searched for exc_info if it was passed an exception object, or otherwise had a way of invoking the string-only code path.

This came to light because gunicorn frequently has a "Resource temporarily unavailable" exception in exc_info, and calling the airbrake library anywhere after that will report it instead of the string error provided.

stavxyz commented 7 years ago

Relevant: https://stackoverflow.com/a/31921513

sirdodger commented 7 years ago

Yes, I've been working around it by wrapping my airbrake invocation when passed a string to convert the string into an exception. The resulting stack trace is useless, but the point was to not have a stack trace at all, so no big loss.

        if isinstance(exception, basestring):
            try:
                raise NotReallyAnException(exception)
            except NotReallyAnException as ex:
                exception = ex

        return airbrake.notify(exception, **extra_vars)
stavxyz commented 7 years ago

@sirdodger

In your example above, I definitely agree that if your message 'Write unrelated error to airbrake' is being overwritten by 'blah' then that's a bug.

regardless if it is currently in an exception handler or not

I think this is actually subjective, or at the very least, differs between python versions. From the stack overflow answer:

In Python 2, sys.exc_info() is cleared when you exit the frame where it was caught. So if you put the try..except in a function, exiting the function clears sys.exc_info(). In Python 3, the information is cleared when you exit the exception handler.

I don't know of any other way to determine whether an exception is currently being handled other than by calling sys.exc_info().

I think the behavior for the airbrake client in this case is worth contemplating for sure... One question I have, is it possible to post an event to airbrake without a stack trace? If that is possible, then I would agree you should be able to push events to airbrake through this client without wondering whether python thinks there is relevant exc_info() on hand. Do you think the default behavior should change, which is not to allow python to decide whether there is "current" exception info? What about a keyword argument on the notify method, such as check_exc_info=True?

sirdodger commented 7 years ago

I believe the stack trace is required, though I have not tested it. This module attempts to enter a stub backtrace if only a string is provided:

https://github.com/airbrake/airbrake-python/blob/eff66c68c51f961dcc6eaccb284ea4c9901b845b/airbrake/notice.py#L66

I am not too worried about suppressing the stack trace. I agree that a flag argument to notify() to suppress it and use the stub instead would be nice, but not all that critical. However, I think that if a custom string is passed to notify(), it should absolutely use that instead of the message in the exception.