1021842 - Talos: improve graphserver's formula for "final test result"

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Avi Halachmi (:avih)]

+++ This bug was initially created as a clone of Bug #1020677 +++

Bug 1020677 discusses 2 main changes to improve our regression detection for tests where each sub-test is independently meaningful (e.g. yes TART, not TP5):

1. Improved formula for processing the sub-results into a single number.
2. Regression detection which also takes individual sub-results into account.

This bug is about #1 above.

Summary of the current formula:

> - A talos run of a test consists of N executions of each sub-test (mostly N==25).
> - Take median for each N results per sub-test.
> - Ignore the highest of those medians.
> - Average the rest <-- the "final" number by which we calculate regressions
>                        and which graphserver displays at its graphs.

The new formula will:
- Use geometric mean instead of average,
- Without dropping the highest sub-result.

The formula is used explicitly in one place (when collecting the data, e.g. when talos sends it to graphserver) where graphserver then stores the result of this calculation, and it's also used implicitly at any later time when the graphs are displayed (using this stored pre-calculated value).

So, unfortunately, we can't just modify the formula and expect all the graphs to reflect it retroactively, because the calculation result is cached, and it's probably impractical to literally rewrite history cache using a new formula (though TBH, it might not be an absolutely terrible idea if we believe the new formula is better).

So, we'll approach this transition from the old to the new formula as follows:
- The old formula will keep producing results for the transition period.
- The new formula will produce results under a new temporary test name for this period.

This will allow us to catch regressions around the switch (using any of the formulas).

Once the transition period ends and we're happy with the new formula, we'll terminate the old formula, and will only report values using the new formula - with the _old_ names (so effectively also terminating the new temporary names). Such that whoever browses the graph doesn't have to switch to a different test (name), and instead may see only some "jump" in values when the formula switched.

We'll also add a dev.tree-management and wiki notice about tests which started using the new formula, and at which date.

Anything still missing?

Comment 1

[Avi Halachmi (:avih)]

Joel, so re bug 1020677 comment 41 - 44, seems like nothing blocks us from going on with this. Correct?

How do you suggest to continue? I'd prefer if you take this bug, but if you can't, let me know and I'll try to handle it myself. Right now it seems we've decided to continue but.. nothing happens :)

Comment 2

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

Attachment: initial work done (0.5)

uploading my WIP patch, it requires a few more changes and a lot of local testing.  I won't get to this for at least a week, most likely 2 weeks.

Comment 3

[Avi Halachmi (:avih)]

Comment on attachment 8448973 [details] [diff] [review]
initial work done (0.5)

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

I know it's a WIP patch, but wanted to check it out regardless.

What repo is it at (so I could compare the existing formula [insisible at the patch] to the new one)?

::: server/pyfomatic/collect.py
@@ +228,3 @@
>    _updateAverageForTestRun(average, databaseCursor, inputStream, metadata)
> +
> +  # I am concerned that this might fail and we never 

Accidentally cut line?

::: sql/data.sql
@@ +4570,4 @@
>  insert into tests values (NULL,"sessionrestore_no_auto_restore","Session Restore no Auto Restore Test",1,1,NULL);
>  
>  insert into tests values (NULL,"ts_paint_cold","TS Paint, Cold Load Time",1,1,NULL);
> +insert into tests values (NULL,"tp5o_scroll_paint","TP5 Scroll",1,1,15);

I'm guessing this unrelated to this bug?

Comment 4

[Avi Halachmi (:avih)]

Pardon the (way too many) typos on comment 3...

Comment 5

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

here is the repo:
http://hg.mozilla.org/graphs

you can run it locally without too much trouble.  yes, the patch needs a lot of cleanup :)

Comment 6

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

Attachment: add geomean field to db and populate it (1.0)

Comment 7

[Avi Halachmi (:avih)]

Comment on attachment 8499099 [details] [diff] [review]
add geomean field to db and populate it (1.0)

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

Due to my lack of familiarity with Graph server internals, this is a f+ that it looks roughly right to me (I especially like the calculation part), but it shouldn't be considered a proper review.

Comment 8

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

Attachment: modify api that graph.html uses to include geomean in addition to everything else (0.9)

this hasn't been tested, a few more code paths for me to explore.

Comment 9

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

for the core geomean calculation in the first patch, we need to run this on the database:
alter table alerts add geomean FLOAT;

when we get r+, I will request this be done on the graph server db's.  :MattN, this is something you will need to run manually on your local database.

Comment 10

[Robert Helmer [:rhelmer]]

Comment on attachment 8499099 [details] [diff] [review]
add geomean field to db and populate it (1.0)

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

Unless I am missing something,

::: server/pyfomatic/collect.py
@@ +229,5 @@
> +
> +  try:
> +    databaseCursor.execute("""select exp(sum(log(value))/count(value)) from test_run_values where test_run_id = %s""", (metadata.test_run_id))
> +    geomean = databaseCursor.fetchall()[0][0]
> +    if geomean is None:average

Is "average" supposed to be there ^

@@ +234,5 @@
> +      geomean = values[1]
> +    _updateGeoMeanForTestRun(geomean, databaseCursor, inputStream, metadata)
> +  except Exception, x:
> +    # we don't want to rollback or raise an exception, this should be passively running in the background
> +    pass

Silently eating exceptions makes me sad, although the comment is reasonable. Maybe we could log something?

Comment 11

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

Attachment: add geomean field to db and populate it (1.1)

