1108764 - Allow easy sharing of links to a specific log line in the log viewer

Status: VERIFIED FIXED

(Tree Management :: Treeherder, enhancement, P3)

Description

[Hal Wine [:hwine] use NI!]

Root cause analysis of some error messages requires someone else examining specific log snippets. Treeherder makes finding those snippets straight forward for a treeherder savvy user. 

Currently Treeherder doesn't provide a reference to a given portion of a raw log. If you are treeherder savvy, you can "re-navigate", but for non-devs that's somewhat harder or impossible. I often end up creating a lot of pastebins instead of being able to just put a URL in channel.

If possible, it'd be great to expose links to each selectable element in the upper right log entry pane.

The shortcoming of the "view raw log" for this use case is the difficulty of navigating to a specific line number in the raw log. E.g. line 1351 of 1424

Comment 1

[Ed Morley [:emorley]]

If I understand correctly, this bug is really "need to be able to permalink a specific log line (or range of lines) in the log viewer, even if that line isn't a failure"?

Comment 3

[Jonathan French (:jfrench)]

Marking this as a possible 'good next bug' (not necessarily a first bug).

Github's UX, appearance and link construction for a linked line, would be a likely candidate to mimic.

Comment 5

[Gabriel Machado [:goma]]

I'd like to work on this bug. 
Could you guys provide me more information about it?
What files should I look?

Comment 6

[Jonathan French (:jfrench)]

Hi Goma, this is where log lines get generated
https://github.com/mozilla/treeherder/search?utf8=%E2%9C%93&q=displayedLogLines

I would think per comment 3, we would like to replicate Github itself in both its path construction and it's UI/UX for selecting a log line(s). It seems quite well done. Unless there's some Moz tool we want to mimic, I defer to :camd on that.

eg. selecting a single line
https://github.com/mozilla/treeherder/blob/master/LICENSE.txt#L5

selecting a sequence of lines
https://github.com/mozilla/treeherder/blob/master/LICENSE.txt#L5-L9

Comment 7

[Cameron Dawson [:camd]]

Goma--  Well... what he said.  :)  Thanks Jon, for answering the question.  That sounds right to me as well.

Comment 8

[Gabriel Machado [:goma]]

Hey guys :)
I have some questions:
A) Could I change the error's highlight colour to red(instead of green), and use the green colour for the selected lines?
B) Should I implement the ctrl+click part too, like on github?

Comment 9

[Jonathan French (:jfrench)]

(In reply to Gabriel Machado [:goma] from comment #8)
> Hey guys :)
> I have some questions:
> A) Could I change the error's highlight colour to red(instead of green), and
> use the green colour for the selected lines?
> B) Should I implement the ctrl+click part too, like on github?

No we want the error line color to remain the same. This is standard bootstrap 'danger' aka. error red.

I'm not sure I understand what ctrl+click does on github? For logviewer we just want the user to be able to click on an adjacent line number in a new line column, have the line background switch to the same github color (#F8EEC7), and apply that selected line number or line-range to the url.

Comment 10

[Jonathan French (:jfrench)]

Attachment: specLikeGithub

Like this Github appearance.

We will also want to accommodate large line numbers in the column, if the user loads a massively long file. But have a more modest width by default.

I haven't tried or dug around for huge files on Github, but if you can't find any you could probably check in some million-line file into some scratch repo in your own remote somewhere and see how they handle that :)

Comment 11

[(not currently active) Ted Mielczarek]

(In reply to Hal Wine [:hwine] (Out until Sep 4 - use NI) from comment #0)
> If possible, it'd be great to expose links to each selectable element in the
> upper right log entry pane.

Calling this bit from comment 0 out to make sure it's addressed, I could use this frequently. I'd like to be able to click on one of the errors in the error summary and have that add a hash to the URL in the URL bar so that I could then copy and paste the URL to share a link to a specific error with someone.

Comment 12

[Jonathan French (:jfrench)]

Gabriel would like to have a go at this, so assigning. Thank you :-) Feel free to chat with us in channel if you have questions!

Comment 13

[Gabriel Machado [:goma]]

Hey guys. I'm doing the last part, the redirection to a line. I'd like to know how exactly should be the behaviour for it. 
For example, accessing:
http://treeherder.mozilla.org/logviewer.html#?job_id=13785238&repo=mozilla-inbound#L2797-L2808

Should the first line on logviewer be the line 2797? Or should the logviewer put the line 2797 on the middle of the screen, like on github?

Comment 14

[Jonathan French (:jfrench)]

