1323536 - Make text field for annotating jobs mandatory when using "fixed by commit"

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect)

Description

[Armen [:armenzg]]

sheriffs: are you OK with preventing annotating "fixed by commit" with the empty string?

SETA analyzes sets of jobs that are annotated with "fixed by commit".

In the last 90 days [1] we had 11,380 jobs annotated with the empty string.
I still don't know in which ways this can affect SETA's analysis, however, I would like to improve this a bit.

Asking that the drop down show you the revisions for a tree might be ideal but for now I would like to prevent using the empty string. I can understand this being a bit of a pain specially with so much annotation going on, however, I hope this will improve SETA's analysis and make less mistakes. I also hope the current Stockwell initiative will reduce starring.

[1] http://seta-dev.herokuapp.com/data/seta/

Comment 1

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

as a note, stockwell shouldn't reduce the fixed by commit messages- but I would like to see us make this the rare case where we have incorrect/incomplete data.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Not sure how I feel about this. On one hand, yes, having more data is nice. On the other hand, do we know that this is actually causing problems now? Why does SETA care whether a reason is given or not?

Comment 3

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

this actually makes SETA very useless- it seems that the last 6 months we have had a large spike in these |fixed_by_commit=''| messages.

The way SETA works is that it looks for all the unique fixed_by_commit values and builds a list:
fixed_by_commit = {
'reason1': [job1, job2, job3, ...]
'reason2': [job1, job2, job3, ...]
...
'reason790': [jobX, jobY, jobZ, ...]
}

This is a simple list of all the reasons (unique fixed_by_commit values).  Most have between 1 and 100 jobs which failed, but for the case of reason='', we have 10000+ jobs.  Imagine if we had CPP tests failing a few times for a few different revisions, but not in another other ones- then imagine if those CPP tests were annotated as fixed_by_commit=''.  We would effectively have every unique job in the 10000+ instances and we would programatically reduce the jobs to ignore all CPP failures because they were in the '' reason but we determined that mochitest-1 would catch all failures in '' and it would catch failures in 100 other reasons.

I know it is slightly more work, but it is sort of pointless to have this field if we don't actually use it.  Prior to this field, sheriffs would put hg revisions in the bugid field when starring a failure, I would parse all the non-green jobs and if the bug id was a revision I would use those for the reasons.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Tens of thousands sounds insanely high. Do we have data on where they're all coming from?

Comment 5

[Armen [:armenzg]]

KWierso: did you mention you had a solution for this? I can't remember what we discussed.

Comment 6

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