oops- there was a cut/paste nugget in there :(

I have cleaned up it, and added print statements.  This is running in the context of a webserver, so I assume these print statements show up in the webserver log somewhere?

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8499136 [details] [diff] [review]
add geomean field to db and populate it (1.1)

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

Hmm so print might not work - there isn't really any logging going on in here already is there :/

This is running under WSGI now so may not work if something is printed to stdout, I will test.

Comment 13

[Robert Helmer [:rhelmer]]

Comment on attachment 8499136 [details] [diff] [review]
add geomean field to db and populate it (1.1)

lgtm, tested print on the dev server and goes to Apache error log, which seems fine.

Comment 14

[Matthew N. [:MattN]]

(In reply to Joel Maher (:jmaher) from comment #9)
> alter table alerts add geomean FLOAT;

Note that the correct table is test_runs:

alter table test_runs add geomean FLOAT;

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8499136 [details] [diff] [review]
add geomean field to db and populate it (1.1)

While testing this patch I got the following error upon upload of a TART result:

  Exception: to determine geomean from 'test_run_values' for  1369 - 'long' 
  object is not iterable

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8499136 [details] [diff] [review]
add geomean field to db and populate it (1.1)

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

::: server/pyfomatic/collect.py
@@ +228,4 @@
>    _updateAverageForTestRun(average, databaseCursor, inputStream, metadata)
> +
> +  try:
> +    databaseCursor.execute("""select exp(sum(log(value))/count(value)) from test_run_values where test_run_id = %s""", (metadata.test_run_id))

(In reply to Matthew N. [:MattN] from comment #15)
> 'long' object is not iterable

This is from a missing comma after "metadata.test_run_id". The argument should be a tuple like:
(metadata.test_run_id,)

With that the patch works on my server :)

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8499113 [details] [diff] [review]
modify api that graph.html uses to include geomean in addition to everything else (0.9)

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

I have this being used on my GS instance when "?geo" is the query string: http://graphs.mattn.ca/graph-local.html?geo

I didn't test all of the affected endpoints and I didn't check the math by hand.

::: server/api.py
@@ +322,5 @@
>              for changeset, total in averages.iteritems())
> +
> +        geomeans = dict(
> +            (changeset, total / geo_totals[changeset])
> +            for changeset, total in geomeans.iteritems())

OK, so this is averaging the geometric means for a specific changeset but it's just confusing because geo_totals actually contains a count of the number of geometrics means for a changeset, not a total as the name says. num_geos would be easier to understand IMO. Not sure if you want to change that for none, one, or both of the cases (avg & geo).

@@ +328,5 @@
>                    'averages': averages,
>                    'min': min(row['average'] for row in rows),
>                    'max': max(row['average'] for row in rows),
> +                  'geomeans': geomeans,
> +                  'mingeo': min(row['geomean'] for row in rows),

Not sure how much of a problem this is but "mingeo" ends up being "null" since there are old rows where mingeo wasn't calculated yet. It would be nice for it to exclude null.

Comment 18

[Matthew N. [:MattN]]

I tested that the following two endpoints show the data:
http://graphs.mattn.ca/api/test/runs/revisions?revision=b6c2deadbeef (used by compare-talos)
http://graphs.mattn.ca/api/test/runs?id=293&branchid=1&platformid=13 (used by graphs)

Comment 19

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

landed this backend change:
http://hg.mozilla.org/graphs/rev/77bdf53fae9f

waiting for the database to be updated before we make this live.  Next up is getting the api changes landed.

Comment 20

[Avi Halachmi (:avih)]

Do we have a bug to backfill the new column with the new calculation result? such that we could examine 2-3+ months old data with the new calculation?

Comment 21

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

I was not aware of doing that, I was under the impression this plan was to make this possible and collect it going forward.  What benefit would we get from going back in time?  If you think it is beneficial, please file a bug and outline what needs to be done.

Comment 22

[Avi Halachmi (:avih)]

Bug 1020677 comment 41 - 44 suggested that we should just switch the formula, at which case back-filling (practically - rewriting history) would probably a bad idea.

However, the way which was apparently chosen to go forward, implied by the patch at this bug, is that we add a new column for the new formula calculation, at which case this column would be empty for older results, at which case I think it should be possible to back-fill it.

If we think that the new formula is better, and I think we do, then being able to watch graphs over few months with the new formula would be better than watching them with the old formula.

So I guess the first question is "would it be possible to back-fill the new column of the new formula?"

If yes (and I _think_ it should be a yes), then I'll file a bug for it.

Comment 23

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

yeah, upon further thought and your comment, we really should backfill as much as possible.  I suspect it just requires us writing a script to query the data for the page values, then calculate/insert the geomean.

That might take some fiddling with, but it should be possible.  I suspect a custom script that we could run would be the ideal thing to do.

Comment 24

[Avi Halachmi (:avih)]

Comment on attachment 8499136 [details] [diff] [review]
add geomean field to db and populate it (1.1)

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

::: server/pyfomatic/collect.py
@@ +228,4 @@
>    _updateAverageForTestRun(average, databaseCursor, inputStream, metadata)
> +
> +  try:
> +    databaseCursor.execute("""select exp(sum(log(value))/count(value)) from test_run_values where test_run_id = %s""", (metadata.test_run_id))

So what happens if value is 0? log(value) would be -INFINITY, which is probably undesirable (and in a simpler calculation of Nth root of the product, the product would be 0).

I think some (sub)tests do report 0 on cases of error, and while we do want it to affect the result, I don't think we want it to null-ify it.

In the simplified calculation, I'd say just use Nth root of the product of N-1 values (if it only had a single 0 sub-result) which would be the equivalent of averaging N values where one of them is 0, though I'm not sure how to approach it with the formula which you used at this patch.

Comment 25

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #24)
>
> So what happens if value is 0? log(value) would be -INFINITY, which is
> probably undesirable (and in a simpler calculation of Nth root of the
> product, the product would be 0).


Hmm.. I think there's a simple workaround for this issue, and which would also solve the general issue of values lower than 1 (which I don't think we have right now, but it's not impossible to have them).

How about just do ... sum(log(value + 1))/count(value) ... ?

Such change would still keep the relative results the same, would hardly affect the absolute values (since most of them are considerably higher than 1 anyway) and will solve the 0 issue and the general "smaller than 1" issue.

Joel?

Comment 26

[Avi Halachmi (:avih)]

Maybe also subtract 1 from the final result (because it's a mean of each value plus 1) and clamping it at 0 if it's negative?

Comment 27

[Kyle Lahnakoski [:ekyle]]

I suggest ignoring the zeros, and using absolute value, just in case:

> select 
>     exp(sum(log(abs(value)))/count(value)) 
> from 
>     test_run_values 
> where 
>     test_run_id = %s""" and
>     abs(value) > 0

Comment 28

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

Kyle, what would happen if we selected nothing?

Comment 29

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

Attachment: modify api that graph.html uses to include geomean in addition to everything else (1.0)

Matt, thanks for the comment here, I have updated the patch to make the variable names more useful and to default to a value of 0 if we find a non float type.

Avi, for geomeans I am really taking an average of the geomeans if we end up with >1 value for a given build/test/platform/branch combo.  Do you think that is correct?

Comment 30

[Avi Halachmi (:avih)]

(In reply to Kyle Lahnakoski [:ekyle] from comment #27)
> I suggest ignoring the zeros, and using absolute value, just in case:
> 
> > select 
> >     exp(sum(log(abs(value)))/count(value)) 
> > from 
> >     test_run_values 
> > where 
> >     test_run_id = %s""" and
> >     abs(value) > 0

This would completely ignore the 0's at the geomean calculation. However, I'd think that preferably having 0's at some sub tests would also be reflected at the summary/average/geomean.

The average already reflects it by including 0 at the average (so the average will be lower), my suggestion of adding 1 to each value and then subtracting 1 from the sum would be the equivalent when doing a geomean, I think. But if that's hard to define in SQL syntax, then your suggestion is still better than the patch prior to comment 29. Thanks.

(In reply to Joel Maher (:jmaher) from comment #29)
> ...
> Avi, for geomeans I am really taking an average of the geomeans if we end up
> with >1 value for a given build/test/platform/branch combo.  Do you think
> that is correct?

Not sure I follow you. geomean _is_ the replacement for the average at the new calculation, what does "average of geomeans" mean?

Also, I really liked the previous calculation, how's the new one better? (other than also dealing with 0's - which I think we could also have done with the previous calculation instead of replacing it altogether).

Comment 31

[Kyle Lahnakoski [:ekyle]]

(In reply to Joel Maher (:jmaher) from comment #28)
> Kyle, what would happen if we selected nothing?

The result would be NULL (well, not even calculated).  If there are no values, or all values are zero, then geomean has no clear definition.  Alternatively, I would define geomean([])=1, just for the sake of logical consistency.

Comment 32

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #30)
> (In reply to Kyle Lahnakoski [:ekyle] from comment #27)
> > I suggest ignoring the zeros, and using absolute value, just in case:
> > 
> > > select 
> > >     exp(sum(log(abs(value)))/count(value)) 
> > > from 
> > >     test_run_values 
> > > where 
> > >     test_run_id = %s""" and
> > >     abs(value) > 0
> 
> This would completely ignore the 0's at the geomean calculation. However,
> I'd think that preferably having 0's at some sub tests would also be
> reflected at the summary/average/geomean.
> ...


I think there is actually a case where we'd like to completely ignore the value - if there's no value (i.e. not even 0). We could get this case if we cancel a page/sub-test, so that its value isn't sent anymore to graphserver, but does exist for test runs prior to removing this page. On such case we should completely ignore it at any calculation (be it average, geomean, or anything else) where it isn't available.

Joel, What happens now if we cancel a page, e.g. like we removed alipay.com from tp5o at bug 1026869 ? is it taken into account with the current average calculation for tp5o?

Kyle, what about comment 30 WRT the inclusion of 0's into the geomean to indicate a difference at the summary as well?

Comment 33

[Kyle Lahnakoski [:ekyle]]

In reply to Avi Halachmi (:avih) from comment #30)
> (In reply to Kyle Lahnakoski [:ekyle] from comment #27)
> > > select 
> > >     exp(sum(log(abs(value)))/count(value)) 
> > > from 
> > >     test_run_values 
> > > where 
> > >     test_run_id = %s""" and
> > >     abs(value) > 0
> 
> This would completely ignore the 0's at the geomean calculation. However,
> I'd think that preferably having 0's at some sub tests would also be
> reflected at the summary/average/geomean.

Avi, 

I am concerned your log(v+1) proposal will swamp legitimate performance values between 0 and 1.  I do not know if that even happens, so maybe my concern is unwarranted.

Ignoring zeros can be good, especially if zero is being used to indicate failure.  Furthermore, the ignorance will impact the overall geomean[1]; which means the introduction of zeros will be detectable.  Finally, the v8 test suite uses geomean: using log(v+1) we loose the ability to compare.


[1] except in the case when the ignored metric is very close to 1

Comment 34

[Robert Helmer [:rhelmer]]

Comment on attachment 8500664 [details] [diff] [review]
modify api that graph.html uses to include geomean in addition to everything else (1.0)

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

Looks reasonable to me, I am not set up to test at the moment. I wish we had tests on this codebase :/

Comment 35

[Avi Halachmi (:avih)]

(In reply to Kyle Lahnakoski [:ekyle] from comment #33)
> I am concerned your log(v+1) proposal will swamp legitimate performance
> values between 0 and 1.  I do not know if that even happens, so maybe my
> concern is unwarranted.

What does "swamp" mean/imply here? could you please provide sample set of numbers which would exhibit this issue with the (v+1) calculation? Also note than apart from adding 1 to each value, my suggestion also includes subtracting one from the final result (comment 26).

> Ignoring zeros can be good, especially if zero is being used to indicate
> failure.  Furthermore, the ignorance will impact the overall geomean[1];
> which means the introduction of zeros will be detectable.

Indeed ignoring 0's will impact the overall, but IMO less so and less deterministically so than including an unlikely low value into the calculation (such as 0 into an average - which we do now already).

> Finally, the v8
> test suite uses geomean: using log(v+1) we loose the ability to compare.

Not sure I understand this. Would you be able to provide a sample set of results which would get hurt from using the v+1 ... -1 calculation?

Comment 36

[Matthew N. [:MattN]]

Comment on attachment 8500664 [details] [diff] [review]
modify api that graph.html uses to include geomean in addition to everything else (1.0)

Flag me for review again once we've settled on the formula and I can test.

Comment 37

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

Kyle,

I want to get this resolved, can you help clarify your concerns with avi's approach of:
exp(sum(log(abs(value + 1)))/count(value)) - 1

Comment 38

[Kyle Lahnakoski [:ekyle]]

Avi,

(In reply to Avi Halachmi (:avih) from comment #35)
> What does "swamp" mean/imply here?

My apologies upfront about being pedantic here.  A geometric argument would be easier to digest.

Let
    F0(v)=exp(sum(log(vi))/n)
    F1(v)=exp(sum(log(vi+1))/n)-1
where
    v = [v1, v2, v3, ..., vn]

Consider a performance change of E in one of the statistics, such that v1' = (1+E)*v1
and v' = [v1', v2, v3, ..., vn]


F1(v') = exp(sum(log(vi'+1))/n)
F1(v') = exp([log(v1'+1) + log(v2+1) + ... + log(vn+1)]/n)
F1(v') = exp([log((1+E)*v1 + 1) + log(v2+1) + ... + log(vn+1)]/n)
F1(v') = exp([log((1+E)*v1 + 1) - log(v1+1) + log(v1+1) + log(v2+1) + ... + log(vn+1)]/n)
F1(v') = exp([log((1+E)*v1 + 1) - log(v1+1) + sum(log(vi+1))]/n)
F1(v') = exp(log((1+E)*v1 + 1)/n - log(v1+1)/n + sum(log(vi+1))/n)
F1(v') = exp(log((1+E)*v1 + 1)/n - log(v1+1)/n) * exp(sum(log(vi+1))/n))
F1(v') = exp(log((1+E)*v1 + 1) - log(v1+1))^(1/n) * F1(v)
F1(v') = ((1+E)*v1 + 1)/(v1+1))^(1/n) * F1(v)
F1(v') = ((v1+1 + v1*E)/(v1+1))^(1/n) * F1(v)

We will use Taylor's theorem near v1 to approximate the slope wrt E:

dF1(v)/dE = (1/n)*(((v1+1 + v1*E)/(v1+1))^((1/n)-1))*v1/(v1+1) * F1(v)
dF1(v)/dE (at E=0) = (1/n)*(1^((1/n)-1))*v1/(v1+1) * F1(v)
dF1(v)/dE (at E=0) = (1/n) * v1/(v1+1) * F1(v)

F1(v') = F1(v) + (1/n) * v1/(v1+1) * F1(v) * E  (at E near 0)

We look at F1(v') - F1(v) to emphasize our slope:

F1(v') - F1(v) = (1/n) * v1/(v1+1) * F1(v) * E  (at E near 0)
F1(v') - F1(v) = v1/(v1+1) * (F1(v) * E)/n  (at E near 0)

Using similar logic, compare this to behaviour of F0 near E=0:

F0(v') - F0(v) = (F0(v) * E)/n  (at E near 0)

The F1 function has an extra v1/(v1+1) term; for large values this is near 1, but for small values this number is small, and makes error detection more difficult.

F1(v') - F1(v) = v1/(v1+1) * (F0(v') - F0(v))  (at E near 0)


Suppose we have a statistic around 0.5, and it changes by 10%:  How much does F1 change? 

F1(v') - F1(v) = 0.5/(0.5+1) * (F0(v') - F0(v))  (at E near 0)
F1(v') - F1(v) = 1/3 * (F0(v') - F0(v))  (at E near 0)

Which is 1/3 less sensitive than F0.  And smaller values are worse, of course.

This is what I meant by "swamp".  All this is mute if performance values are > 1.

Comment 39

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

thanks Kyle.  what formula do you use in dzalerts?

Comment 40

[Kyle Lahnakoski [:ekyle]]

Avi, 

How does F1 compare to F0 in the face of zeros? We start with F1, and compare the new value to the old:

F1(v')/F1(v) = prod(vi'+1)^(1/n) / prod(vi+1)^(1/n)
F1(v')/F1(v) = (prod(vi'+1)/prod(vi+1))^(1/n)
F1(v')/F1(v) = [(0+1)*(v2+1)*(v3+1)*...*(vn+1) / ((v1+1)*(v2+1)*(v3+1)*...*(vn+1))]^(1/n)
F1(v')/F1(v) = [(0+1)/(v1+1)]^(1/n)
F1(v')/F1(v) = [1/(v1+1)]^(1/n)

Same for F0, which ignores the zero (v1' = 0):

F0(v')/F0(v) = [v2 * v3 * ... * vn]^(1/(n-1)) / [v1 * v2 * v3 * ... * vn]^(1/n)
F0(v')/F0(v) = [v2 * v3 * ... * vn]^(n/n(n-1)) / [v1 * v2 * v3 * ... * vn]^((n-1)/n(n-1))
F0(v')/F0(v) = [[v2 * v3 * ... * vn]^n / [v1 * v2 * v3 * ... * vn]^(n-1)]^(1/(n(n-1)))
F0(v')/F0(v) = [[v2 * v3 * ... * vn] / v1^(n-1)]^(1/(n(n-1)))
F0(v')/F0(v) = [v2 * v3 * ... * vn]^(1/(n(n-1))) / v1^(1/n)
F0(v')/F0(v) = (1/v1)^(1/n) * F0(v')^(1/n)


Let K1 = F1(v')/F1(v) 
Let K0 = F0(v')/F0(v)

When is K1 > K0 ?

<=> [1/(v1+1)]^(1/n) > (1/v1)^(1/n) * F0(v')^(1/n)
<=> [1/(v1+1)]^(1/n) / (1/v1)^(1/n) > F0(v')^(1/n) 
<=> [(1/(v1+1)) / (1/v1)]^(1/n) > F0(v')^(1/n) 
<=> [v1/(v1+1)]^(1/n) > F0(v')^(1/n)
<=> [v1/(v1+1)] > F0(v')

If performance values less than 1 do not exist: Then F0(v') > 1, and K1 < K0, and we can conclude F0 is a better zero indicator than F1.  If performance values less than 1 do exist, then K1 could perform better given the condition above.  The difference between F0 and F1 is probably inconsequential in practice due to high vi>>1 values and n>3.  

When F0 and F1 do differ, we must also consider the noise is similarly different.  An analysis of how this affects confidence must be done to be sure F0 still performs better than F1.

In conclusion, I like F0 for its adherence to definition, and its proper treatment of performance values less than one.

Comment 41

[Kyle Lahnakoski [:ekyle]]

Joel,

I use the formal definition of geomean: F0

Comment 42

[Kyle Lahnakoski [:ekyle]]

(In reply to Avi Halachmi (:avih) from comment #35)
> (In reply to Kyle Lahnakoski [:ekyle] from comment #33)
> > Finally, the v8
> > test suite uses geomean: using log(v+1) we loose the ability to compare.
> 
> Not sure I understand this. Would you be able to provide a sample set of
> results which would get hurt from using the v+1 ... -1 calculation?

All I am saying here is that the V8 performance statistic uses the formal definition of geomean to combine the test results.  If we use some other aggregate metric, like F1, then we will not be able to compare the official V8 statistic to the one we see in graph server.

Comment 43

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

I thought I had implemented the formal definition to start with.  Here is what I have in talos:
http://hg.mozilla.org/build/talos/file/b2a3d0493173/talos/filter.py#l61

but we have questions about values <1.

Kyle, can you link me to the formal definition you are using?

Comment 44

[Avi Halachmi (:avih)]

(In reply to Kyle Lahnakoski [:ekyle] from comment #38)
> Avi,
> 
> (In reply to Avi Halachmi (:avih) from comment #35)
> > What does "swamp" mean/imply here?
> 
> My apologies upfront about being pedantic here.  A geometric argument would
> be easier to digest.

Pedantic is appreciated, I just didn't understand what swamp imply :) and now I do - it compresses the dynamic range slightly more than the "pure" geometric mean formula, and this compression increases the closer we are to 0.

I also never claimed that the two calculations are equivalent - they're clearly not.

However, consider these two "sets of results" which are undeniably much closer to 0 than to 1 and therefore should "suffer" from being less sensitive to detect regression between them with the +/- 1 variant:

var set1 = [.01, .02, .03, .04, .05];
var set2 =      [.02, .03, .04, .05, .06];

Their averages are 0.03 and 0.04 respectively.
"proper" geometric means are 0.026 and 0.037
geometric means with the +1/-1 variant are 0.0299 and 0.0399

regression from set1 to set2:

Using average: 33.33%
Using proper geometric mean: 43.1%
Using +1/-1 geometric mean: 33.4%

The +/-1 version is indeed less sensitive than the proper geomean.

However, even for such small values, the +1/-1 geomean regression calculations will still be more sensitive than the average-based regression calculation which we've been using till now. So compared to now, I don't think it will hurt us.

> This is what I meant by "swamp".  All this is mute if performance values are
> > 1.

I've never seen results smaller than 1. Maybe Joel did?

(In reply to Kyle Lahnakoski [:ekyle] from comment #40)
> ...
> In conclusion, I like F0 for its adherence to definition, and its proper
> treatment of performance values less than one.

I don't disagree with the preference to choose the purest form available. But I'm trying to apply a practical approach where this specific deviation from "purest" might have more pros than cons, or that in practice the cons are negligible, as I _think_ they are.

(In reply to Kyle Lahnakoski [:ekyle] from comment #42)
> All I am saying here is that the V8 performance statistic uses the formal
> definition of geomean to combine the test results.  If we use some other
> aggregate metric, like F1, then we will not be able to compare the official
> V8 statistic to the one we see in graph server.

I still don't get it. We've been using averages till now, and it never bothered us. Surely geomean +/-1 would be still better than averages?

Also, the V8 numbers are more than 15k, the +/-1 variant difference compared to "pure" version would be less than negligible.

Comment 45

[Kyle Lahnakoski [:ekyle]]

Joel, 

The code has implemented the formal definition.

Comment 46

[Kyle Lahnakoski [:ekyle]]

Avi, 

We are in agreement.

(In reply to Avi Halachmi (:avih) from comment #44)
> (In reply to Kyle Lahnakoski [:ekyle] from comment #42)
> > All I am saying here is that the V8 performance statistic uses the formal
> > definition of geomean to combine the test results.  If we use some other
> > aggregate metric, like F1, then we will not be able to compare the official
> > V8 statistic to the one we see in graph server.
> 
> I still don't get it. We've been using averages till now, and it never
> bothered us. Surely geomean +/-1 would be still better than averages?

Using averages was wrong, and geomean +/-1 is way better.  Also geomean +/-1 is negligibly different from the geomean for the purposes of regression detection given the >>1 performance values we see.

Comment 47

[Kyle Lahnakoski [:ekyle]]

(In reply to Kyle Lahnakoski [:ekyle] from comment #45)
> Joel, 
> 
> The code has implemented the formal definition.

well, except for it's treatment of zeros, where it is undefined :)

Comment 48

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

Attachment: reduce the chance of values = 0 (1.0)

Comment 49

[Avi Halachmi (:avih)]

Comment on attachment 8503345 [details] [diff] [review]
reduce the chance of values = 0 (1.0)

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

w00t!

Comment 50

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

https://hg.mozilla.org/graphs/rev/a1977105ed7c

Comment 51

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

Attachment: modify api that graph.html uses to include geomean in addition to everything else (1.1)

slight adjustment to the script, accounting for your previous feedback and double checking the code.

Going from the definitions listed here:
https://wiki.mozilla.org/Perfomatic:API#Get_test_run_information

We have 3 cases where the average is displayed and we should also display the geometric mean:
run info:
desc: https://wiki.mozilla.org/Perfomatic:API#Get_test_run_information
ex: http://graphs.mozilla.org/api/test/runs/info/?id=3794718 (broken)

test runs:
desc: https://wiki.mozilla.org/Perfomatic:API#Get_test_runs_.28data.29
ex: http://graphs.mozilla.org/api/test/runs?id=223&branchid=131&platformid=31&days=7

by revision:
desc: https://wiki.mozilla.org/Perfomatic:API#Get_test_data_by_revision
ex: http://graphs.mozilla.org/api/test/runs/revisions?revision=10d2046d2b64&revision=d16525937c8b

going through the code for these 3 cases, I believe I have properly handled added geomean to the api.

Comment 52

[Matthew N. [:MattN]]

My server is now updated with the landed patch and attachment 8503400 [details] [diff] [review]. Testing now.

Comment 53

[Matthew N. [:MattN]]

The mingeo and/or maxgeo line with the min/max is causing a 500 error on the server. I think it's not returning a number in some cases.

Comment 54

[Matthew N. [:MattN]]

Comment on attachment 8503400 [details] [diff] [review]
modify api that graph.html uses to include geomean in addition to everything else (1.1)

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

::: server/api.py
@@ +329,5 @@
>                    'min': min(row['average'] for row in rows),
>                    'max': max(row['average'] for row in rows),
> +                  'geomeans': geomeans,
> +                  'mingeo': min(type(row['geomean']) if type(row['geomean']) == type(1.0) else 0.0 for row in rows),
> +                  'maxgeo': max(type(row['geomean']) if type(row['geomean']) == type(1.0) else 0.0 for row in rows),

I think the first |type(…)| on each of these lines is wrong but I'm not expert in Python. It ends up returning the type instead of the value.

Comment 55

[Robert Helmer [:rhelmer]]

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #54)
> Comment on attachment 8503400 [details] [diff] [review]
> modify api that graph.html uses to include geomean in addition to everything
> else (1.1)
> 
> Review of attachment 8503400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: server/api.py
> @@ +329,5 @@
> >                    'min': min(row['average'] for row in rows),
> >                    'max': max(row['average'] for row in rows),
> > +                  'geomeans': geomeans,
> > +                  'mingeo': min(type(row['geomean']) if type(row['geomean']) == type(1.0) else 0.0 for row in rows),
> > +                  'maxgeo': max(type(row['geomean']) if type(row['geomean']) == type(1.0) else 0.0 for row in rows),
> 
> I think the first |type(…)| on each of these lines is wrong but I'm not
> expert in Python. It ends up returning the type instead of the value.

You're right... comparing type(row['geomean']) == type(1.0) is a bit confusing too, that should probably just be e.g.:

'mingeo': min(row['geomean'] if type(row['geomean']) == float else 0.0 for row in rows),

Comment 56

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

ah:
'mingeo': min(row['geomean'] if isinstance(x, float) else 0.0 for row in rows),


can we assume all values are float?  I assume so, but let me double check.

Comment 57

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

Attachment: modify api that graph.html uses to include geomean in addition to everything else (2.0)

updated patch to check for float|int.  Tested with mocked up python locally.

Comment 58

[Matthew N. [:MattN]]

Comment on attachment 8504277 [details] [diff] [review]
modify api that graph.html uses to include geomean in addition to everything else (2.0)

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

::: server/api.py
@@ +329,5 @@
>                    'min': min(row['average'] for row in rows),
>                    'max': max(row['average'] for row in rows),
> +                  'geomeans': geomeans,
> +                  'mingeo': min(row['geomean'] if isinstance(row['geomean'], float) or isinstance(row['geomean'], int) else 0.0 for row in rows),
> +                  'maxgeo': max(row['geomean'] if isinstance(row['geomean'], float) or isinstance(row['geomean'], int) else 0.0 for row in rows),

This seems to work. I would have preferred just checking for the None/Null case (however that gets represented) instead of float/int but this is better than nothing.

Comment 59

[Robert Helmer [:rhelmer]]

Comment on attachment 8504277 [details] [diff] [review]
modify api that graph.html uses to include geomean in addition to everything else (2.0)

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

::: server/api.py
@@ +329,5 @@
>                    'min': min(row['average'] for row in rows),
>                    'max': max(row['average'] for row in rows),
> +                  'geomeans': geomeans,
> +                  'mingeo': min(row['geomean'] if isinstance(row['geomean'], float) or isinstance(row['geomean'], int) else 0.0 for row in rows),
> +                  'maxgeo': max(row['geomean'] if isinstance(row['geomean'], float) or isinstance(row['geomean'], int) else 0.0 for row in rows),

Hm so this could be coerced to float and have a None check instead:

'mingeo': min(float(row['geomean']) if not isinstance(row['geomean'], None) else 0.0 for row in rows),

That will raise an exception if something is in there that can't be coerced to float. If this is never expected to happen then raising here might be a good thing.

Comment 60

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #59)
> Comment on attachment 8504277 [details] [diff] [review]
> modify api that graph.html uses to include geomean in addition to everything
> else (2.0)
> 
> Review of attachment 8504277 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: server/api.py
> @@ +329,5 @@
> >                    'min': min(row['average'] for row in rows),
> >                    'max': max(row['average'] for row in rows),
> > +                  'geomeans': geomeans,
> > +                  'mingeo': min(row['geomean'] if isinstance(row['geomean'], float) or isinstance(row['geomean'], int) else 0.0 for row in rows),
> > +                  'maxgeo': max(row['geomean'] if isinstance(row['geomean'], float) or isinstance(row['geomean'], int) else 0.0 for row in rows),
> 
> Hm so this could be coerced to float and have a None check instead:
> 
> 'mingeo': min(float(row['geomean']) if not isinstance(row['geomean'], None)
> else 0.0 for row in rows),
> 
> That will raise an exception if something is in there that can't be coerced
> to float. If this is never expected to happen then raising here might be a
> good thing.

(maybe I should say "cast" instead of "coerce" since it's explicit)

Anyway that's more of a response to comment 58, you can take it or leave it I think the code looks OK to go in as-is. Thanks to MattN for testing this.

Comment 61

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

https://hg.mozilla.org/graphs/rev/f8c4ec8f9ac9

Comment 62

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

:rhelmer,  now that we have the backend in place, can we get this deployed and started on collected data.  We can view the results from a local version of graph.html.  Should we file a bug for deployment?  do you have a suggested path forward?

Comment 63

[Robert Helmer [:rhelmer]]

(In reply to Joel Maher (:jmaher) from comment #62)
> :rhelmer,  now that we have the backend in place, can we get this deployed
> and started on collected data.  We can view the results from a local version
> of graph.html.  Should we file a bug for deployment?  do you have a
> suggested path forward?

I filed bug 1082694 to track this and will work on it now.

Comment 64

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

so here is what I believe is left to do:
* look at the data now that we have some and verify it is what we want
* deploy a UI change to production that will view this data
* adjust the alerting all to reference the geomean instead of average field

am I missing anything?

Comment 65

[Matthew N. [:MattN]]

(In reply to Joel Maher (:jmaher) from comment #64)
> * deploy a UI change to production that will view this data

That's bug 1077177 for graph server btw.

Comment 66

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

after a brief chat on irc, the next steps are:
* send geometric mean alerts to dev.tree-alerts (the new newsgroup)
* continue to send average alerts to dev.tree-management
* after a week or two to ensure all is well, disable average alerts to dev.tree-management

This involves some assumptions, but all very doable:
* analyze_talos.py has enough cpu bandwidth to run two passes
* we are fine not emailing patch authors twice (only email on 'average' alerts for now)
* someone will look at both sources.

The problem becomes, what do we do if we find alerts that show up in 'average' only and not in 'geomean'?  Will we be fine ignoring those?

Comment 67

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

Attachment: update scripts / tooling to support geomean (1.0)

I am not sure how to modify the analysis.cfg.template.  I would like to make sure that we have:
# Who should emails be sent to
regression_emails = [e-mail]
geomean_regression_emails = [e-mail]

I honestly have no idea what value is in regression_emails, so figuring out what is there would be a good idea.

Comment 68

[Robert Helmer [:rhelmer]]

Comment on attachment 8526144 [details] [diff] [review]
update scripts / tooling to support geomean (1.0)

I don't actually know where this tool is run... IIRC catlee has the analysis tool running somewhere.

Comment 69

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

catlee, can you answer this question:
I am not sure how to modify the analysis.cfg.template.  I would like to make sure that we have:
# Who should emails be sent to
regression_emails = [e-mail]
geomean_regression_emails = [e-mail]

I guess making available the analysis.cfg file (sans private keys, etc.) would be a good thing :)

Comment 70

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

Also, if catlee knows more about this script and it isn't run on the normal graph server (i.e. this is run someplace that he has access to), then I would like to know what the procedure is for deploying this.

Comment 71

[Ed Morley [:emorley]]

(In reply to Joel Maher (:jmaher) from comment #69)
> I guess making available the analysis.cfg file (sans private keys, etc.)
> would be a good thing :)

Ideally we could just check it into the repo in the appropriate location, but named as analysis.cfg.example etc

Comment 72

[Chris AtLee [:catlee]]

That's what analysis.cfg.template is supposed to be.

Comment 73

[Chris AtLee [:catlee]]

In any case, the deployment process right now is to file a bug in releng (cc me) specifying which settings need to be added or modified.

Comment 74

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

Attachment: update scripts / tooling to support geomean (1.1)

fixed reference to data_type as a value instead of a string.

Comment 75

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

landed on graphs:
http://hg.mozilla.org/graphs/rev/4095308c115a

Comment 76

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

fixed a typo and verified an alert on tree-alerts:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-alerts/a7L7nh1PJlQ

caveat: this most likely will only show unique geomean alerts due to caching of the warnings to prevent duplication of alerts.  The talos alert sheriff team will look at both average/geomean on all alerts for the bake in period until we turn off the dev.tree-management average alerts.

Comment 77

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

Attachment: store the single value for average as geomean (1.0)

on 3 tests: tcanvasmark, v8_7, and kraken we upload a single value from talos:
http://hg.mozilla.org/build/talos/file/tip/talos/output.py#l253

these are of type 'AVERAGE', not 'VALUES'.  In the code to store a geometric mean, we only handle 'VALUES'.  This patch stores the single value as the geometric mean for the 'AVERAGE' inputs we get.

Comment 78

[Avi Halachmi (:avih)]

Comment on attachment 8537197 [details] [diff] [review]
store the single value for average as geomean (1.0)

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

Changing to f+ because I'm not familiar with the graphserver internals, but the fix looks valid. THanks!

Comment 79

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

http://hg.mozilla.org/graphs/rev/c6266505147e

:rhelmer, can you deploy this to the live server?

Comment 80

[Robert Helmer [:rhelmer]]

(In reply to Joel Maher (:jmaher) from comment #79)
> http://hg.mozilla.org/graphs/rev/c6266505147e
> 
> :rhelmer, can you deploy this to the live server?

Done in bug 1112387

Comment 81

[Ed Morley [:emorley]]

(In reply to Joel Maher (:jmaher) from comment #76)
> caveat: this most likely will only show unique geomean alerts due to caching
> of the warnings to prevent duplication of alerts.  The talos alert sheriff
> team will look at both average/geomean on all alerts for the bake in period
> until we turn off the dev.tree-management average alerts.

How are the new alerts looking? Are we good to switch off the ones sent to dev.tree-management now? :-)

Comment 82

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

yes, we are ready to do it, I will need to figure out a patch and coordinate with catlee on how to deploy it.  I verified the data, found an error a couple weeks ago, now all the data is good.

Comment 83

[Ed Morley [:emorley]]

That's great, thank you :-)

Comment 84

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

Attachment: run analysis for geomean as default (1.0)

This patch does a couple things:
1) sets geomean as the default on graph server
2) only runs one analysis, not two, so we will be posting to dev.tree-alerts
3) fixes the upload/reporting to use geomean and passively collect average

Once this is live we could eventually remove the average code.

Comment 85

[Chris AtLee [:catlee]]

Comment on attachment 8544026 [details] [diff] [review]
run analysis for geomean as default (1.0)

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

the regression analysis bits look fine to me. let me know when you want to deploy.

Comment 86

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

Comment on attachment 8544026 [details] [diff] [review]
run analysis for geomean as default (1.0)

via irc chat with rhelmer, he is fine with this patch and will help coordinate deployment.

Comment 87

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

checked in:
http://hg.mozilla.org/graphs/rev/f0a48c84d941

Next steps are to:
1) deploy changes to live server (:rhelmer)
2) update analyze_talos machine (:catlee)

I would like to do this in the same general time window.  Maybe Monday or Tuesday morning PST so :rhelmer can deploy and then in EST midday/afternoon :catlee can update the analyze talos script.

;rhelmer, :catlee- can you guys confirm this update/deployment time sounds good?

Comment 88

[Robert Helmer [:rhelmer]]

(In reply to Joel Maher (:jmaher) from comment #87)
> checked in:
> http://hg.mozilla.org/graphs/rev/f0a48c84d941
> 
> Next steps are to:
> 1) deploy changes to live server (:rhelmer)
> 2) update analyze_talos machine (:catlee)
> 
> I would like to do this in the same general time window.  Maybe Monday or
> Tuesday morning PST so :rhelmer can deploy and then in EST midday/afternoon
> :catlee can update the analyze talos script.
> 
> ;rhelmer, :catlee- can you guys confirm this update/deployment time sounds
> good?

WFM!

Comment 89

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

this was just deployed about 15 minutes ago.