(In reply to Gabriel Machado [:goma] from comment #13)
> Hey guys. I'm doing the last part, the redirection to a line. I'd like to
> know how exactly should be the behaviour for it. 
> For example, accessing:
> http://treeherder.mozilla.org/logviewer.html#?job_id=13785238&repo=mozilla-
> inbound#L2797-L2808
> 
> Should the first line on logviewer be the line 2797? Or should the logviewer
> put the line 2797 on the middle of the screen, like on github?

Thanks :goma! Yes I think mid-screen would be fine if that's easy to do? This would help provide viewing context for lines above and below the link.

Unrelated: if that mid-point scrolllTo spans two log chunks, I think we'd also load and handle that fine.

Github looks like they scroll slightly higher than mid-screen, when viewing a L(x) link. Maybe we could do similar? I'm not sure what their percentage is, but I agree with that logic.

Comment 15

[Cameron Dawson [:camd]]

Yeah, I think either one would work.  Mid-screen sounds nice.  Or perhaps 1/3 way down.  So 1/3 before, 2/3 after?  But that's just nit-picking.  As long as the selected line is highlighted so your eye jumps to it, it'll be great.

Comment 16

[Gabriel Machado [:goma]]

Hey guys. 
About the redirection, I have a question.
It seems that the redirection has being doing by this part here:
https://github.com/mozilla/treeherder/blob/master/ui/partials/logviewer/lvLogSteps.html#L24

However, how exactly the page redirect automatically to the first error line when the page is loaded?

I mean, as this code is a ng-click, I think that it should redirects just when the button is clicked.

Comment 17

[Jonathan French (:jfrench)]

(In reply to Gabriel Machado [:goma] from comment #16)
> 
> However, how exactly the page redirect automatically to the first error line
> when the page is loaded?

Thanks for asking :goma, yeah that preload behavior is done mostly in logviewer.js here
https://github.com/mozilla/treeherder/blob/master/ui/js/controllers/logviewer.js#L221-L236

It was part of this PR landed to load the first failure line, if one exists in the log artifact.
https://github.com/mozilla/treeherder/pull/828/files

Unrelated, but when you get a chance if you could add your Github PR as a text attachment to this bug? There's a workflow describing how to do that on the ATeam bootcamp RTD. It's the fifth bullet point under the 'Git and Github' section. Thanks!
https://ateam-bootcamp.readthedocs.org/en/latest/guide/development_process.html

Comment 18

[Gabriel Machado [:goma]]

Attachment: Link to Github Pull Request

Comment 19

[Gabriel Machado [:goma]]

Thanks :jfrench. I've already added the Github PR :)

Another question:
I'm doing the redirection using the function defined here:
https://github.com/gbrmachado/treeherder/blob/Bug1108764/ui/js/directives/treeherder/log_viewer_steps.js#L18-L38

So, basically, what I'm doing to redirect the page:
https://github.com/gbrmachado/treeherder/blob/ca0878feac28a159f2bdc74979c72a25c0a91dbf/ui/js/controllers/logviewer.js#L254-L261
(My understanding is: If there is a hash value, the site will redirect to the line defined in the hash, otherwise, it will redirect to the first error line.)
obs: this steps[2] is just a workaround, I'll fix it :P

However, although the site is working fine, I'm getting an error: $event.stopPropagation is not a function.
How exactly should I deal with this error? Should I create a new function without this stopPropagation??

Comment 20

[Jonathan French (:jfrench)]

> If there is a hash value, the site will redirect to the line defined in the hash, otherwise, it will redirect to the first error line.

Yes, that's the desired behavior. We still want the pre-load and scrollTo to an error line if one exists, or a pre-load and scrollTo to the first line of the first step, if no error exists.

I just tried your latest branch locally :goma, awesome work so far!

Let's take the error part of the discussion to the Github PR if that's ok? (that's where we typically get into the specifics of the PR changes). If you can also edit the PR attachment you made for the bug and add r? to cameron dawson (camd). He'll do the core part of the review. I'll have a quick look to see if I see anything now w.r.t. the stopPropagation error, or other things.

Thank you again, this is a great improvement!

Comment 21

[Cameron Dawson [:camd]]

Looks like the needinfo for me is already answered by jfrench.  I'll move on to the review.

Comment 22

[Cameron Dawson [:camd]]

Sorry I didn't get to this review today.  I will do so tomorrow after the Treeherder staff meeting.

Comment 23

[Cameron Dawson [:camd]]

I reviewed it and commented in the PR.  The code so far looks like an r+.  But there was two scenarios that don't seem to be covered yet.

Nice work so far!!

Comment 24

[Cameron Dawson [:camd]]

Comment on attachment 8660782 [details] [review]
Link to Github Pull Request

This looks good to me.  We just need to squash those commits and should be good to go.  If it passes jfrench's testing.  :)

Comment 25

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/4b1c450b95a23fb0bb47d42a5a5716a19d05acaa
Bug 1108764 - Add line highlighting and line linking in logviewer

https://github.com/mozilla/treeherder/commit/5af5d2e2b837d5a48a32e9e0ff11396448b00280
Merge pull request #948 from gbrmachado/Bug1108764

Bug 1108764 - Allow easy sharing of links to a specific log line in the log viewer

Comment 26

[Jonathan French (:jfrench)]

Marking fixed per above merge. Thank you :goma! This should be on the stage/prod instances in the coming days.

Comment 27

[Jonathan French (:jfrench)]

Verified fixed on stage.