Create and display "comparison statistics" to compare two statistics

Create and display "comparison statistics" to compare two statistics 
in one test run.  

R=tonyg

[Randy Smith (Not in Mondays), 2013-09-18 18:49:29]

Tony: Willing to give me an initial, high-level review of this and let me know
areas I should focus on?  It works; the question is whether this is the way it
should work, and what the testing, if any, should look like.

https://x20web.corp.google.com/~rdsmith/results-23431036-1.html is an example of
an output html file.

[tonyg, 2013-09-22 19:06:11]

Sorry for the delay in getting back to you. This looks pretty reasonable to me,
I'm not opposed to it at all. But just wanted to ask a couple of questions so I
can understand what the considerations are:

1. Did you consider doing the comparisons entirely in the HTML results? e.g.
check and two stats and click compare.
2. In the UI, it seems like if the unit is "percent", we always want the bar
chart to display a height of 100%. Right now there is no visual difference
between an 8% and an 80% improvement because they are all scaled.
3. For the non-comparison results, it looks like we lost the bar graph display.
4. The 6 parameters to AddComparison are not readable at the callsite. Is there
a way to make it more understandable? 
5. Regarding testing, I'd like to see a unittest for
buildbot_page_measurement_results. Everything else should be okay.

Dave, would you mind taking a look too?

[Randy Smith (Not in Mondays), 2013-09-25 20:42:50]

On 2013/09/22 19:06:11, tonyg wrote:
> Sorry for the delay in getting back to you. 

Wow, I managed to miss this in my inbox; I'm not sure why.  My turn to apologize
for being late in getting back to you.

> This looks pretty reasonable to me,
> I'm not opposed to it at all. But just wanted to ask a couple of questions so
I
> can understand what the considerations are:
> 
> 1. Did you consider doing the comparisons entirely in the HTML results? e.g.
> check and two stats and click compare.

Hmmm.  I didn't consider that (this was my understanding of the plan you and I
worked out verbally a month or so ago).  That *could* work, but keep in mind
that what I'm attempting to reach for is a situation where I'm doing automatic
comparisons between these comparison stats.  In other words, I want to know
"percentage improved from cold to warm loads" and then compare *that* between a
run with the disk cache and a run with the simple cache (or a run with the
simple cache versus and old run with the simple cache) to figure out how much
the simple cache/newest version of the simple cache has improved/regressed.  

Another way of putting that is that, from my perspective, the percent
improvement from cold to warm runs is *the* key statistic.  I'd rather have it
front and center than hide it.  I know that's not true for everyone running the
page cycler (you may remember I initially created a new measurement, but then
folded it into the page cycler for simplicity), but it probably is true for
everyone using --cold-load-percent, which is what generates the comparison
statistic.

> 2. In the UI, it seems like if the unit is "percent", we always want the bar
> chart to display a height of 100%. Right now there is no visual difference
> between an 8% and an 80% improvement because they are all scaled.

Sounds good; I'll fix that. 

> 3. For the non-comparison results, it looks like we lost the bar graph
display.

Whoops, that's a bug.  I'll fix it. 

> 4. The 6 parameters to AddComparison are not readable at the callsite. Is
there
> a way to make it more understandable? 

Yeah, this is sub-ideal.  I considered a couple of other alternatives (make
everything keyword arguments and throw an exception if I didn't get them, make
the call take three arguments (result, reference, comparison), each of which was
a tuple, make the call take three arguments, each of which is a measurement
name) but none of them struck me as better than just having six arguments. 
Having said that, if you'd like me to take one of those options (or some other
option) I am quite happy to do so; I have no investment in the current signature
:-J.

> 5. Regarding testing, I'd like to see a unittest for
> buildbot_page_measurement_results. Everything else should be okay.

SG; will do.

> Dave, would you mind taking a look too?

[Randy Smith (Not in Mondays), 2013-10-21 20:51:53]

