Add exception logging function to syslog module

[ericvsmith]

BPO	8214
Nosy	@pitrou, @ericvsmith, @benjaminp, @merwok, @briancurtin
Files	logexception.diff: Implementation of first function, to log exception information. logexception2.patch: Full implementation for Python 2. logexception3.patch: Patch with changes Jack suggested. logexception4.patch: Test fix suggested by Brian. logexception5.patch: Test fix suggested by Bejnamin.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['extension-modules', 'type-feature'] title = 'Add exception logging function to syslog module' updated_at = user = 'https://github.com/ericvsmith' ``` bugs.python.org fields: ```python activity = actor = 'BreamoreBoy' assignee = 'jafo' closed = False closed_date = None closer = None components = ['Extension Modules'] creation = creator = 'eric.smith' dependencies = [] files = ['17089', '17159', '17174', '17177', '17179'] hgrepos = [] issue_num = 8214 keywords = ['patch', 'needs review'] message_count = 20.0 messages = ['101605', '101668', '101681', '101682', '101683', '101685', '101686', '101687', '104211', '104693', '104746', '104754', '104755', '104756', '104757', '104759', '104761', '104762', '105181', '221669'] nosy_count = 6.0 nosy_names = ['jafo', 'pitrou', 'eric.smith', 'benjamin.peterson', 'eric.araujo', 'brian.curtin'] pr_nums = [] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue8214' versions = ['Python 3.5'] ```

[ericvsmith]

Sean Reifschneider proposed [1] adding the ability to log an exception using the syslog module.

My proposed implementation is along the lines of:

def logexceptions(chain=True):
    import sys
    import traceback
    import syslog

    # Should we chain to the existing sys.excepthook?
    current_hook = sys.excepthook if chain else None

    def syslog_exception(etype, evalue, etb):
        if current_hook:
            current_hook(etype, evalue, etb)
        # The result of traceback.format_exception might contain
        # embedded newlines, so we have the nested loops.
        for line in traceback.format_exception(etype, evalue, etb):
            for line in line.rstrip().split('\n'):
                syslog.syslog(line)
    sys.excepthook = syslog_exception

Although it would need to be written in C to work in the existing syslog module, and of course it would call syslog.syslog directly.

[1] http://mail.python.org/pipermail/python-ideas/2010-March/006927.html

[vstinner]

syslog_exception() should be declared outside logexceptions(), so it's possible to call it directly. Example:

try: ... except Exception: syslog_exception(*sys.exc_info())

[ericvsmith]

Good point. So that makes the implementation more like:

import traceback
import syslog
import sys

def syslog_exception(etype, evalue, etb):
    # The result of traceback.format_exception might contain
    # embedded newlines, so we have the nested loops.
    for line in traceback.format_exception(etype, evalue, etb):
        for line in line.rstrip().split('\n'):
            syslog.syslog(line)

def logexceptions(chain=True):
    # Should we chain to the existing sys.excepthook?
    current_hook = sys.excepthook if chain else None

    def hook(etype, evalue, etb):
        if current_hook:
            current_hook(etype, evalue, etb)
        syslog_exception(etype, evalue, etb)
    sys.excepthook = hook

We could then make syslog_exception take a tuple instead of the 3 values. I'm not sure which is more convenient, or more widely used in other APIs.

Once it's moved into syslog, maybe syslog_exception named syslog.log_exception.

[vstinner]

Once it's moved into syslog, maybe syslog_exception named syslog.log_exception.

In that case, how would be called the second function? Can write a patch with an unit test?

[ericvsmith]

> Once it's moved into syslog, maybe syslog_exception named syslog.log_exception.

That's "should be named", of course. Although on second thought maybe syslog.syslog_exception really is the right name, to mirror syslog.syslog.

In that case, how would be called the second function? Can write a patch with an unit test?

I'm not sure which second function you mean.

As far as the implementation, Sean's working on it, although I'm willing to help.

[vstinner]

I'm not sure which second function you mean.

"logexceptions" which replaces sys.excepthook. "log_exception" and "log_exceptions" are very close.

cgitb has a function "enable" which sets sys.excepthook.

[ericvsmith]

Ah, I see. Yes, logexceptions needs a better name. Maybe enable_exception_logging.

[ericvsmith]

Here's a version that would be more analogous to what the C implementation would look like. It uses a class instead of a closure to capture the "chain" value. The 2 exposed functions syslog_exception and enable_exception_logging are the new APIs in this proposal, which would be written in C as part of the syslog module. The class is a hidden implementation detail.

########################
import traceback
import syslog
import sys

def syslog_exception(etype, evalue, etb):
    # The result of traceback.format_exception might contain
    # embedded newlines, so we have the nested loops.
    for line in traceback.format_exception(etype, evalue, etb):
        for line in line.rstrip().split('\n'):
            syslog.syslog(line)

class _Hook(object):
    def __init__(self, chain):
        # Should we chain to the existing sys.excepthook?
        self._current_hook = sys.excepthook if chain else None

    def _hook(self, etype, evalue, etb):
        if self._current_hook:
            self._current_hook(etype, evalue, etb)
        syslog_exception(etype, evalue, etb)

def enable_exception_logging(chain=True):
    sys.excepthook = _Hook(chain)._hook

[eacf80b2-9081-4882-a7f5-27e9644c3853]

I believe I have the first function implemented. See the attached patch for that code and feel free to review.

[eacf80b2-9081-4882-a7f5-27e9644c3853]

I have completed the exception handling code as prototyped in msg101687. Please review.

[eacf80b2-9081-4882-a7f5-27e9644c3853]

Jack Diederich commented:

I don't have my tracker login on this computer so I'll post here. I'd +1 on making the module python with just the core functionality imported from C (it releases the GIL when doing IO). Then you could replace the few hundred lines of C with just the few lines of python from the prototype. That said... The parens in "return (NULL)" are extra and against PEP-7 (though there are already a bunch in syslogmodule.c). You need to NULL check the saved_hook in newhookobject() before INCREF'ing it. Should the saved hook be called after the syslog call? It might do anything. The patch needs unit tests.

[pitrou]

Rewriting another implementation of traceback formatting in C is bad. Just import the "traceback" module and call the desired function in that module.

[pitrou]

Oh, well, sorry. That's what you are already doing. Forget me :)

[eacf80b2-9081-4882-a7f5-27e9644c3853]

Thanks for the review Jack.

I was very tempted to split it into C and Python components, but I decided against it because it's so close to the 2.7 release. I think it would be best to defer that for the Python 3 release, because of potential packaging issues. I'm open to discussion on that though.

I've changed all the return(NULL)s in the package.

NULL check on saved_hook is done.

I also had the same thought about the saved_hook/syslog ordering, so I've changed it.

I've added soe unit tests. I tried getting fancy and testing the exception handling, but had to fork to do it and then unittests were still catching the exception, so I just left it the minimal set of tests I put in there.

Thanks.

[briancurtin]

The test file should use test_support.import_module("syslog") instead of the try/except, which would then make the skip decorators unnecessary.

[eacf80b2-9081-4882-a7f5-27e9644c3853]

Changed to use import_module.

[benjaminp]

You shouln't use fail*. They're deprecated.

[eacf80b2-9081-4882-a7f5-27e9644c3853]

Switched to using assertTrue/assertFalse.

[eacf80b2-9081-4882-a7f5-27e9644c3853]

This won't go into 2.7.0. I will do a patch for 3.x, and wait for 2.7.0 to see if it can go into 2.7.1.

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

@Sean is this something you can pick up again, it seems a shame to let your past efforts gather dust?