Merge lp:~matsubara/launchpad/bug-436640 into lp:launchpad/db-devel

[Diogo Matsubara]

[r=gary][ui=none][bug=436640] add api to generate informational oopses in errorlog.py and flag DisconnectionErrors and UserRequestOops as informational only.

[Diogo Matsubara (matsubara)]

This branch add a new api: handling() to the ErrorReportingUtility which allows OOPS reports to be flagged as informational only. It also updates DisconnectionErrors and UserRequestOops to use said API.

Tests

$ bin/test -u test_errorlog
$ bin/test -u test_publication
$ bin/test -u test_user_requested_oops

Demo and Q/A

1. Open https://staging.launchpad.net/++oops++
2. Grab the OOPS id in the html source code
3. Wait 10 minutes for the oops to be synced from staging to devpad
4. https://lp-oops.canonical.com/oops/?oopsid=$oops_id_from_step_2
5. Check that the OOPS have the flag Informational: True in the left column

[Gary Poster (gary)]

Diogo, this is a great branch! I don't have any suggestions. Thank you!

Gary

review: Approve

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2009-08-06 14:25:28 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2009-10-08 01:11:12 +0000
@@ -132,7 +132,7 @@
     implements(IErrorReport)
 
     def __init__(self, id, type, value, time, pageid, tb_text, username,
-                 url, duration, req_vars, db_statements):
+                 url, duration, req_vars, db_statements, informational):
         self.id = id
         self.type = type
         self.value = value
@@ -146,6 +146,7 @@
         self.db_statements = db_statements
         self.branch_nick = versioninfo.branch_nick
         self.revno  = versioninfo.revno
+        self.informational = informational
 
     def __repr__(self):
         return '<ErrorReport %s %s: %s>' % (self.id, self.type, self.value)
@@ -161,6 +162,7 @@
         fp.write('User: %s\n' % _normalise_whitespace(self.username))
         fp.write('URL: %s\n' % _normalise_whitespace(self.url))
         fp.write('Duration: %s\n' % self.duration)
+        fp.write('Informational: %s\n' % self.informational)
         fp.write('\n')
         safe_chars = ';/\\?:@&+$, ()*!'
         for key, value in self.req_vars:
@@ -184,6 +186,7 @@
         username = msg.getheader('user')
         url = msg.getheader('url')
         duration = int(float(msg.getheader('duration', '-1')))
+        informational = msg.getheader('informational')
 
         # Explicitely use an iterator so we can process the file
         # sequentially. In most instances the iterator will actually
@@ -217,7 +220,8 @@
         tb_text = ''.join(lines)
 
         return cls(id, exc_type, exc_value, date, pageid, tb_text,
-                   username, url, duration, req_vars, statements)
+                   username, url, duration, req_vars, statements,
+                   informational)
 
 
 class ErrorReportingUtility:
@@ -396,6 +400,10 @@
 
     def raising(self, info, request=None, now=None):
         """See IErrorReportingUtility.raising()"""
+        self._raising(info, request=request, now=now, informational=False)
+
+    def _raising(self, info, request=None, now=None, informational=False):
+        """Private method used by raising() and handling()."""
         if now is not None:
             now = now.astimezone(UTC)
         else:
@@ -483,7 +491,8 @@
 
             entry = ErrorReport(oopsid, strtype, strv, now, pageid, tb_text,
                                 username, strurl, duration,
-                                req_vars, statements)
+                                req_vars, statements,
+                                informational)
             entry.write(open(filename, 'wb'))
 
             if request:
@@ -495,6 +504,10 @@
         finally:
             info = None
 
+    def handling(self, info, request=None, now=None):
+        """Flag ErrorReport as informational only."""
+        self._raising(info, request=request, now=now, informational=True)
+
     def _do_copy_to_zlog(self, now, strtype, url, info, oopsid):
         distant_past = datetime.datetime(1970, 1, 1, 0, 0, 0, tzinfo=UTC)
         when = _rate_restrict_pool.get(strtype, distant_past)
@@ -618,7 +631,7 @@
         request.oopsid is not None or
         not request.annotations.get(LAZR_OOPS_USER_REQUESTED_KEY, False)):
         return None
