1126448 - Autophone - improve phonedash graph UI for try builds

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Autophone currently handles try builds as builds from a normal repository and does not distinguish between try builds from different users. See for example

http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/local-blank/norejected/2015-01-20/2015-01-27/notcached/noerrorbars/standarderror/try which contains unrelated try builds from me and glandium.

It is very confusing as to what the graph actually means and requires the user to distinguish their builds from others solely by the revision.

Phonedash should provide users with the ability to display try builds only from specific users as well as buildid ranges.

Comment 1

[Mike Hommey [:glandium]]

(In reply to Bob Clary [:bc:] from comment #0)
> Phonedash should provide users with the ability to display try builds only
> from specific users as well as buildid ranges.

I don't know what you mean by buildid ranges, but one thing that's bothering currently is the use of time as x axis for try builds, especially when you want to compare them, and they all happened in a short time frame.

Comment 2

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> I don't know what you mean by buildid ranges, but one thing that's bothering
> currently is the use of time as x axis for try builds, especially when you
> want to compare them, and they all happened in a short time frame.

FWIW, this is something that AWSY handles with an "evenspacing" argument ; it also has a notion of series so that you can get a graph with only your own builds on it. It's sad that we have at least three different graphing tools, all with their own issues, and each having to address them independently (by three, I'm thinking of graphs.m.o, awsy, and phonegap. There might be others)

Comment 3

[Bob Clary [:bc] (inactive)]

bug 1260824 introduced a new version of phonedash which allows the selection of build ids on the x-axis. This allows you to zoom into a series of closely spaced builds in order to view builds created in a very short time frame.

For example on 2016-05-01 there were about 40 builds in a very short amount of time due to bug 1191356 and bug 1156062.

Looking at the graph for local-nytimes, throbber start first visit:

http://phonedash.mozilla.org/#/2016-05-01/2016-05-02/binning=repo-phonetype&rejected=norejected&errorbars=noerrorbars&errorbartype=standarderror&valuetype=median&local-nytimes=on&throbberstart=on&first=on&mozilla-inbound=on&nexus-4=on&nexus-4-1=on&nexus-4-2=on&nexus-4-5=on&nexus-4-6=on&nexus-5=on&nexus-5-1=on&nexus-5-2=on&nexus-5-3=on&nexus-5-4=on&nexus-6=on&nexus-6-1=on&nexus-6-2=on&nexus-6p=on&nexus-6p-3=on&nexus-6p-4=on&nexus-6p-5=on&nexus-9=on&nexus-9-1=on&nexus-9-2=on&nexus-s=on&nexus-s-3=on&nexus-s-8=on&nexus-s-9=on

They are initially indistinguishable. However if you select the time range around the builds, you can zoom down to see the individual commits. If the tooltip that is displayed when you click on a point gets in the way of selecting another point, they can be dragged out of the way now.

I believe I can use the same approach to dynamically load the repositories used in bug 1260824 to display the patch author's email as part of the repo selection for try builds. The author can be captured from the pulse message which notified us of the build. Capturing and recording the author might have other benefits as well. We would have to modify the jobs database to save the author information locally as well as the phonedash database, but that isn't too difficult.

I imagine initially showing something like the following on the first display of a date range with try builds:

[ ] try
    [ ] [e-mail]
    [ ] [e-mail]

You can then deselect the other repositories and select the try builds for the appropriate email address.

Comment 4

[Bob Clary [:bc] (inactive)]

I have a patch for phonedash and autophone but can no longer test it since autophone depends on the try build's revision actually pointing to the try revision.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: bug-1126448-phonedash-v1.patch

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: bug-1126448-autophone-v1.patch

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: try-ui.png

screenshot

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8756422 [details] [diff] [review]
bug-1126448-phonedash-v1.patch

Review of attachment 8756422 [details] [diff] [review]:
-----------------------------------------------------------------

oh try builds and their authors!

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8756423 [details] [diff] [review]
bug-1126448-autophone-v1.patch

Review of attachment 8756423 [details] [diff] [review]:
-----------------------------------------------------------------

would like to understand a bit more before r+

::: tests/perftest.py
@@ +222,5 @@
> +        author = None
> +        re_try = re.compile('.*/try-builds/([^-]*)-')
> +        match = re_try.match(self.build.url)
> +        if match:
> +            author = match.group(1)

I could be missing something- but why do we need the author for posting data to perfherder?  Is this for phonedash only?

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8756426 [details]
try-ui.png

this is nice- especially the try-bclary branch :)

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 8756423 [details] [diff] [review]
> bug-1126448-autophone-v1.patch
> 
> Review of attachment 8756423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> would like to understand a bit more before r+
> 
> ::: tests/perftest.py
> @@ +222,5 @@
> > +        author = None
> > +        re_try = re.compile('.*/try-builds/([^-]*)-')
> > +        match = re_try.match(self.build.url)
> > +        if match:
> > +            author = match.group(1)
> 
> I could be missing something- but why do we need the author for posting data
> to perfherder?  Is this for phonedash only?

Just phonedash. We need to save it in the database since we lose easy access to the author and will need it in the phonedash app.

Comment 12

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/7e47888602ba5f6a55f0a35ce541c327688bfe01
https://github.com/mozilla/phonedash/commit/a01da92a40dcfc64506c94c871589d629ec66ae4

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: bug-1126448-author-clean-v1.patch

Somehow missed a problem with the server side code when author is null.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8756510 [details] [diff] [review]
bug-1126448-author-clean-v1.patch

Review of attachment 8756510 [details] [diff] [review]:
-----------------------------------------------------------------

::: server/handlers.py
@@ +117,4 @@
>  
>          for key in ('phoneid', 'testname', 'revision', 'author', 'bldtype',
>                      'productname', 'productversion', 'osver', 'machineid'):
> +            if r['data'][key] and not is_clean(r['data'][key]):

can we assume r['data'] exists and there is a 'key' in there?

Comment 15

[Bob Clary [:bc] (inactive)]

Yes. r['data'] is the data sent from publish_results in perftest.py. The keys in the is_clean loop are all included. I will have to make the database modification which will add author as null but this patch should handle that.

Comment 16

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/phonedash/commit/cf8b3cb825822c308dae1088673ea610abe1e34b

Comment 17

[Bob Clary [:bc] (inactive)]

deployed 2016-05-25 23:30