1245760 - Display the failed buildbot step in the log summary pane if nothing else

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P2)

Description

[Mike Hommey [:glandium]]

Here is where this comes from: I got a build failure on try during the mock-install step, that looks like this:

https://s3-external-1.amazonaws.com/mozilla-releng-mock-centos-us-east-1/mirrors/centos/6/latest/os/x86_64/gtk2-devel-2.18.9-6.el6.centos.x86_64.rpm: [Errno 12] Timeout on https://s3-external-1.amazonaws.com/mozilla-releng-mock-centos-us-east-1/mirrors/centos/6/latest/os/x86_64/gtk2-devel-2.18.9-6.el6.centos.x86_64.rpm: (28, 'Operation too slow. Less than 1 bytes/sec transfered the last 30 seconds')

The failure summary on treeherder was:

"Failure summary is empty"

The log viewer actually even hides the buildbot step that failed, which makes it useless.

https://treeherder.mozilla.org/logviewer.html#?job_id=16311451&repo=try

So the only solution here is to go read the raw log, for an uninteresting infra error...

At the very least, it would have been useful if the failure summary had displayed the buildbot step that failed, even if the log parser doesn't know about the specific error. It happens really too often that the failure summary only has "Failure summary is empty" to say.

Comment 1

[Cameron Dawson [:camd]]

It looks like we are missing the regex to catch this particular error somehow.

Comment 2

[Phil Ringnalda (:philor)]

That's not what he filed this bug about, though: this bug is "When there is no parsed failure, the log viewer should open displaying the first buildbot step with a non-zero result, so I don't have to click on the yellow-background link to that step."

Personally, I'd go through an enormous number of bugs off that failure ("Buildbot should retry failed mock-install downloads", "Buildbot should have more than one mirror for mock-install to download from", "Buildbot should set a parseable error for mock-install download failures", "Buildbot should retry jobs on mock-install failures") before I got to this one, though.

Comment 3

[Cameron Dawson [:camd]]

Philor-- I can see there's more here than what I'd mentioned.  Thanks for clarifying.

Perhaps these could be the improvements here:

1. Select the line of the first non-success (these steps have result of "unknown" rather than fail, for some reason) step when LV opens (as you suggested)
2. Consider improving regexes to include this and other possible errors so the lines show in the Failure Summary panel
3. Include at least the title of the non-success steps in the Failure Summary panel, which are links to that step in the log-viewer (utilizing the new ``#L73`` syntax to jump to line 73 by default (for example).
4. Improve the text of "Unnamed step" to be more informative to that line.

It looks to me in that example that step 30 is the one with the result of 2, which indicates the error.  That and step 33 are not hidden for me at first.  It's probably not obvious that you can click the step to jump to that part of the log.  When I did that, I did find the text at the end of the step.  But the step is called "Unnamed step" which is non-helpful at best...

Comment 4

[Jonathan Griffin (:jgriffin)]

martianwars, is this something you'd be interested in tackling?

Comment 5

[Kalpesh Krishna [:martianwars]]

jgriffin, camd,
Yeah I think I would like to give this a shot. Could you which part of the codebase needs changes and a bit more on the first part of this bug?

Comment 6

[Cameron Dawson [:camd]]

Hey martianwars, thanks for taking this on!

So you might start here:https://github.com/mozilla/treeherder/blob/608f0f6beaa87e438ea58ea4518e531958a40789/ui/plugins/failure_summary/controller.js#L36-L36

That line is loading only the "Bug Suggestions" artifact.  But the artifact used by the log viewer is called "text_log_summary".  If you changed the param to ``get_list`` from ``name`` to be ``name__in`` and listed "Bug Summary,text_log_summary" then you should get both artifacts.  Then in the ``.then`` portion of that function, if the list of bug suggestions ends up empty, you could take a look in the ``text_log_summary`` for any log steps that have a result other than "success"

Here's a link to an example of the raw data we get from the AJAX call: https://treeherder.allizom.org/api/project/mozilla-inbound/artifact/?job_id=21919832&name__in=text_log_summary,Bug+suggestions

You could show the ``name`` and ``result`` field of each of those steps.  And make them a link to the log viewer with that line at the end of the URL with ``#L34`` type of syntax.

Comment 7

[Ed Morley [:emorley]]

I believe the bug is more about adjusting the UX of the log viewer, rather than the failure summary in the main UI (see comment 2). Not having an error summary is enough of an edge case, that I don't think it's worth the complexity and perf hit to also load the much bigger text_log_summary artifact for every failure summary panel invocation.

Comment 8

[Cameron Dawson [:camd]]

Well, I think it's both.  

Part 1
======
I was going based on this part of the initial description of this bug:

> At the very least, it would have been useful if the failure summary had displayed the buildbot step that 
> failed, even if the log parser doesn't know about the specific error. It happens really too often that 
> the failure summary only has "Failure summary is empty" to say.

That being said, I think making the change in BOTH places might be good.  But you make a good point about performance.  Perhaps a better approach for my description in comment 6 would be that, if you get no results from the "Bug suggestions" artifact, then fetch the "text_log_summary" to attempt to show *something* useful.  Or we could skip doing this in favor of the second part...

Part 2
======
However, it's true, that the log viewer is ALSO not doing the right thing in this case.  This is the line where we decide what to show when the log viewer first opens:
https://github.com/mozilla/treeherder/blob/fb70245edd22ca7c340dcb3d4860b99df8cb0ec6/ui/js/controllers/logviewer.js#L269-L269

I think we need some better logic in there to find a step that is not in ``all_errors`` but that would still make sense as the initial step to select.

Comment 9

[GitHub Autolander Bot]

Attachment: [treeherder] martiansideofthemoon:tyranitar > mozilla:master

Comment 10

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8734921 [details] [review]
[treeherder] martiansideofthemoon:tyranitar > mozilla:master

The log URL is wrong, it's the raw file and not the same in a logviewer. How could I rectify the same?
Also, the UI is a little shabby. Any suggestions to improve it?

Comment 11

[Cameron Dawson [:camd]]

Kalpesh: I added a couple suggestions in the PR.  Thanks for tackling this!

Comment 12

[Cameron Dawson [:camd]]

Comment on attachment 8734921 [details] [review]
[treeherder] martiansideofthemoon:tyranitar > mozilla:master

Made comments in the PR.

Comment 13

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8734921 [details] [review]
[treeherder] martiansideofthemoon:tyranitar > mozilla:master

Hey Cameron,
It works for me on this URL http://localhost:8000/#/jobs?repo=mozilla-inbound&revision=4391e6b0e891&selectedJob=21919832
I hope it is okay :)

