549999 - Make finding new crashes in the latest nightly builds easier

Status: RESOLVED FIXED

(Socorro :: General, task)

Description

[Chris Howse [:chowse]]

Crashes by build: http://dbaron.org/mozilla/crashes-by-build
Discussion: http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/f2cb859bb023e79c

Provide a quick and straightforward way of getting lists of crashes from recent nightly builds. This simplifies identifying issues introduced or fixed by recent patches and filing new bugs for them.

Comment 1

[Chris Howse [:chowse]]

Two quick proposals:

ADVANCED SEARCH:
http://people.mozilla.org/~chowse/drop/socorro/nightlies/01_search.png
For the Build ID field, provide a drop-down with nightly builds from the last week (or more?).

NIGHTLIES REPORT:
http://people.mozilla.org/~chowse/drop/socorro/nightlies/02_page.png
A new report, in the style of Top Crashers, but with UI for selecting recent builds. The referenced builds would always be relative to today (e.g. 1st item is 1-day-old nightly, 2nd is 2-day-old, etc.), so this could be bookmarked for future reference (a la dbaron's page).

Comment 2

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Looking good Chowse...  I like including the "nightlies" link within the navigation bar.

I can integrate this in for milestone 1.6.  Just let me know when you have a finalized view for me to use.  I'll begin taking a look at grabbing the nightly Build IDs that need to be used for this.

Comment 3

[(not currently active) Ted Mielczarek]

In your mockup, I note you have "Nightlies" next to "3.7a3pre". Versions with "pre" in them will always be nightlies. Perhaps we can just make those version links link to the nightly reports? Also, note that we have nightly builds on every branch: 3.7a1pre, 3.6.2pre, 3.1.9pre...

Comment 4

[Chris Howse [:chowse]]

(In reply to comment #3)
> In your mockup, I note you have "Nightlies" next to "3.7a3pre". Versions with
> "pre" in them will always be nightlies. Perhaps we can just make those version
> links link to the nightly reports? Also, note that we have nightly builds on
> every branch: 3.7a1pre, 3.6.2pre, 3.1.9pre...

Maybe Nightlies should be treated as a Report (like Top Crashers, Crashes/User, etc.) rather than a version. Are nightly builds still continuously produced for older versions-- would the date UI make sense for these?

Also: would it be useful to see crashes for nightlies regardless of OS? or for multiple builds?

Comment 5

[(not currently active) Ted Mielczarek]

Yes, we continuously produce nightlies for all supported branches. (Which means back to 3.0.x currently.)

Comment 6

[Chris Howse [:chowse]]

Attachment: Nightlies Report

Updated mock, with Nightlies as a report rather than a version.

Comment 7

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Attachment: Patch 1 for #549999

This is a request for a code review for the 1st part of this implementation - a python script that will be run nightly to populate a new table called "builds".  

This script will pull the .txt files from select directories in http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ to determine build ids and changeset ids.

The builds table is:

CREATE TABLE builds (
product CHAR(20),
version CHAR(20),
platform CHAR(20),
buildid BIGINT,
changeset CHAR(20),
filename VARCHAR(60),
date timestamp without time zone default now(),
CONSTRAINT builds_key UNIQUE (product, version, platform, buildid));

Comment 8

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 431246 [details] [diff] [review]
Patch 1 for #549999

By adding both a cron job and new database table, you've taken on an arduous task.  I'm sorry, but this is going to be  long.

== Script Form ==

To facilitate code reuse and testing, most of this code should exist in a module within the socorro package (as in socorro/cron/build.py).

Each function within the new build.py should have corresponding unit tests in socorro/unittest/cron/testBuilds.py 

The script part should just set up logging and then call the 'main' routine, passing in the configuration.  Use startBugzilla.py as a canonical example. 

== Configuration Handling ==

The parameters 'base_url', 'platforms', 'versions' should be defined within  buildsconfig.py.dist.  Then the 'main' routine should use the asserts to make sure that the required parts are in the configuration.  The configuration manager class allows values to be set at run time via command line.  Since 'platforms' and 'versions' are tuples rather than strings, you'll have have code that can convert a simple string to one of these tuples.  An example:

platforms = cm.Option()
platforms.doc = 'a comma delimited list of platforms'
platforms.default = 'linux-i686,mac,win32'
platforms.fromStringConverter = lambda x: tuple(x.split(','))

== Database  ==

There is no need for the dbConnect as a wrapper around the db.Database object.  Your wrapper appears to just do failure logging, and the Database class already does that on its own.  In your mainline script, just add the logger object to the config like this:

config.logger = logger 

The database object will then use it when you pass the config in.

Each time you call dbConnect, a brand new connection to the database will be established.  You should probably get a connection once and pass it around as a parameter to the functions that need it.  You should also remember to close the connection when you're done to prevent resource leaks.

The table 'builds' needs to have its definition added to socorro/database/schema.py  This allows one central location for maintenance and for bootstraping a new system.  Of course, once you make changes to schema.py, its unit test will need to be altered.

In Postgresql, the canonical data type for strings is 'text'.  All columns in new tables should use that datatype for strings without regard for length.  This advice comes from the PostgreSQL guys that audited our code last year.

== Exception Handling ==

In each case where you use logging to report an exception, you're using 'debug' logging level even if the exception is fatal.  Since the exception is not propagated outward, the context of the error is lost.  In each place where you catch an exception you either need to try to recover or abort the script.  The script, as it is, will try to continue running after logging an exception.  This will probably result in some other exception being thrown later, obscuring the actual location of the problem.  In socorro.lib.util there is a couple functions to help: reportExceptionAndContinue and reportExceptionAndAbort; just pass in your logger and proper debug tracebacks will be generated in the log.  

Any place where there is a commit within a try/except block, there should also be a call to rollback in the error handling code.

== Logging ==

In your logging lines you're often concatenating strings with multiple plus signs.  In Python this is very inefficient because each '+' will result in another complete copy of the whole string.  
 logger.info("Did not find build entries for " + product + " " + version + " " + platform + " " + buildid)
could be
logger.info (“Did not find build entries for %s %s %s %s” % (product, version, platform, bulidid))
Any time you are tempted to use a plus sign more than once for string catenation, you should switch to the % form intead.
But of course, logging is a special case.  Sometimes the level at which a logger is set means that a given log statement may be ignored.  In both your example and my example above, python will create the result string and then pass it on to logger.info, where it might be ignored.  A subtle but significantly more efficient way to do it is like this:
logger.info(Did not find build entries for %s %s %s %s”, product, version, platform, bulidid)
this time we pass all the values into logger.info and let it worry about putting the string together.  If it decides that it's going to ignore the log, it doesn't have the overhead of having concatenated the string.
== Other ==

I agree, beautifulsoup would be a better choice.

Comment 9

[(not currently active) Ted Mielczarek]

Two quick notes:
1) My patch to upload the .txt files didn't land on the 1.9.0 branch (and probably never will, we're ending support there soon)
2) I stole the SGMLParser goop from sayrer's nightly parser script (http://hg.mozilla.org/webtools/nightly/), so I didn't exactly look for the best possible solution there.

