Add failed_once category to Sheriff-O-Matic.

Add failed_once category to Sheriff-O-Matic.

Failures that have only happened once, and only on one bot should be classified
differently than other failures-- it's quite possible that they're flakes.

NOTRY=true
BUG=405241
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181478

[teravest, 2014-09-04 20:56:26]

teravest@chromium.org changed reviewers:
+ ojan@chromium.org

[teravest, 2014-09-04 20:56:27]

I added some text to make it clear which failures had only occurred once on one
bot.

I'm willing to get rid of the text if you want, but I felt there should be some
signal in addition to being sorted further down the page.

[ojan, 2014-09-04 22:23:33]

On 2014/09/04 at 20:56:27, teravest wrote:
> I added some text to make it clear which failures had only occurred once on
one bot.
> 
> I'm willing to get rid of the text if you want, but I felt there should be
some signal in addition to being sorted further down the page.

This is great. This was intended to be there from the beginning, but was left
out from the original patch. Thanks for fixing this. We'll probably want to
style it to make it a bit more prominent (e.g. bold + a slightly larger font
size). You can do that as part of this patch or as a followup or I can do it
later.

Either way, lgtm assuming you fix the excessive looping.

[teravest, 2014-09-05 00:37:15]

Any hint on the excessive looping? I need to poke around more to
understand the structures I guess, but I assume you're saying the
Object.keys() inside this.failures.map() is excessive.

Are you saying I should return once failures is greater than 1? Or am
I missing some better way to get at the failingBuildCount data?

On Thu, Sep 4, 2014 at 4:23 PM,  <ojan@chromium.org> wrote:
> On 2014/09/04 at 20:56:27, teravest wrote:
>>
>> I added some text to make it clear which failures had only occurred once
>> on
>
> one bot.
>
>> I'm willing to get rid of the text if you want, but I felt there should be
>
> some signal in addition to being sorted further down the page.
>
> This is great. This was intended to be there from the beginning, but was
> left
> out from the original patch. Thanks for fixing this. We'll probably want to
> style it to make it a bit more prominent (e.g. bold + a slightly larger font
> size). You can do that as part of this patch or as a followup or I can do it
> later.
>
> Either way, lgtm assuming you fix the excessive looping.
>
>
>
> https://codereview.chromium.org/546433003/

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[ojan, 2014-09-05 18:39:44]

Ugh. Rietveld didn't publish my comments! I wrote these in the original review.
:( Hopefully this answers your question.

https://codereview.chromium.org/546433003/diff/1/Tools/GardeningServer/model/...
File Tools/GardeningServer/model/ct-failure-group.html (right):

https://codereview.chromium.org/546433003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:51:
this.failures.map(function(f) {
In the common case, this is going to do a ton of work that it doesn't need to.
As soon as the totalFailures is > 1, we can bail out of both loops. How about
writing this as helper function (e.g. _failedOnce) that does a nested for loop
and early returns a boolean?

This is on my mind because we recently had 7600 failures on tip of tree and
sheriff-o-matic basically became unusable due to excessive, unnecessary looping.
:(

https://codereview.chromium.org/546433003/diff/1/Tools/GardeningServer/model/...
Tools/GardeningServer/model/ct-failure-group.html:58: this.category =
'failed_once';
Nit: We don't have many cases of string values like this, so there's no standard
practice exactly, but I slightly lean towards camelcase (i.e. failedOnce).

[teravest, 2014-09-05 19:02:28]

On 2014/09/05 18:39:44, ojan-only-code-yellow-reviews wrote:
> Ugh. Rietveld didn't publish my comments! I wrote these in the original
review.
> :( Hopefully this answers your question.

Thanks for following up!

> 
>
https://codereview.chromium.org/546433003/diff/1/Tools/GardeningServer/model/...
> File Tools/GardeningServer/model/ct-failure-group.html (right):
> 
>
https://codereview.chromium.org/546433003/diff/1/Tools/GardeningServer/model/...
> Tools/GardeningServer/model/ct-failure-group.html:51:
> this.failures.map(function(f) {
> In the common case, this is going to do a ton of work that it doesn't need to.
> As soon as the totalFailures is > 1, we can bail out of both loops. How about
> writing this as helper function (e.g. _failedOnce) that does a nested for loop
> and early returns a boolean?
> 
> This is on my mind because we recently had 7600 failures on tip of tree and
> sheriff-o-matic basically became unusable due to excessive, unnecessary
looping.
> :(

Sounds good. I've added a helper function and this should bail out of the loop
early now, when possible.

> 
>
https://codereview.chromium.org/546433003/diff/1/Tools/GardeningServer/model/...
> Tools/GardeningServer/model/ct-failure-group.html:58: this.category =
> 'failed_once';
> Nit: We don't have many cases of string values like this, so there's no
standard
> practice exactly, but I slightly lean towards camelcase (i.e. failedOnce).

Changed to camelcase.

[teravest, 2014-09-05 19:20:42]

Holding off on submitting this...

This was fine in manual testing before, but now I see slightly different output
than what's served on http://sheriff-o-matic.appspot.com/.

Investigating.

[teravest, 2014-09-05 19:24:36]

Never mind, I guess there's some weird caching happening locally. Works fine
from other profiles on Chrome (weird).

[teravest, 2014-09-05 19:24:41]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-09-05 19:25:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/546433003/20001

[commit-bot: I haz the power, 2014-09-05 19:26:00]

Committed patchset #2 (id:20001) as 181478