Add support for rebaselining from trybots.

Add support for rebaselining from trybots.

This adds support to rebaseline_server to fetch actual results from
tryjobs run on rietveld issues.

Committed: https://skia.googlesource.com/skia/+/d09ade4b44edc114c94f28f1c9770ba42f1271c8

[bungeman-skia, 2014-11-03 23:22:40]

bungeman@google.com changed reviewers:
+ borenet@google.com, rmistry@google.com, stephana@google.com

[bungeman-skia, 2014-11-03 23:22:41]

This CL depends on https://codereview.chromium.org/697043003/ .

At PS1 this is not really complete, but it does work. Before going over this
again to clean it up some more, please let me know if there is anything which
should be done completely differently.

Larger known issues are that on the server side of this the code should really
have a notion of a 'source' of actuals in order to avoid the code duplication
there. Also, the rietveld json api constants should be named.

[borenet, 2014-11-04 12:47:58]

I defer to Stephan and Ravi.

[rmistry, 2014-11-04 12:54:26]

rmistry@google.com changed required reviewers:
+ stephana@google.com

[rmistry, 2014-11-04 12:54:26]

This is an interesting idea.
I made Stephan a required reviewer. I will comment on Python later in the day.

[rmistry, 2014-11-05 13:39:16]

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:138: non_try_builder = builder
Seems confusing to switch non_try_builder's value between a try_builder and then
to a non_try_builder. How about directly doing:

if not builder.endswith('-Trybot'):
  continue