Comment 10

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Thanks Lars!  I really appreciate the level of detail of your code review - very, very helpful! I'll start refactoring this morning...

Thanks Ted - Good to know that 1.9.0 is ending support - I'll remove that from the code.  I stole the SGMLParser code from your script :)  It seemed the best solution was Beautiful Soup, but that module wasn't available for use so I went with the quickest solution possible.

Speaking of Beautiful Soup, what's the protocol for recommending and installing a new python module?

Comment 11

[K Lars Lohn [:lars] [:klohn]]

(In reply to comment #10)
> Speaking of Beautiful Soup, what's the protocol for recommending and installing
> a new python module?

There is no formal defined procedure.  When introducing a new dependency (which has been very rare), document it on the googlecode upgrade page.  When submitting the IT request for staging, make sure that you document the new dependency.

Comment 12

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Attachment: PHP Patch 1 for #549999

Attached is the patch for the PHP / UI portion of this bug.

The output more closely resembles dbaron's page, as I didn't want to replicate all of the code for the crash query results pages in the nightly builds page.  Rather, it seemed more appropriate to just provide links to the query results pages for each of these nightly builds, at least for this particular release.  

The link to the nightly builds page is in the navigation bar for each product and product/version.

Also available is an RSS feed that will contain links directly to the crash query for the most recent nightly builds, so that developers can access results from this page outside of the site.

URLs for testing:

http://rsnyder.khan.mozilla.org/reporter/products/Firefox/builds
http://rsnyder.khan.mozilla.org/reporter/products/Firefox/builds?rss=true

http://rsnyder.khan.mozilla.org/reporter/products/Firefox/versions/3.7a4pre/builds
http://rsnyder.khan.mozilla.org/reporter/products/Firefox/versions/3.7a4pre/builds?rss=true

Comment 13

[Austin King [:ozten]]

Comment on attachment 434710 [details] [diff] [review]
PHP Patch 1 for #549999

Great work and fast turn around!

Couple of nits, but you're good to go with this patch.

webapp-php/application/views/common/product_builds.php consider getting rid of echo and multi-line strings. Sprinkle in some <?php ?> instead.

Nice link in the head. We should also put a Feed icon in the page linking to it too.

I'd prefer the rss url was build.rss instead of build?rss=true.

Comment 14

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Attachment: Python Patch 2 for 549999

Lars, attached is the second patch for Python.  I've implemented most of the changes you recommended... The mock objects were greatly helpful in writing the unit tests.  Thanks again for those pointers.

Comment 15

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 435112 [details] [diff] [review]
Python Patch 2 for 549999

My issues are pretty minor.  I'm giving this a '+' and leave it to you to implement my suggestions at your discretion. 

socorro/unittest/cron/testBuilds.py

in the function makeBogusBuilds, you pass in a parameter called 'logger' but never use it.  Replace the 'print' with a logging message instead.  BTW, if an insertion fails, you might as well abort the test run by putting in an assertion error.

scripts/config/buildsconfig.py.dist
the symbol 'base_url' should be an option. As it is written now, the value of base_url is written in stone and cannot be overridden with a command line parameter or environment variable.


I have a number of other issues, but they come from the boiler plate text that you copied from other  tests, so I'm not going to have you start the crusade to clean that stuff up...

Comment 16

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Thanks Lars.  The issues with testBuilds.py and buildsconfig.py.dist have been resolved.

Chris - I'll create another ticket that captures your original mockups for the UI for the builds.  Creating a basic UI was all I had time for this release - the changes that you have in the mockup will require some refactoring on the PHP side to ensure that we're getting code re-use.

==

Adding         scripts/config/buildsconfig.py.dist
Adding         scripts/startBuilds.py
Adding         socorro/cron/builds.py
Sending        socorro/database/schema.py
Adding         socorro/unittest/cron/testBuilds.py
Sending        socorro/unittest/database/testSchema.py
Sending        webapp-php/application/config/routes.php
Sending        webapp-php/application/controllers/products.php
Adding         webapp-php/application/models/build.php
Sending        webapp-php/application/views/common/list_by_signature.php
Adding         webapp-php/application/views/common/product_builds.php
Sending        webapp-php/application/views/layout.php
Adding         webapp-php/application/views/products/product_builds.php
Adding         webapp-php/application/views/products/product_version_builds.php
Sending        webapp-php/css/screen.css
Adding  (bin)  webapp-php/img/feed-icon16x16.png
Adding  (bin)  webapp-php/img/feed-icon32x32.png
Transmitting file data .................
Committed revision 1906.

Comment 17

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

One final commit, the buildsconfig.py.dist file needed to be updated.

==

Sending        config/buildsconfig.py.dist
Transmitting file data .
Committed revision 1907.