Merge lp:~matsubara/python-oops-datedir-repo/improve-db-statement-matches into lp:python-oops-datedir-repo

[Diogo Matsubara]

This branch fixes a couple of small things:

- improves how request variables and db statements are identified by the parser. Before, it'd allow db statements to be read as if they were request variables. The check for request variables is now a bit more precise.

- Duration got from oops report file is now parsed as a float rather than as an int.

- The first traceback line is preserved avoiding a traceback without a newline character in its first line.

[Abel Deuring (adeuring)]

Hi Diogo,

the branch looks good.

Just a minor suggestion:

> === modified file 'oops_datedir_repo/serializer_rfc822.py'
> --- oops_datedir_repo/serializer_rfc822.py 2011-08-25 20:53:24 +0000
> +++ oops_datedir_repo/serializer_rfc822.py 2011-08-26 14:17:14 +0000
> @@ -70,7 +70,7 @@
> exc_value = msg.getheader('exception-value')
> datestr = msg.getheader('date')
> if datestr is not None:
> - date =iso8601.parse_date(msg.getheader('date'))
> + date = iso8601.parse_date(msg.getheader('date'))
> else:
> date = None
> topic = msg.getheader('topic')
> @@ -79,9 +79,9 @@
> username = msg.getheader('user')
> url = msg.getheader('url')
> try:
> - duration = int(float(msg.getheader('duration', '-1')))
> + duration = float(msg.getheader('duration', '-1'))
> except ValueError:
> - duration = -1
> + duration = float(-1)
> informational = msg.getheader('informational')
> branch_nick = msg.getheader('branch')
> revno = msg.getheader('revision')
> @@ -94,28 +94,27 @@
> lines = iter(msg.fp)
>
> def is_req_var(line):
> - return "=" in line
> + return "=" in line and not is_statement(line)
>
> + statement_pat = re.compile(r'^(\d+)-(\d+)(?:@([\w-]+))?\s+(.*)')
> def is_statement(line):
> - return re.match(r'^(\d+)-(\d+)(?:@([\w-]+))?\s+(.*)', line)
> + return statement_pat.match(line)

is_statement() returns a match object or None, from statement_pat.match()...

>
> def is_traceback(line):
> - return 'traceback' in line.lower() or '== EXTRA DATA ==' in line
> + return line.lower().startswith('traceback') or line.startswith(
> + '== EXTRA DATA ==')
>
> req_vars = []
> statements = []
> first_tb_line = ''
> for line in lines:
> + first_tb_line = line
> line = line.strip()
> if line == '':
> continue
> else:
> - if is_req_var(line):
> - key, value = line.split('=', 1)
> - req_vars.append((urllib.unquote(key), urllib.unquote(value)))
> - elif is_statement(line):
> - match = re.match(
> - r'^(\d+)-(\d+)(?:@([\w-]+))?\s+(.*)', line)
> + if is_statement(line):
> + match = statement_pat.match(line)
> assert match is not None, (
> "Unable to interpret oops line: %s" % line)

... and here, statement_pat.match() is called again. I think
you don't need is_statement() at all: Something like

     match = statement_pat.match(line)
     if match is not None:
         start, end, db_id, statement = match.groups()

would be "good enough" :)

review: Approve (code)

[lp:~matsubara/python-oops-datedir-repo/improve-db-statement-matches]

11. By Diogo Matsubara on 2011-08-26

review comments

[Diogo Matsubara (matsubara)]

Thanks Abel, applied your suggestions!

=== modified file 'oops_datedir_repo/serializer_rfc822.py'
--- oops_datedir_repo/serializer_rfc822.py	2011-08-25 20:53:24 +0000
+++ oops_datedir_repo/serializer_rfc822.py	2011-08-26 15:01:46 +0000
@@ -70,7 +70,7 @@
     exc_value = msg.getheader('exception-value')
     datestr = msg.getheader('date')
     if datestr is not None:
-        date =iso8601.parse_date(msg.getheader('date'))
+        date = iso8601.parse_date(msg.getheader('date'))
     else:
         date = None
     topic = msg.getheader('topic')
@@ -79,9 +79,9 @@
     username = msg.getheader('user')
     url = msg.getheader('url')
     try:
-        duration = int(float(msg.getheader('duration', '-1')))
+        duration = float(msg.getheader('duration', '-1'))
     except ValueError:
-        duration = -1
+        duration = float(-1)
     informational = msg.getheader('informational')
     branch_nick = msg.getheader('branch')
     revno = msg.getheader('revision')
@@ -93,38 +93,35 @@
     # support iteration.
     lines = iter(msg.fp)
 
+    statement_pat = re.compile(r'^(\d+)-(\d+)(?:@([\w-]+))?\s+(.*)')
+
     def is_req_var(line):
-        return "=" in line
-
-    def is_statement(line):
-        return re.match(r'^(\d+)-(\d+)(?:@([\w-]+))?\s+(.*)', line)
+        return "=" in line and not statement_pat.match(line)
 
     def is_traceback(line):
-        return 'traceback' in line.lower() or '== EXTRA DATA ==' in line
+        return line.lower().startswith('traceback') or line.startswith(
+            '== EXTRA DATA ==')
 
     req_vars = []
     statements = []
     first_tb_line = ''
     for line in lines:
+        first_tb_line = line
         line = line.strip()
         if line == '':
             continue
         else:
-            if is_req_var(line):
-                key, value = line.split('=', 1)
-                req_vars.append((urllib.unquote(key), urllib.unquote(value)))
-            elif is_statement(line):
-                match = re.match(
-                    r'^(\d+)-(\d+)(?:@([\w-]+))?\s+(.*)', line)
-                assert match is not None, (
-                    "Unable to interpret oops line: %s" % line)
+            match = statement_pat.match(line)
+            if match is not None:
                 start, end, db_id, statement = match.groups()
                 if db_id is not None:
                     db_id = intern(db_id)  # This string is repeated lots.
                 statements.append(
                     (int(start), int(end), db_id, statement))
+            elif is_req_var(line):
+                key, value = line.split('=', 1)
+                req_vars.append((urllib.unquote(key), urllib.unquote(value)))
             elif is_traceback(line):
-                first_tb_line = line
                 break
 
     # The rest is traceback.

=== modified file 'oops_datedir_repo/tests/test_serializer_rfc822.py'
--- oops_datedir_repo/tests/test_serializer_rfc822.py	2011-08-25 20:53:24 +0000
+++ oops_datedir_repo/tests/test_serializer_rfc822.py	2011-08-26 15:01:46 +0000
@@ -97,10 +97,11 @@
             HTTP_REFERER=http://localhost:9000/
             name%3Dfoo=hello%0Aworld
 
-            00001-00005@store_a SELECT 1
+            00001-00005@store_a SELECT 1 = 2
             00005-00010@store_b SELECT 2
 
-            traceback-text"""))
+            traceback-text
+                foo/bar"""))
         report = read(fp)
         self.assertEqual(report['id'], 'OOPS-A0001')
         self.assertEqual(report['req_vars'][0], ('HTTP_USER_AGENT',
@@ -109,6 +110,7 @@
                                              'http://localhost:9000/'))
         self.assertEqual(report['req_vars'][2], ('name=foo', 'hello\nworld'))
         self.assertEqual(len(report['db_statements']), 2)
+        self.assertEqual(report['tb_text'], 'traceback-text\n    foo/bar')
 
     def test_read_no_store_id(self):
         """Test ErrorReport.read() for old logs with no store_id."""