1279591 - Perfherder ignores test results with "0" value in compare view

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

According to the comments, we do this because "Talos tests may output 0 as an indication of failure". I have never personally seen this, and this behavior caused confusion in bug 1279292. I'm going to suggest we just display/include these new values in the compare view.

Comment 1

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1279591 > mozilla:master

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8762141 [details] [review]
[treeherder] wlach:1279591 > mozilla:master

This makes the null / 0 values in:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b1047ee23ca1&newProject=try&newRevision=b1047ee23ca1&originalSignature=12ea55b0c0e0b98c6b70a890fdc51244b7604e89&newSignature=12ea55b0c0e0b98c6b70a890fdc51244b7604e89&framework=1

display correctly (e.g. tscrollx tiled-fixed.html.CSSOM.checkerboard opt e10s).

Am I missing something or do we really need to exclude 0 results from talos in some cases? I don't recall putting this check in the compare view so I'm guessing it was one of you.

Comment 3

[Avi Halachmi (:avih)]

Good question. I do recall such notion but I don't recall specific cases it was meant to address.

Joel doesn't recall either, and on IRC we agreed that unless it makes everything explode, showing 0 even if it indicates an error is actually a good thing - this way we can notice it and try to do something about it.

Comment 4

[Avi Halachmi (:avih)]

Comment on attachment 8762141 [details] [review]
[treeherder] wlach:1279591 > mozilla:master

I do have one minor concern though, and it's that if we do have subtests which report 0, no one would notice them unless they open the subtests page - but the overall test score would seem magically better.

Let's hope this doesn't happen, and that if it does, we catch that on time.

Comment 5

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 6

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/6fd2ed2ab6af63ea567468e29d7ace327065e1f3
Bug 1279591 - Include 0 / null values in Perfherder compare view (#1585)

This was intended to guard against Talos outputting 0 values in the case of
error, but I don't believe that happens anymore (if it ever did?)

Comment 7

[William Lachance (:wlach)]

(not sure what's going on with autolander... merged manually)

Comment 8

[Avi Halachmi (:avih)]

I just recalled another thing. Geometric mean is not great with 0, and it's applied to subtests on more than one place I think...

We do have workaround in some places where we add 1 to all values, calculate the geomean, then reduce 1 from the result, but I don't recall where this workaround is deployed, or that I even know all the places it could bite us.

But as wlach said on IRC, it would make it more likely to make us notice issues sooner...