Comment 14

[Cameron Dawson [:camd]]

I checked the time it took to fetch the Bug Suggestions only and the Bug Suggestions with the text_log_summary and it took about the same time, afiact.  A few milliseconds off here or there.  But probably not worth separating it to a second request, tbh.  60-ish ms either way.

Comment 15

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/71f787261496e9918a7446140f77ca97d08b071d
Bug 1245760 - Display the failed buildbot step in the log summary pane if nothing else.

https://github.com/mozilla/treeherder/commit/e8fe392477b842089e274adb963488ca58df965a
Merge pull request #1370 from martiansideofthemoon/tyranitar

Bug 1245760 - Display the failed buildbot step in the log summary pan…

Comment 16

[Cameron Dawson [:camd]]

The commit above addresses showing the unsuccessful log lines in the failure summary panel.  The next step is to address the log viewer itself.

thanks Kalpesh!!

Comment 17

[Kalpesh Krishna [:martianwars]]

Cameron,
I had fun :)
What sort of changes do I need in the Log Viewer? Which part of the code contains the log viewer.

Thanks! I hope to do this next part as well!

Comment 18

[Cameron Dawson [:camd]]

(In reply to Kalpesh Krishna [:martianwars] from comment #17)
> What sort of changes do I need in the Log Viewer? Which part of the code
> contains the log viewer.

Kalpesh: check out comment 8, Part 2.  That's the location to start looking.  :)

Comment 19

[Cameron Dawson [:camd]]

Kalpesh: also, check out philor's first paragraph in comment 2.  I think that's the other part that's desired in this bug.

Comment 20

[Kalpesh Krishna [:martianwars]]

Attachment: logviewer fix

Testing it using http://localhost:8000/logviewer.html#?job_id=16311451&repo=try

Comment 21

[Cameron Dawson [:camd]]

I left a comment in the PR.  :)

Comment 22

[Cameron Dawson [:camd]]

left a comment on the PR.  Heading in the right direction, but some of the behavior doesn't seem quite right yet.  Thanks!!  :)

Comment 23

[Cameron Dawson [:camd]]

Comment on attachment 8736486 [details] [review]
logviewer fix

Hey Kalpesh: I'm clearing this review rather than giving a + or - because I think you're close but not quite there.

Please reassign to me when you've made your adjustments.  And thanks!!!  :)

Comment 24

[Cameron Dawson [:camd]]

Hey Kalpesh: I left a comment in the PR again.  That one issue looks to be fixed, but I found another.  This is probably a similar fix, however.  So shouldn't be too tough.

Thanks!  :)

Comment 25

[Cameron Dawson [:camd]]

Comment on attachment 8736486 [details] [review]
logviewer fix

Kalpesh: I'm clearing the review flag again per my latest comment.  Please reassign to me when you've had a crack at a fix.

Thanks again.  :)

Comment 26

[Cameron Dawson [:camd]]

Comment on attachment 8736486 [details] [review]
logviewer fix

Clearing one last time so you can make a few more mods.  But you are SUPER close!  This should definitely be an r+ after these changes and then I'll merge it.

Please reassign when you've made the changes.  :)

Comment 27

[Cameron Dawson [:camd]]

Comment on attachment 8736486 [details] [review]
logviewer fix

Looks great!!  Thanks again for all your hard work!  I'll merge this momentarily.  :)

Comment 28

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/337e89145ece97068afec6de7d5052ddcaf3d445
Bug 1245760 - Moving to first non-success step on loading log viewer with text log summary. Also selecting complete set of lines on clicking failed steps.

https://github.com/mozilla/treeherder/commit/1998eb6e585048a31a8f06cf60054f85b85a6256
Merge pull request #1379 from martiansideofthemoon/cyndaquil

Bug 1245760 - Moving to first non-success step on loading log viewer with…

Comment 29

[Cameron Dawson [:camd]]

Merged!  Thanks for all your work!  Would you like to do the honors of marking this fixed?  :)