Tony, willing to take another look?  Status:
+ Fixed the loss of the bar graph display (oops--JS data structure warpage)
+ Made default scale for percent 0-100.  Note that clicking on the scale
(turning minZero off) will result in a fully scaled display (from below the
lowest percent to above the highest).
+ Fixed a bug where the page failed to load if there were any errors reported in
it (the comparison statistic was pointing to a stat that didn't exist).

Things I *didn't* do from your list, and am asking for more feedback from you on
if you still want me to do them:
+ Tweak AddComparison API.  As noted above, I couldn't come up with an
alternative I liked better than just having six arguments.  If you want to
specify an alternative (of the ones I listed above or something else) I'm happy
to implement.
+ As described above, I didn't put the comparison functionality in the HTML
results. 

I didn't really end up doing much in buildbot_page_measurement_results.py, so
the unit test I added is pretty simple.  Let me know if you'd like more testing
(there or elsewhere).

[Randy Smith (Not in Mondays), 2013-11-06 22:31:54]

Tony, Dave: Ping?  I just sync'd to LKGR and spawned some try jobs.  Beyond
that, status is as described in my last post.

[tonyg, 2013-11-18 17:37:10]

Nat is about to totally redo the results object in
https://codereview.chromium.org/27486002/ . We'll need to reconcile these.

I recommend you get his buy-in on the results object change first, then I'll
review the HTML output part in a subsequent change.

[Randy Smith (Not in Mondays), 2013-11-18 17:41:00]

On 2013/11/18 17:37:10, tonyg wrote:
> Nat is about to totally redo the results object in
> https://codereview.chromium.org/27486002/ . We'll need to reconcile these.
> 
> I recommend you get his buy-in on the results object change first, then I'll
> review the HTML output part in a subsequent change.

Sounds ... well, I can't quite say "good", but makes sense :-} :-J.  Nat: I
think what probably makes sense is for me to wait until you've landed 27486002,
and then create a CL with just the results object changes in it.  I'm tracking
your CL now.  Let me know if you want to do something different (e.g. skimming
this CL and giving me guidance or high level feedback before I dive into the
details).

[tonyg, 2013-11-18 17:46:30]

On 2013/11/18 17:41:00, rdsmith wrote:
> On 2013/11/18 17:37:10, tonyg wrote:
> > Nat is about to totally redo the results object in
> > https://codereview.chromium.org/27486002/ . We'll need to reconcile these.
> > 
> > I recommend you get his buy-in on the results object change first, then I'll
> > review the HTML output part in a subsequent change.
> 
> Sounds ... well, I can't quite say "good", but makes sense :-} :-J.  Nat: I
> think what probably makes sense is for me to wait until you've landed
27486002,
> and then create a CL with just the results object changes in it.  I'm tracking
> your CL now.  Let me know if you want to do something different (e.g. skimming
> this CL and giving me guidance or high level feedback before I dive into the
> details).

BTW, I realized I was too terse after you've been pinging this review for two
months. Sorry :(

To be honest, I'm not wild about the AddComparison API and have been incorrectly
and unfairly postponing reviewing this because I'm not sure what to suggest
exactly. So since Nat is really trying to get a solid API around results now, I
thought he could suggest something that will fit well.

I was initially hoping that this could be done all in the UI, but it sounds like
you weren't on board with that idea either.

[Randy Smith (Not in Mondays), 2013-11-18 18:13:56]

On 2013/11/18 17:46:30, tonyg wrote:
> On 2013/11/18 17:41:00, rdsmith wrote:
> > On 2013/11/18 17:37:10, tonyg wrote:
> > > Nat is about to totally redo the results object in
> > > https://codereview.chromium.org/27486002/ . We'll need to reconcile these.
> > > 
> > > I recommend you get his buy-in on the results object change first, then
I'll
> > > review the HTML output part in a subsequent change.
> > 
> > Sounds ... well, I can't quite say "good", but makes sense :-} :-J.  Nat: I
> > think what probably makes sense is for me to wait until you've landed
> 27486002,
> > and then create a CL with just the results object changes in it.  I'm
tracking
> > your CL now.  Let me know if you want to do something different (e.g.
skimming
> > this CL and giving me guidance or high level feedback before I dive into the
> > details).
> 
> BTW, I realized I was too terse after you've been pinging this review for two
> months. Sorry :(
> 
> To be honest, I'm not wild about the AddComparison API and have been
incorrectly
> and unfairly postponing reviewing this because I'm not sure what to suggest
> exactly. So since Nat is really trying to get a solid API around results now,
I
> thought he could suggest something that will fit well.
> 
> I was initially hoping that this could be done all in the UI, but it sounds
like
> you weren't on board with that idea either.

So I had thought that we had discussed the various options in a VC meeting some
time ago, and this was the architecture we had decided on.  I'm willing to do a
different architecture (sigh), but I'd like it to address the concerns we
discussed in that meeting.  Shall you, I, and Nat sit down again and try and
figure out how to do this right?

[tonyg, 2013-11-18 18:22:27]

On 2013/11/18 18:13:56, rdsmith wrote:
> On 2013/11/18 17:46:30, tonyg wrote:
> > On 2013/11/18 17:41:00, rdsmith wrote:
> > > On 2013/11/18 17:37:10, tonyg wrote:
> > > > Nat is about to totally redo the results object in
> > > > https://codereview.chromium.org/27486002/ . We'll need to reconcile
these.
> > > > 
> > > > I recommend you get his buy-in on the results object change first, then
> I'll
> > > > review the HTML output part in a subsequent change.
> > > 
> > > Sounds ... well, I can't quite say "good", but makes sense :-} :-J.  Nat:
I
> > > think what probably makes sense is for me to wait until you've landed
> > 27486002,
> > > and then create a CL with just the results object changes in it.  I'm
> tracking
> > > your CL now.  Let me know if you want to do something different (e.g.
> skimming
> > > this CL and giving me guidance or high level feedback before I dive into
the
> > > details).
> > 
> > BTW, I realized I was too terse after you've been pinging this review for
two
> > months. Sorry :(
> > 
> > To be honest, I'm not wild about the AddComparison API and have been
> incorrectly
> > and unfairly postponing reviewing this because I'm not sure what to suggest
> > exactly. So since Nat is really trying to get a solid API around results
now,
> I
> > thought he could suggest something that will fit well.
> > 
> > I was initially hoping that this could be done all in the UI, but it sounds
> like
> > you weren't on board with that idea either.
> 
> So I had thought that we had discussed the various options in a VC meeting
some
> time ago, and this was the architecture we had decided on.  I'm willing to do
a
> different architecture (sigh), but I'd like it to address the concerns we
> discussed in that meeting.  Shall you, I, and Nat sit down again and try and
> figure out how to do this right?

Probably a good idea. Want to pop something on the calendar?

[tonyg, 2015-01-13 02:41:50]

tonyg@chromium.org changed reviewers:
- tonyg@chromium.org

[tonyg, 2015-01-13 02:42:02]

tonyg@chromium.org changed reviewers:
- dtu@chromium.org