← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~lifeless/python-oops/extraction into lp:python-oops

 

Robert Collins has proposed merging lp:~lifeless/python-oops/extraction into lp:python-oops.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lifeless/python-oops/extraction/+merge/71794

Now we start getting interesting functionality - this sets default hooks to handle exceptions, the reporting program, the topic (a more generic pageid), and datestamping.
-- 
https://code.launchpad.net/~lifeless/python-oops/extraction/+merge/71794
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops/extraction into lp:python-oops.
=== modified file 'oops/__init__.py'
--- oops/__init__.py	2011-08-16 06:12:49 +0000
+++ oops/__init__.py	2011-08-17 01:15:24 +0000
@@ -25,7 +25,7 @@
 # established at this point, and setup.py will use a version of next-$(revno).
 # If the releaselevel is 'final', then the tarball will be major.minor.micro.
 # Otherwise it is major.minor.micro~$(revno).
-__version__ = (0, 0, 5, 'beta', 0)
+__version__ = (0, 0, 6, 'beta', 0)
 
 __all__ = [
     'Config'

=== modified file 'oops/config.py'
--- oops/config.py	2011-08-16 06:24:29 +0000
+++ oops/config.py	2011-08-17 01:15:24 +0000
@@ -43,6 +43,9 @@
 the inclusion of binary data in the report, and provides cross-language
 compatibility.
 
+A minimal report can be empty, but this is fairly useless and may even be
+rejected by some repositories.
+
 Some well known keys used by Launchpad in its OOPS reports::
 
 * id: The name of this error report.
@@ -50,6 +53,7 @@
 * value: The value of the exception that occurred.
 * time: The time at which the exception occurred.
 * pageid: The identifier for the template/script that oopsed.
+  [deprecated: This maps to the new topic key instead.]
 * branch_nick: The branch nickname.
 * revno: The revision number of the branch.
 * tb_text: A text version of the traceback.
@@ -59,8 +63,14 @@
 * branch_nick: A name for the branch of code that was running when the report
   was triggered.
 * revno: The revision that the branch was at.
-* Informational: A flag, True if the error wasn't fatal- if it was
-  'informational'.
+* reporter: Describes the program creating the report. For instance you might
+  put the name of the program, or its website - as long as its distint from
+  other reporters.
+* topic: The subject or context for the report. With a command line tool you
+  might put the subcommand here, with a web site you might put the template (as
+  opposed to the url). This is used as a weak correlation hint: reports from the
+  same topic are more likely to have the same cause than reports from different
+  topics.
 """
 
 
@@ -72,6 +82,8 @@
 
 from copy import deepcopy
 
+from createhooks import default_hooks
+
 
 class Config:
     """The configuration for the OOPS system.
@@ -94,7 +106,7 @@
 
     def __init__(self):
         self.filters = []
-        self.on_create = []
+        self.on_create = list(default_hooks)
         self.template = {}
         self.publishers = []
 

=== added file 'oops/createhooks.py'
--- oops/createhooks.py	1970-01-01 00:00:00 +0000
+++ oops/createhooks.py	2011-08-17 01:15:24 +0000
@@ -0,0 +1,123 @@
+# Copyright (c) 2010, 2011, Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Various hooks that can be used to populate OOPS reports.
+
+The default_hooks list contains some innocuous hooks which most reporters will
+want.
+"""
+
+__all__ = [
+    'attach_exc_info',
+    'attach_date',
+    'copy_reporter',
+    'copy_topic',
+    'copy_url',
+    'default_hooks',
+    'safe_unicode',
+    ]
+
+__metaclass__ = type
+
+import datetime
+import traceback
+
+from pytz import utc
+
+# Used to detect missing keys.
+_sentinel = object()
+
+# (Prep for python3 - should be set conditional on version
+strtypes = (basestring,)
+
+
+def _simple_copy(key):
+    """Curry a simple hook that copies a key from context to report."""
+    def copy_key(report, context):
+        value = context.get(key, _sentinel)
+        if value is not _sentinel:
+            report[key] = value
+    copy_key.__doc__ = (
+            "Copy the %s field from context to report, if present." % key)
+    return copy_key
+
+copy_reporter = _simple_copy('reporter')
+copy_topic = _simple_copy('topic')
+copy_url = _simple_copy('url')
+ 
+
+def safe_unicode(obj):
+    """Used to reliably get *a* string for an object.
+
+    This is called on objects like exceptions, where bson won't be able to
+    serialize it, but a representation is needed for the report. It is
+    exposed a convenience for other on_create hook authors.
+    """
+    if isinstance(obj, unicode):
+        return obj
+    # A call to str(obj) could raise anything at all.
+    # We'll ignore these errors, and print something
+    # useful instead, but also log the error.
+    # We disable the pylint warning for the blank except.
+    try:
+        value = unicode(obj)
+    except:
+        value = u'<unprintable %s object>' % (
+            unicode(type(obj).__name__))
+    # Some objects give back bytestrings to __unicode__...
+    if isinstance(value, str):
+        value = value.decode('latin-1')
+    return value
+
+
+def attach_date(report, context):
+    """Set the time key in report to a datetime of now."""
+    report['time'] = datetime.datetime.now(utc)
+
+
+def attach_exc_info(report, context):
+    """Attach exception info to the report.
+
+    This reads the 'exc_info' key from the context and sets the:
+    * type
+    * value
+    * tb_text 
+    keys in the report.
+
+    exc_info must be a tuple, but it can contain either live exception
+    information or simple strings (allowing exceptions that have been
+    serialised and received over the network to be reported).
+    """
+    info = context.get('exc_info')
+    if info is None:
+        return
+    report['type'] = getattr(info[0], '__name__', info[0])
+    report['value'] = safe_unicode(info[1])
+    if isinstance(info[2], strtypes):
+        tb_text = info[2]
+    else:
+        tb_text = u''.join(map(safe_unicode, traceback.format_tb(info[2])))
+    report['tb_text'] = tb_text
+
+
+# hooks that are installed into Config objects by default.
+default_hooks = [
+    attach_exc_info,
+    attach_date,
+    copy_reporter,
+    copy_topic,
+    copy_url,
+    ]

=== modified file 'oops/tests/__init__.py'
--- oops/tests/__init__.py	2011-08-15 06:42:14 +0000
+++ oops/tests/__init__.py	2011-08-17 01:15:24 +0000
@@ -22,6 +22,7 @@
 def test_suite():
     test_mod_names = [
         'config',
+        'createhooks',
         ]
     return TestLoader().loadTestsFromNames(
         ['oops.tests.test_' + name for name in test_mod_names])

=== modified file 'oops/tests/test_config.py'
--- oops/tests/test_config.py	2011-08-16 06:24:29 +0000
+++ oops/tests/test_config.py	2011-08-17 01:15:24 +0000
@@ -24,13 +24,14 @@
 from testtools.matchers import Is
 
 from oops.config import Config
+from oops.createhooks import default_hooks
 
 
 class TestConfig(testtools.TestCase):
 
     def test_init(self):
         config = Config()
-        self.assertEqual([], config.on_create)
+        self.assertEqual(default_hooks, config.on_create)
         self.assertEqual([], config.publishers)
         self.assertEqual({}, config.template)
 
@@ -39,24 +40,28 @@
         def capture(id, report, context):
             calls.append((id, report, context))
         config = Config()
+        config.on_create = []
         config.on_create.append(partial(capture, '1'))
         config.on_create.append(partial(capture, '2'))
-        report = config.create('2')
+        report = config.create(dict(foo='2'))
         self.assertThat(report, Is(calls[0][1]))
         self.assertThat(report, Is(calls[1][1]))
-        self.assertEqual([('1', {}, '2'), ('2', {}, '2')], calls)
+        self.assertEqual(
+                [('1', {}, {'foo':'2'}), ('2', {}, {'foo':'2'})], calls)
 
     def test_on_create_no_context(self):
         calls = []
         def capture(report, context):
             calls.append((report, context))
         config = Config()
+        config.on_create = []
         config.on_create.append(capture)
         report = config.create()
         self.assertEqual([({}, {})], calls)
 
     def test_create_template(self):
         config = Config()
+        config.on_create = []
         config.template['base'] = True
         config.template['list'] = []
         report = config.create()

=== added file 'oops/tests/test_createhooks.py'
--- oops/tests/test_createhooks.py	1970-01-01 00:00:00 +0000
+++ oops/tests/test_createhooks.py	2011-08-17 01:15:24 +0000
@@ -0,0 +1,105 @@
+# Copyright (c) 2010, 2011, Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the oops config module."""
+
+__metaclass__ = type
+
+import datetime
+from functools import partial
+import sys
+
+import testtools
+from testtools.matchers import Is
+
+from oops.createhooks import (
+    attach_exc_info,
+    attach_date,
+    default_hooks,
+    copy_reporter,
+    copy_topic,
+    copy_url,
+    )
+
+
+class TestCreateHooks(testtools.TestCase):
+
+    def test_attach_exc_info_missing(self):
+        report = {}
+        attach_exc_info(report, {})
+        self.assertEqual({}, report)
+
+    def test_attach_exc_info_actual_exception(self):
+        report = {}
+        try:
+            raise ValueError('foo bar')
+        except ValueError:
+            attach_exc_info(report, {'exc_info':sys.exc_info()})
+        self.assertEqual('ValueError', report['type'])
+        self.assertEqual('foo bar', report['value'])
+        self.assertIsInstance(report['tb_text'], basestring)
+
+    def test_attach_date(self):
+        report = {}
+        attach_date(report, {})
+        self.assertIsInstance(report['time'], datetime.datetime)
+
+    def test_attach_exc_info_strings(self):
+        report = {}
+        attach_exc_info(report,
+                {'exc_info':('ValueError', 'foo bar', 'my traceback')})
+        self.assertEqual('ValueError', report['type'])
+        self.assertEqual('foo bar', report['value'])
+        self.assertEqual('my traceback', report['tb_text'])
+
+    def test_attach_exc_info_broken_exception(self):
+        class UnprintableException(Exception):
+            def __str__(self):
+                raise RuntimeError('arrgh')
+            __repr__ = __str__
+        report = {}
+        try:
+            raise UnprintableException('foo bar')
+        except UnprintableException:
+            attach_exc_info(report, {'exc_info':sys.exc_info()})
+        self.assertEqual(
+                '<unprintable UnprintableException object>', report['value'])
+        self.assertIsInstance(report['tb_text'], basestring)
+
+    def test_defaults(self):
+        self.assertEqual([attach_exc_info, attach_date, copy_reporter, copy_topic,
+            copy_url], default_hooks)
+
+    def test_reporter(self):
+        report = {}
+        copy_reporter(report, {})
+        self.assertEqual({}, report)
+        copy_reporter(report, {'reporter': 'foo'})
+        self.assertEqual({'reporter':'foo'}, report)
+
+    def test_topic(self):
+        report = {}
+        copy_topic(report, {})
+        self.assertEqual({}, report)
+        copy_topic(report, {'topic': 'foo'})
+        self.assertEqual({'topic':'foo'}, report)
+
+    def test_url(self):
+        report = {}
+        copy_url(report, {})
+        self.assertEqual({}, report)
+        copy_url(report, {'url': 'foo'})
+        self.assertEqual({'url':'foo'}, report)

=== modified file 'setup.py'
--- setup.py	2011-08-16 06:12:49 +0000
+++ setup.py	2011-08-17 01:15:24 +0000
@@ -23,7 +23,7 @@
         os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
 
 setup(name="oops",
-      version="0.0.5",
+      version="0.0.6",
       description=\
               "OOPS report model and default allocation/[de]serialization.",
       long_description=description,