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

Notice.py initialiser should use str in message #79

Open
zachgoldstein opened this issue Apr 26, 2017 · 5 comments
Open

Notice.py initialiser should use str in message #79

zachgoldstein opened this issue Apr 26, 2017 · 5 comments

Comments

@zachgoldstein
Copy link
Contributor

From #76 (comment)

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
Copy link
Contributor

sirdodger commented Nov 9, 2017

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
Copy link
Contributor

stavxyz commented Nov 9, 2017

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

@sirdodger
Copy link
Contributor

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
Copy link
Contributor

stavxyz commented Nov 15, 2017

@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
Copy link
Contributor

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:

'backtrace': [{'file': 'N/A',

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.

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

3 participants