-    globalErrorUtility.raising(
+    globalErrorUtility.handling(
         (UserRequestOops, UserRequestOops(), None), request)
     return request.oopsid
 

=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2009-10-01 15:36:20 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2009-10-08 01:11:12 +0000
@@ -484,7 +484,7 @@
         # Log a soft OOPS for DisconnectionErrors as per Bug #373837.
         # We need to do this before we re-raise the exception as a Retry.
         if isinstance(exc_info[1], DisconnectionError):
-            getUtility(IErrorReportingUtility).raising(exc_info, request)
+            getUtility(IErrorReportingUtility).handling(exc_info, request)
 
         def should_retry(exc_info):
             if not retry_allowed:

=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py	2009-07-29 14:28:18 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py	2009-10-08 01:11:12 +0000
@@ -58,7 +58,8 @@
                             [('name1', 'value1'), ('name2', 'value2'),
                              ('name1', 'value3')],
                             [(1, 5, 'store_a', 'SELECT 1'),
-                             (5, 10, 'store_b', 'SELECT 2')])
+                             (5, 10, 'store_b', 'SELECT 2')],
+                            False)
         self.assertEqual(entry.id, 'id')
         self.assertEqual(entry.type, 'exc-type')
         self.assertEqual(entry.value, 'exc-value')
@@ -69,6 +70,7 @@
         self.assertEqual(entry.username, 'username')
         self.assertEqual(entry.url, 'url')
         self.assertEqual(entry.duration, 42)
+        self.assertEqual(entry.informational, False)
         self.assertEqual(len(entry.req_vars), 3)
         self.assertEqual(entry.req_vars[0], ('name1', 'value1'))
         self.assertEqual(entry.req_vars[1], ('name2', 'value2'))
