Merge lp:~vila/udd/analyze-log-imports into lp:udd

[Vincent Ladeuil]

Parse and report successful imports while parsing the importer log files.

Based on https://code.launchpad.net/~vila/udd/analyze_log/+merge/74056, this patch parses and reports the successful imports found in the log file.

This diverges a slight bit from the initial purpose of the
analyze_log.py purpose. If more divergences appear, it may be
nice to switch to a more general approach
s/report_times(/report('times', or report('imports', / and allow
some command-line or config parameter to decide which parts
should reported but I don't it need for now.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/05/2011 10:08 AM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/udd/analyze-log-imports into lp:udd with lp:~vila/udd/analyze_log as a prerequisite.
>
> Requested reviews:
> Ubuntu Distributed Development Developers (udd)
>
> For more details, see:
> https://code.launchpad.net/~vila/udd/analyze-log-imports/+merge/74057
>
> Based on https://code.launchpad.net/~vila/udd/analyze_log/+merge/74056, this patch parses and reports the successful imports found in the log file.
>
> This diverges a slight bit from the initial purpose of the
> analyze_log.py purpose. If more divergences appear, it may be
> nice to switch to a more general approach
> s/report_times(/report('times', or report('imports', / and allow
> some command-line or config parameter to decide which parts
> should reported but I don't it need for now.
>
>

I'm not sure I understand what you mean by "diverges from the initial
purpose". Could you clarify a bit?

Would you want to add some test case to the matching logic? (They should
be fairly easy to write as unit tests.)

+ # Cough, not really times... cough

^- what are they then? The comment doesn't really add any clarity.

 review: needsinfo

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5l1f4ACgkQJdeBCYSNAAOyFwCfUAnmHFltJWBpDzhRoUXaMflc
yUcAoLS9QEy3P7+OskmNR5j0wILXFNMq
=mUIq
-----END PGP SIGNATURE-----

review: Needs Information

[Vincent Ladeuil (vila)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 09/05/2011 10:08 AM, Vincent Ladeuil wrote:
> > Vincent Ladeuil has proposed merging lp:~vila/udd/analyze-log-imports into
> lp:udd with lp:~vila/udd/analyze_log as a prerequisite.
> >
> > Requested reviews:
> > Ubuntu Distributed Development Developers (udd)
> >
> > For more details, see:
> > https://code.launchpad.net/~vila/udd/analyze-log-imports/+merge/74057
> >
> > Based on https://code.launchpad.net/~vila/udd/analyze_log/+merge/74056, this
> patch parses and reports the successful imports found in the log file.
> >
> > This diverges a slight bit from the initial purpose of the
> > analyze_log.py purpose. If more divergences appear, it may be
> > nice to switch to a more general approach
> > s/report_times(/report('times', or report('imports', / and allow
> > some command-line or config parameter to decide which parts
> > should reported but I don't it need for now.
> >
> >
>
> I'm not sure I understand what you mean by "diverges from the initial
> purpose". Could you clarify a bit?

The initial purpose was to report import times (report_*times*), it now also reports imports and imports != times.

>
> Would you want to add some test case to the matching logic? (They should
> be fairly easy to write as unit tests.)

That's one exception to TDD to me: I don't think automated tests add value here because I have real data to play with, both from the past and more importantly, live data from the running importer. So it's already easy enough to catch errors and fix them.

Regressions doesn't really matter either as we don't want to maintain compatibility with old logs anyway, if these logs needs to be analysed, it's far easier to just retrieve the old version of analyze_log.py.

>
> + # Cough, not really times... cough
>
> ^- what are they then? The comment doesn't really add any clarity.

48 + # Cough, not really times... cough
49 + for distro, release, version in self.versions:
50 + out.write('Imported: %-10s %-12s %s\n' % (distro, release, version))

It displays the distro, the release and the version that was imported, those aren't times.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/06/2011 10:50 AM, Vincent Ladeuil wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 09/05/2011 10:08 AM, Vincent Ladeuil wrote:
>>> Vincent Ladeuil has proposed merging lp:~vila/udd/analyze-log-imports into
>> lp:udd with lp:~vila/udd/analyze_log as a prerequisite.
>>>
>>> Requested reviews:
>>> Ubuntu Distributed Development Developers (udd)
>>>
>>> For more details, see:
>>> https://code.launchpad.net/~vila/udd/analyze-log-imports/+merge/74057
>>>
>>> Based on https://code.launchpad.net/~vila/udd/analyze_log/+merge/74056, this
>> patch parses and reports the successful imports found in the log file.
>>>
>>> This diverges a slight bit from the initial purpose of the
>>> analyze_log.py purpose. If more divergences appear, it may be
>>> nice to switch to a more general approach
>>> s/report_times(/report('times', or report('imports', / and allow
>>> some command-line or config parameter to decide which parts
>>> should reported but I don't it need for now.
>>>
>>>
>>
>> I'm not sure I understand what you mean by "diverges from the initial
>> purpose". Could you clarify a bit?
>
> The initial purpose was to report import times (report_*times*), it now also reports imports and imports != times.
>
>>
>> Would you want to add some test case to the matching logic? (They should
>> be fairly easy to write as unit tests.)
>
> That's one exception to TDD to me: I don't think automated tests add value here because I have real data to play with, both from the past and more importantly, live data from the running importer. So it's already easy enough to catch errors and fix them.
>

You don't know if you are missing things running against the live
importer. (AFAIK.)

> Regressions doesn't really matter either as we don't want to maintain compatibility with old logs anyway, if these logs needs to be analysed, it's far easier to just retrieve the old version of analyze_log.py.
>
>>
>> + # Cough, not really times... cough
>>
>> ^- what are they then? The comment doesn't really add any clarity.
>
> 48 + # Cough, not really times... cough
> 49 + for distro, release, version in self.versions:
> 50 + out.write('Imported: %-10s %-12s %s\n' % (distro, release, version))
>
> It displays the distro, the release and the version that was imported, those aren't times.
>

The comment still doesn't add value, IMO.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5l8AwACgkQJdeBCYSNAANAFgCdGZUPNy6LWYtuNyAhNHUg71SK
8OgAoNWSCGS6mZXt88nkJ7LcRqszJElU
=KGAm
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

<snip/>

    >> That's one exception to TDD to me: I don't think automated tests
    >> add value here because I have real data to play with, both from
    >> the past and more importantly, live data from the running
    >> importer. So it's already easy enough to catch errors and fix
    >> them.
    >>

    > You don't know if you are missing things running against the live
    > importer. (AFAIK.)

Just try it then, you'll know more ;)

I've tested this script for several days on the live importer and also
on some older logs while changing it. It makes a real difference in the
feedback compared to tail -F.

This script is meant to help and the added feature is far from life
critical (even for me who took the time to write it). There are
certainly other areas that need more testing, I'd prefer to focus on
them.

    >>
    >> It displays the distro, the release and the version that was imported, those aren't times.
    >>

    > The comment still doesn't add value, IMO.

I put it there as a reminder that the method is not name doesn't
represent the job done and explained so in more words in the cover
letter.

I'll gladly update this comment if you have a better way to express
that.

[Martin Pool (mbp)]

Perhaps rather than futzing with the comment it's easier to just change it to something like 'report_metrics' now.

If you're not going to do that I would make the comment something like

 # Although this is called report_times, we're abusing it slightly to report non-time but similarly structured data.

If this is not run automatically (is it?) you should add something about it to readme.

[Vincent Ladeuil (vila)]

Comment fixed as proposed.

> If this is not run automatically (is it?) you should add something about it to readme.

No, it's not, it is mainly meant as helping us get a better feeling of what's is going no (or was going on for that matter). I intend to refine it as the needs arise, feel free to tweak to your hearth content.

=== modified file 'analyze_log.py'
--- analyze_log.py	2011-09-05 08:08:23 +0000
+++ analyze_log.py	2011-09-05 08:08:23 +0000
@@ -244,6 +244,27 @@
     def __init__(self, name):
         super(PackageParser, self).__init__(name)
         self.tb = None
+        self.versions = []
+
+    # Match the full list of imports
+    _imported_re = re.compile('Imported \[(.*)\] ')
+    # Match one import
+    _package_imported_re = re.compile('PackageToImport\([^)]+\)')
+
+    def see_success(self, success):
+        if success == 'Nothing new ':
+            # .. here, move along
+            return
+
+        imports = self._imported_re.match(success)
+        if imports is None:
+            return
+
+        for imp in self._package_imported_re.findall(imports.groups()[0]):
+            # Get the interesting bits out of:
+            # 'PackageToImport(mrbayes, 3.1.2-2, ubuntu, oneiric, release, )'
+            _, version, distro, release, _, _ = imp.split(',')
+            self.versions.append((distro, release, version))
 
     def see(self, log, state, stamp, args):
         if state in ('trying', 'starting'):
@@ -251,7 +272,7 @@
         elif state in ('locked', 'finished'):
             self.ends(stamp)
         elif state == 'success':
-            # There is something to grab in args there, distro, pocket, etc
+            self.see_success(args[0])
             self.ends(stamp)
         else:
             assert state == 'failed'
@@ -260,6 +281,13 @@
             # traceback
             self.parse_traceback(log)
 
+    def report_times(self, out, epoch=None, apocalypse=None):
+        super(PackageParser, self).report_times(out, epoch=epoch,
+                                                apocalypse=apocalypse)
+        # Cough, not really times... cough
+        for distro, release, version in self.versions:
+            out.write('Imported: %-10s %-12s %s\n' % (distro, release, version))
+
 
 class Log(object):