Crashes by build: http://dbaron.org/mozilla/crashes-by-build
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.
Two quick proposals:
For the Build ID field, provide a drop-down with nightly builds from the last week (or more?).
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).
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.
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...
(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?
Yes, we continuously produce nightlies for all supported branches. (Which means back to 3.0.x currently.)
Created attachment 430380 [details]
Updated mock, with Nightlies as a report rather than a version.
Created attachment 431246 [details] [diff] [review]
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 (
date timestamp without time zone default now(),
CONSTRAINT builds_key UNIQUE (product, version, platform, buildid));
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)
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.
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.
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?
(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.
Created attachment 434710 [details] [diff] [review]
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:
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.
Created attachment 435112 [details] [diff] [review]
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 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.
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.
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...
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 (bin) webapp-php/img/feed-icon16x16.png
Adding (bin) webapp-php/img/feed-icon32x32.png
Transmitting file data .................
Committed revision 1906.
One final commit, the buildsconfig.py.dist file needed to be updated.
Transmitting file data .
Committed revision 1907.