non_try_builder = builder[:-len('-Trybot')]
...

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:161: logging.warning('generic
exception: ' + traceback.format_exc())
traceback.format_exc() returns None so this will probably print:
generic exception: None

It will be useful to output the complete stacktrace here when something goes
wrong. Maybe collapse all the except sections into:

except Exception:
  logging.exception('Error opening %s', gm_upload_output_url)

logging.exception grabs the exception details including the traceback from the
current frame.

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/server.py
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/server....
gm/rebaseline_server/server.py:398: # Clean out actuals_dir first, in case some
builders have gone away
There is a lot of code duplication here with the section below maybe this will
help:

* Extract 398-401 and 435-438 into a function called clean_actuals_dir
* Extract 407-428 and the same section below into a separate function called
from both places.

[rmistry, 2014-11-05 13:39:25]

rmistry@google.com changed required reviewers:
+ rmistry@google.com

[stephana, 2014-11-05 17:28:19]

On 2014/11/05 13:39:16, rmistry wrote:
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
> File gm/rebaseline_server/download_actuals.py (right):
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
> gm/rebaseline_server/download_actuals.py:138: non_try_builder = builder
> Seems confusing to switch non_try_builder's value between a try_builder and
then
> to a non_try_builder. How about directly doing:
> 
> if not builder.endswith('-Trybot'):
>   continue
> 
> non_try_builder = builder[:-len('-Trybot')]
> ...
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
> gm/rebaseline_server/download_actuals.py:161: logging.warning('generic
> exception: ' + traceback.format_exc())
> traceback.format_exc() returns None so this will probably print:
> generic exception: None
> 
> It will be useful to output the complete stacktrace here when something goes
> wrong. Maybe collapse all the except sections into:
> 
> except Exception:
>   logging.exception('Error opening %s', gm_upload_output_url)
> 
> logging.exception grabs the exception details including the traceback from the
> current frame.
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/server.py
> File gm/rebaseline_server/server.py (right):
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/server....
> gm/rebaseline_server/server.py:398: # Clean out actuals_dir first, in case
some
> builders have gone away
> There is a lot of code duplication here with the section below maybe this will
> help:
> 
> * Extract 398-401 and 435-438 into a function called clean_actuals_dir
> * Extract 407-428 and the same section below into a separate function called
> from both places.

I agree this is a good idea. It also means we need to add it to the new
rebaseline server asap. 
You should only add what you need right now since we are aiming to retire this
code by the end of Q4.

[bungeman-skia, 2014-11-07 22:56:53]

Cleaned it up a bit. Still does the same thing, but a bit cleaner.

As I wrote this I realized one more 'actuals provider' which I've wanted in the
past, which is a 'revision actuals provider' which would work like the 'issue
provider' here, but look at builds for a given revision on the main waterfall.
Not sure how useful it would be, but the idea of 'actuals providers' is nice.

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:138: non_try_builder = builder
On 2014/11/05 13:39:16, rmistry wrote:
> Seems confusing to switch non_try_builder's value between a try_builder and
then
> to a non_try_builder. How about directly doing:
> 
> if not builder.endswith('-Trybot'):
>   continue
> 
> non_try_builder = builder[:-len('-Trybot')]
> ...

Done.

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:161: logging.warning('generic
exception: ' + traceback.format_exc())
On 2014/11/05 13:39:16, rmistry wrote:
> traceback.format_exc() returns None so this will probably print:
> generic exception: None
> 
> It will be useful to output the complete stacktrace here when something goes
> wrong. Maybe collapse all the except sections into:
> 
> except Exception:
>   logging.exception('Error opening %s', gm_upload_output_url)
> 
> logging.exception grabs the exception details including the traceback from the
> current frame.

Done.

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/server.py
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/server....
gm/rebaseline_server/server.py:398: # Clean out actuals_dir first, in case some
builders have gone away
On 2014/11/05 13:39:16, rmistry wrote:
> There is a lot of code duplication here with the section below maybe this will
> help:
> 
> * Extract 398-401 and 435-438 into a function called clean_actuals_dir
> * Extract 407-428 and the same section below into a separate function called
> from both places.

Done.

[stephana, 2014-11-10 13:32:09]

On 2014/11/07 22:56:53, bungeman1 wrote:
> Cleaned it up a bit. Still does the same thing, but a bit cleaner.
> 
> As I wrote this I realized one more 'actuals provider' which I've wanted in
the
> past, which is a 'revision actuals provider' which would work like the 'issue
> provider' here, but look at builds for a given revision on the main waterfall.
> Not sure how useful it would be, but the idea of 'actuals providers' is nice.
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
> File gm/rebaseline_server/download_actuals.py (right):
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
> gm/rebaseline_server/download_actuals.py:138: non_try_builder = builder
> On 2014/11/05 13:39:16, rmistry wrote:
> > Seems confusing to switch non_try_builder's value between a try_builder and
> then
> > to a non_try_builder. How about directly doing:
> > 
> > if not builder.endswith('-Trybot'):
> >   continue
> > 
> > non_try_builder = builder[:-len('-Trybot')]
> > ...
> 
> Done.
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/downloa...
> gm/rebaseline_server/download_actuals.py:161: logging.warning('generic
> exception: ' + traceback.format_exc())
> On 2014/11/05 13:39:16, rmistry wrote:
> > traceback.format_exc() returns None so this will probably print:
> > generic exception: None
> > 
> > It will be useful to output the complete stacktrace here when something goes
> > wrong. Maybe collapse all the except sections into:
> > 
> > except Exception:
> >   logging.exception('Error opening %s', gm_upload_output_url)
> > 
> > logging.exception grabs the exception details including the traceback from
the
> > current frame.
> 
> Done.
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/server.py
> File gm/rebaseline_server/server.py (right):
> 
>
https://codereview.chromium.org/688353003/diff/1/gm/rebaseline_server/server....
> gm/rebaseline_server/server.py:398: # Clean out actuals_dir first, in case
some
> builders have gone away
> On 2014/11/05 13:39:16, rmistry wrote:
> > There is a lot of code duplication here with the section below maybe this
will
> > help:
> > 
> > * Extract 398-401 and 435-438 into a function called clean_actuals_dir
> > * Extract 407-428 and the same section below into a separate function called
> > from both places.
> 
> Done.

LGTM 

Ravi please weight in. 
If this is good enough for what you need right now, we should go ahead and land
it. Then we need to figure out how to incorporate it into the new rebaseline
server ASAP.

[rmistry, 2014-11-11 12:26:02]

LGTM

https://codereview.chromium.org/688353003/diff/40001/gm/rebaseline_server/dow...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/688353003/diff/40001/gm/rebaseline_server/dow...
gm/rebaseline_server/download_actuals.py:211: except httplib.HTTPException, e:
Remove this section (HTTPException) since the log will not tell us anything
useful.

[bungeman-skia, 2014-11-11 15:42:51]

https://codereview.chromium.org/688353003/diff/40001/gm/rebaseline_server/dow...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/688353003/diff/40001/gm/rebaseline_server/dow...
gm/rebaseline_server/download_actuals.py:211: except httplib.HTTPException, e:
On 2014/11/11 12:26:02, rmistry wrote:
> Remove this section (HTTPException) since the log will not tell us anything
> useful.

I wanted to have the 'normal' exceptions report messages, and 'unexpected'
exceptions fully logged but not get in the way of the rest of this working. I've
simplified this quite a bit, and the output of the new code is much nicer
anyway.

[bungeman-skia, 2014-11-11 15:43:14]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2014-11-11 15:43:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/688353003/60001

[commit-bot: I haz the power, 2014-11-11 16:12:02]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/d09ade4b44edc114c94f28f1c9770ba42f1271c8

[bungeman-skia, 2014-11-11 20:33:06]

https://codereview.chromium.org/688353003/diff/60001/gm/rebaseline_server/dow...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/688353003/diff/60001/gm/rebaseline_server/dow...
gm/rebaseline_server/download_actuals.py:130: dirs, _ =
get_builders_list(self._summaries_bucket)
Need to remove the ', _', as this used to call 'list_bucket_contents' directly,
but no longer does. This was fixed with
https://skia.googlesource.com/skia/+/842ab70966a344e8e9bdcb43ae41548c8e0f924b .