(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #5)
> KWierso: did you mention you had a solution for this? I can't remember what
> we discussed.

Requiring <anything> in the classification comment when the classification type is "fixed by commit" would be an easy one-liner in https://github.com/mozilla/treeherder/blob/master/ui/plugins/pinboard.js#L143 

(We already require that the classification comment be non-empty for the "intermittent" classification type, so we'd just need to expand that to the "fixed by commit" type.)

Doing better than that (requiring 12/40 char sha, or a url to a backout commit on another branch) would be a bit more work, but doable, likely in that same function.

Comment 7

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

Armen, Joel: Would the "require anything" option be sufficient?

We could do something stricter like requiring a 12/40 char sha and/or a URL to a mercurial commit, or maybe do something fancier like having it so when you select "Fixed by commit" it pops up a list of in-view commits that have been identified as backouts/followups that you can choose from, but those are all things that'll take longer than just making it require something be typed into the comment field.

Comment 8

[William Lachance (:wlach)]

(In reply to Wes Kocher (:KWierso) from comment #7)
> Armen, Joel: Would the "require anything" option be sufficient?
> 
> We could do something stricter like requiring a 12/40 char sha and/or a URL
> to a mercurial commit, or maybe do something fancier like having it so when
> you select "Fixed by commit" it pops up a list of in-view commits that have
> been identified as backouts/followups that you can choose from, but those
> are all things that'll take longer than just making it require something be
> typed into the comment field.

I think it's ok to start by just doing a simple frontend change to require a minimum length string at first (https://www.w3.org/TR/html5/forms.html#the-maxlength-and-minlength-attributes), then implement more advanced validation (either on the frontend or backend, or both) later.

Comment 9

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

I really like the idea of a list of +- 20 revisions, possibly annotated/highlighted to indicate ones that are backouts/fixes.

Using what :wlach indicated as a min/max seems like a reasonable step 1 :)

Alisha, I know you had expressed interest in working on treeherder, this might be a good bug to start with, specifically what Will mentioned in comment 8 by using min/max for the html form.

Here are some docs for setting up treeherder:
http://treeherder.readthedocs.io/

In addition, you would only need to setup the frontend, not the full database- so setup/development should be easier.

Comment 10

[Armen [:armenzg]]

wlach and jmaher covered it all. I have nothing else to add. Thanks KWierso!

Comment 11

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

And for the record for whoever ends up working on this, "fixed by commit" would have thisClass.failure_classification_id equal 2, as defined in https://github.com/mozilla/treeherder/blob/0300fb69bad7223585fd9e9c885b5a159b292221/treeherder/model/fixtures/failure_classification.json#L10

Comment 12

[anejaalisha]

(In reply to Joel Maher ( :jmaher) from comment #9)
> I really like the idea of a list of +- 20 revisions, possibly
> annotated/highlighted to indicate ones that are backouts/fixes.
> 
> Using what :wlach indicated as a min/max seems like a reasonable step 1 :)
> 
> Alisha, I know you had expressed interest in working on treeherder, this
> might be a good bug to start with, specifically what Will mentioned in
> comment 8 by using min/max for the html form.
> 
> Here are some docs for setting up treeherder:
> http://treeherder.readthedocs.io/
> 
> In addition, you would only need to setup the frontend, not the full
> database- so setup/development should be easier.


Okay. I will setup and start working on this.

Comment 13

[anejaalisha]

Link to github pull request: https://github.com/mozilla/treeherder/pull/2062

Comment 14

[GitHub Autolander Bot]

Attachment: [treeherder] alisha17:bug1323536-Make-text-field-for-annotating-jobs-mandatory-when-using_fixedbycommit > mozilla:master

Comment 15

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

I am curious with this patch what type of error shows up when you do not have 8 characters?  I like the simple change in the PR, as long it is clear what someone would need to do in order to use this field when they forget the revision, we should move forward with this.

Comment 16

[anejaalisha]

(In reply to Joel Maher ( :jmaher) from comment #15)
> I am curious with this patch what type of error shows up when you do not
> have 8 characters?  I like the simple change in the PR, as long it is clear
> what someone would need to do in order to use this field when they forget
> the revision, we should move forward with this.

In case the length of text will be less than 8 characters, it will display a message which will tell the user to input at least 8 characters. minlength attribute only works once the user has entered a value at all. The required attribute ensures that empty string is not allowed.

Comment 17

[William Lachance (:wlach)]

Comment on attachment 8824045 [details] [review]
[treeherder] alisha17:bug1323536-Make-text-field-for-annotating-jobs-mandatory-when-using_fixedbycommit > mozilla:master

Do you want to have a look at this Wes?

Comment 18

[William Lachance (:wlach)]

(In reply to anejaalisha from comment #13)
> Link to github pull request: https://github.com/mozilla/treeherder/pull/2062

Just for future reference, there's no need to link to the PR in the bug. The autolander bot will take care of this step automatically.

Comment 19

[anejaalisha]

(In reply to William Lachance (:wlach) from comment #18)
> (In reply to anejaalisha from comment #13)
> > Link to github pull request: https://github.com/mozilla/treeherder/pull/2062
> 
> Just for future reference, there's no need to link to the PR in the bug. The
> autolander bot will take care of this step automatically.

Sure. Will keep that in mind for the next time.

Comment 20

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

Comment on attachment 8824045 [details] [review]
[treeherder] alisha17:bug1323536-Make-text-field-for-annotating-jobs-mandatory-when-using_fixedbycommit > mozilla:master

Put some comments in the PR.

Comment 21

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

for options on populating this with suggestions, we have prior art in perfherder compare:
https://github.com/mozilla/treeherder/blob/0bb3d96db44d09aa8221b0c953d0b648abe992dc/ui/js/controllers/perf/compare.js#L31

and the actual UI:
https://treeherder.allizom.org/perf.html#/comparechooser

Comment 22

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

I ran out of time to test this out today. NIing myself to remind me to check in on monday.

Comment 23

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

Comment on attachment 8824045 [details] [review]
[treeherder] alisha17:bug1323536-Make-text-field-for-annotating-jobs-mandatory-when-using_fixedbycommit > mozilla:master

Works nice.

Comment 24

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/b9622d78f107677cce7d5680375f294973363049
Bug 1323536 - Make text field for annotating jobs mandatory when using fixed by commit (#2062) r=kwierso

With the "fixed by commit" classification type, the classification comment needs to be at least 8 characters.

Comment 25

[GitHub Autolander Bot]

Attachment: [treeherder] alisha17:bug1323536-Make-text-field-for-annotating-jobs-mandatory-when-using-fixedbycommit > mozilla:master

Comment 26

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

I would suggest splitting this work off into a new bug, since this one is already marked fixed.

Comment 27

[anejaalisha]

Created a new bug for further work on this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1333396#c0