@@ -93,7 +95,7 @@
                              ('HTTP_REFERER', 'http://localhost:9000/'),
                              ('name=foo', 'hello\nworld')],
                             [(1, 5, 'store_a', 'SELECT 1'),
-                             (5, 10,'store_b', 'SELECT\n2')])
+                             (5, 10,'store_b', 'SELECT\n2')], False)
         fp = StringIO.StringIO()
         entry.write(fp)
         self.assertEqual(fp.getvalue(), dedent("""\
@@ -107,6 +109,7 @@
             User: Sample User
             URL: http://localhost:9000/foo
             Duration: 42
+            Informational: False
 
             HTTP_USER_AGENT=Mozilla/5.0
             HTTP_REFERER=http://localhost:9000/
@@ -377,19 +380,20 @@
         self.assertEqual(lines[7], 'User: None\n')
         self.assertEqual(lines[8], 'URL: None\n')
         self.assertEqual(lines[9], 'Duration: -1\n')
-        self.assertEqual(lines[10], '\n')
+        self.assertEqual(lines[10], 'Informational: False\n')
+        self.assertEqual(lines[11], '\n')
 
         # no request vars
-        self.assertEqual(lines[11], '\n')
+        self.assertEqual(lines[12], '\n')
 
         # no database statements
-        self.assertEqual(lines[12], '\n')
+        self.assertEqual(lines[13], '\n')
 
         # traceback
-        self.assertEqual(lines[13], 'Traceback (most recent call last):\n')
+        self.assertEqual(lines[14], 'Traceback (most recent call last):\n')
         #  Module canonical.launchpad.webapp.ftests.test_errorlog, ...
         #    raise ArbitraryException(\'xyz\')
-        self.assertEqual(lines[16], 'ArbitraryException: xyz\n')
+        self.assertEqual(lines[17], 'ArbitraryException: xyz\n')
 
     def test_raising_with_request(self):
         """Test ErrorReportingUtility.raising() with a request"""
@@ -432,6 +436,7 @@
             lines.pop(0), 'User: Login, 42, title, description |\\u25a0|\n')
         self.assertEqual(lines.pop(0), 'URL: http://localhost:9000/foo\n')
         self.assertEqual(lines.pop(0), 'Duration: -1\n')
+        self.assertEqual(lines.pop(0), 'Informational: False\n')
         self.assertEqual(lines.pop(0), '\n')
 
         # request vars
@@ -479,7 +484,7 @@
         errorfile = os.path.join(utility.errordir(now), '01800.T1')
         self.assertTrue(os.path.exists(errorfile))
         lines = open(errorfile, 'r').readlines()
-        self.assertEqual(lines[15], 'xmlrpc args=(1, 2)\n')
+        self.assertEqual(lines[16], 'xmlrpc args=(1, 2)\n')
 
     def test_raising_with_webservice_request(self):
         # Test ErrorReportingUtility.raising() with a WebServiceRequest
@@ -546,22 +551,23 @@
         self.assertEqual(lines[7], 'User: None\n')
         self.assertEqual(lines[8], 'URL: https://launchpad.net/example\n')
         self.assertEqual(lines[9], 'Duration: -1\n')
-        self.assertEqual(lines[10], '\n')
+        self.assertEqual(lines[10], 'Informational: False\n')
+        self.assertEqual(lines[11], '\n')
 
         # request vars
-        self.assertEqual(lines[11], 'name1=value1\n')
-        self.assertEqual(lines[12], 'name1=value3\n')
-        self.assertEqual(lines[13], 'name2=value2\n')
-        self.assertEqual(lines[14], '\n')
-
-        # no database statements
+        self.assertEqual(lines[12], 'name1=value1\n')
+        self.assertEqual(lines[13], 'name1=value3\n')
+        self.assertEqual(lines[14], 'name2=value2\n')
         self.assertEqual(lines[15], '\n')
 
+        # no database statements
+        self.assertEqual(lines[16], '\n')
+
         # traceback
-        self.assertEqual(lines[16], 'Traceback (most recent call last):\n')
+        self.assertEqual(lines[17], 'Traceback (most recent call last):\n')
         #  Module canonical.launchpad.webapp.ftests.test_errorlog, ...
         #    raise ArbitraryException(\'xyz\')
-        self.assertEqual(lines[19], 'ArbitraryException: xyz\n')
+        self.assertEqual(lines[20], 'ArbitraryException: xyz\n')
 
         # verify that the oopsid was set on the request
         self.assertEqual(request.oopsid, 'OOPS-91T1')
@@ -598,20 +604,21 @@
         self.assertEqual(lines[7], 'User: None\n')
         self.assertEqual(lines[8], 'URL: None\n')
         self.assertEqual(lines[9], 'Duration: -1\n')
-        self.assertEqual(lines[10], '\n')
+        self.assertEqual(lines[10], 'Informational: False\n')
+        self.assertEqual(lines[11], '\n')
 
         # no request vars
-        self.assertEqual(lines[11], '\n')
+        self.assertEqual(lines[12], '\n')
 
         # no database statements
-        self.assertEqual(lines[12], '\n')
+        self.assertEqual(lines[13], '\n')
 
         # traceback
-        self.assertEqual(lines[13], 'Traceback (most recent call last):\n')
+        self.assertEqual(lines[14], 'Traceback (most recent call last):\n')
         #  Module canonical.launchpad.webapp.ftests.test_errorlog, ...
         #    raise UnprintableException()
         self.assertEqual(
-            lines[16], 'UnprintableException: <unprintable instance object>\n'
+            lines[17], 'UnprintableException: <unprintable instance object>\n'
             )
 
     def test_raising_unauthorized_without_request(self):
@@ -715,16 +722,57 @@
         self.assertEqual(lines[7], 'User: None\n')
         self.assertEqual(lines[8], 'URL: None\n')
         self.assertEqual(lines[9], 'Duration: -1\n')
-        self.assertEqual(lines[10], '\n')
-
-        # no request vars
-        self.assertEqual(lines[11], '\n')
-
-        # no database statements
-        self.assertEqual(lines[12], '\n')
-
-        # traceback
-        self.assertEqual(''.join(lines[13:17]), exc_tb)
+        self.assertEqual(lines[10], 'Informational: False\n')
+        self.assertEqual(lines[11], '\n')
+
+        # no request vars
+        self.assertEqual(lines[12], '\n')
+
+        # no database statements
+        self.assertEqual(lines[13], '\n')
+
+        # traceback
+        self.assertEqual(''.join(lines[14:18]), exc_tb)
+
+    def test_handling(self):
+        """Test ErrorReportingUtility.handling()."""
+        utility = ErrorReportingUtility()
+        now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
+
+        try:
+            raise ArbitraryException('xyz')
+        except ArbitraryException:
+            utility.handling(sys.exc_info(), now=now)
+
+        errorfile = os.path.join(utility.errordir(now), '01800.T1')
+        self.assertTrue(os.path.exists(errorfile))
+        lines = open(errorfile, 'r').readlines()
+
+        # the header
+        self.assertEqual(lines[0], 'Oops-Id: OOPS-91T1\n')
+        self.assertEqual(lines[1], 'Exception-Type: ArbitraryException\n')
+        self.assertEqual(lines[2], 'Exception-Value: xyz\n')
+        self.assertEqual(lines[3], 'Date: 2006-04-01T00:30:00+00:00\n')
+        self.assertEqual(lines[4], 'Page-Id: \n')
+        self.assertEqual(lines[5], 'Branch: %s\n' % versioninfo.branch_nick)
+        self.assertEqual(lines[6], 'Revision: %s\n'% versioninfo.revno)
+        self.assertEqual(lines[7], 'User: None\n')
+        self.assertEqual(lines[8], 'URL: None\n')
+        self.assertEqual(lines[9], 'Duration: -1\n')
+        self.assertEqual(lines[10], 'Informational: True\n')
+        self.assertEqual(lines[11], '\n')
+
+        # no request vars
+        self.assertEqual(lines[12], '\n')
+
+        # no database statements
+        self.assertEqual(lines[13], '\n')
+
+        # traceback
+        self.assertEqual(lines[14], 'Traceback (most recent call last):\n')
+        #  Module canonical.launchpad.webapp.ftests.test_errorlog, ...
+        #    raise ArbitraryException(\'xyz\')
+        self.assertEqual(lines[17], 'ArbitraryException: xyz\n')
 
 
 class TestSensitiveRequestVariables(unittest.TestCase):

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2009-09-17 14:29:22 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2009-10-08 01:11:12 +0000
@@ -115,6 +115,9 @@
         # Ensure the OOPS mentions the correct exception
         self.assertNotEqual(repr(next_oops).find("DisconnectionError"), -1)
 
+        # Ensure the OOPS is correctly marked as informational only.
+        self.assertEqual(next_oops.informational, 'True')
+
         # Ensure that it is different to the last logged OOPS.
         self.assertNotEqual(repr(last_oops), repr(next_oops))
 

=== modified file 'lib/canonical/launchpad/webapp/tests/test_user_requested_oops.py'
--- lib/canonical/launchpad/webapp/tests/test_user_requested_oops.py	2009-07-22 23:31:29 +0000
+++ lib/canonical/launchpad/webapp/tests/test_user_requested_oops.py	2009-10-08 01:11:12 +0000
@@ -6,6 +6,9 @@
 
 import unittest
 
+from zope.component import getUtility
+from zope.error.interfaces import IErrorReportingUtility
+
 from lazr.restful.utils import get_current_browser_request
 
 from canonical.launchpad.webapp.errorlog import (
@@ -68,6 +71,13 @@
         self.assertIs(context, result)
         self.assertTrue(request.annotations.get(LAZR_OOPS_USER_REQUESTED_KEY))
 
+    def test_user_requested_oops_marked_informational(self):
+        # User requested oopses are flagged as informational only.
+        error_reporting_utility = getUtility(IErrorReportingUtility)
+        last_oops = error_reporting_utility.getLastOopsReport()
+        self.assertEqual(last_oops.type, 'UserRequestOops')
+        self.assertEqual(last_oops.informational, 'True')
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/bugs/doc/checkwatches.txt'
--- lib/lp/bugs/doc/checkwatches.txt	2009-10-05 09:18:38 +0000
+++ lib/lp/bugs/doc/checkwatches.txt	2009-10-08 01:11:12 +0000
@@ -86,6 +86,7 @@
     User: None
     URL: None
     Duration: -1
+    Informational: False
     <BLANKLINE>
     error-explanation=ExternalBugtracker for ... is not known.
 
@@ -157,6 +158,7 @@
     User: None
     URL: http://bugs.example.com
     Duration: -1
+    Informational: False
     <BLANKLINE>
     baseurl=http://bugs.example.com
     bugtracker=example-bugs