1200716 - Perfherder should use (only) full filtered values for subtest summaries

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

Right now perfherder ingests the subtest values produced from talos that are in some cases only partially filtered.

From the replicates for each subtest, we usually drop the first 5, then come up with a mean/median/max/min from that. Then there's the additional step of producing something called 'filtered', which in most cases is just the median, but in some cases is a custom calculation on the subtest to make it more meaningful (i.e. for v8_7, we run a subset of the suite summarization on the subtest summary). For these cases, the mean/median/max/min won't be very useful, since they'll be on a completely different scale. To make things even more confusing, we also re-store 'filtered' as 'value' for v8_7 and dromaeo. 

After discussing things with :jmaher and :avih, I think the way forward is to produced one single "subtest summary" value from Talos (what we currently called 'filtered') and (only) process that and store it in Perfherder. If people want data on a finer layer of granularity, they should just look at the raw replicate values from the test.

Here's where these numbers are produced in talos:

http://hg.mozilla.org/build/talos/file/548fa1f4ba2e/talos/results.py#l163

And processed in treeherder:

https://github.com/mozilla/treeherder/blob/master/treeherder/etl/perf_data_adapters.py#L80

Proposed way forward:

1. Modify perfherder to ingest "filtered" and (only) store that per test as a single object "value". This will entail rewriting all the existing subtests data (to extract out "median" and storing that as "value", where it doesn't exist already).
2. Modify talos to only produce "value" for subtests.

Comment 1

[William Lachance (:wlach)]

I'm going to take a pass at fixing this a bit sooner than expected (i.e. now) for perfherder at least, as it turns out that this change means we can get a way with using a slightly simpler data model, which will simplify bug 1192976 a bit.

Comment 2

[William Lachance (:wlach)]

Attachment: PR for perfherder implementation of fix

This fixes things on the perfherder side-- we probably eventually want to simplify things on the talos end as well (i.e. just producing a "value" summary value for subtests, instead of "filtered" and all the rest).

I've tested this locally in a vagrant environment and it works great.

Note that this will result in performance data that came before it to "disappear" from the graphs and compare view. It's not really gone, it's just not visible because old data doesn't have the "value" property. I considered writing a migration script, but it would be hard to create one that doesn't have race conditions. I think I'd prefer just to convert the old data over as part of the script in bug 1192976, which will be much easier (it's just a small modification to the conversion script I've written). We can then carefully stage landing this *just* before bug 1192976, so people won't know what hit them. ;)

Comment 4

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

Comment on attachment 8656151 [details] [review]
PR for perfherder implementation of fix

r+ with test files updated.

Comment 5

[Avi Halachmi (:avih)]

Comment on attachment 8656151 [details] [review]
PR for perfherder implementation of fix

Overall looks good to me.

TBH I'd think it'd been a simpler modification to just add a "filtered" field to all the min/max/etc values, and then later on use this field instead of using "median" (as I believe that's what's been used so far).

But it's fine as is as well.

Comment 6

[William Lachance (:wlach)]

(In reply to Avi Halachmi (:avih) from comment #5)
> TBH I'd think it'd been a simpler modification to just add a "filtered"
> field to all the min/max/etc values, and then later on use this field
> instead of using "median" (as I believe that's what's been used so far).

If you mean changing what we produce by talos, yes, I agree that's a better approach. We'll do that later, see the bug description.

Comment 7

[Avi Halachmi (:avih)]

NO, I meant at your patch. The patch doesn't touch the talos output, but the patch could maybe be simpler if it only added a "filtered" field and use that, while keeping all the other fields as is.

But I trust you to know better than me what's the best change is, code wise.

Comment 8

[William Lachance (:wlach)]

Attachment: Simplify talos output

This should remove a bunch of numbers we're not using from talos output, leaving only 'filtered' and 'value' (filtered is just for backwards compatibility with perfherder, to avoid chicken & egg problem where we can't update perfherder to accept value before talos adds it).

Comment 9

[William Lachance (:wlach)]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf586bdbd7d4

Comment 10

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

Comment on attachment 8660953 [details] [diff] [review]
Simplify talos output

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

this is great, although in this same area of code we could do the units of measurement as well.

Comment 11

[William Lachance (:wlach)]

Attachment: Updated commit message

Updated commit message, carrying over r+

Comment 12

[William Lachance (:wlach)]

See try run in comment 9. Also checked results in perfherder, this looks good to go:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=[try,bd72d04511c657c5c5040f1633fe73642fcdcb3b,1]&series=[try,6233768d21c48fe3ca4c494be32f40d9c00fdb46,1]&highlightedRevisions=cf586bdbd7d4

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a40d943a6ab9

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a40d943a6ab9