Closed
Bug 1108764
Opened 10 years ago
Closed 9 years ago
Allow easy sharing of links to a specific log line in the log viewer
Categories
(Tree Management :: Treeherder, enhancement, P3)
Tree Management
Treeherder
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: hwine, Assigned: goma, Mentored)
References
Details
(Whiteboard: [good next bug][lang=js])
User Story
Thank you for helping out with Treeherder! You can find us on IRC at irc://irc.mozilla.org/treeherder Here's some links to help get you started. Project page: https://wiki.mozilla.org/Auto-tools/Projects/Treeherder Interacting with us, repo locations and links to set up a development version of the software: https://wiki.mozilla.org/Auto-tools/Projects/Treeherder#Contributing https://wiki.mozilla.org/Auto-tools/Projects/Treeherder#Source_and_Docs A-Team general reference, coding style guides: http://ateam-bootcamp.readthedocs.org
Attachments
(2 files)
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•10 years ago
|
||
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"?
Priority: -- → P2
Summary: wanted: link to raw log snippets as displayed → Allow easy sharing of links to a specific log line in the log viewer
Updated•10 years ago
|
Priority: P2 → P3
Updated•10 years ago
|
Priority: P3 → P4
Comment 3•10 years ago
|
||
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.
Mentor: cdawson
User Story: (updated)
Whiteboard: [good next bug]
Updated•10 years ago
|
Whiteboard: [good next bug] → [good next bug][lang=js]
Updated•10 years ago
|
User Story: (updated)
Assignee | ||
Comment 5•9 years ago
|
||
I'd like to work on this bug.
Could you guys provide me more information about it?
What files should I look?
Flags: needinfo?(cdawson)
Comment 6•9 years ago
|
||
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•9 years ago
|
||
Goma-- Well... what he said. :) Thanks Jon, for answering the question. That sounds right to me as well.
Flags: needinfo?(cdawson)
Assignee | ||
Comment 8•9 years ago
|
||
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?
Flags: needinfo?(cdawson)
Comment 9•9 years ago
|
||
(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•9 years ago
|
||
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 :)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cdawson)
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
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!
Assignee: nobody → gbrmachado
Status: NEW → ASSIGNED
Priority: P4 → P3
Assignee | ||
Comment 13•9 years ago
|
||
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?
Flags: needinfo?(cdawson)
Comment 14•9 years ago
|
||
(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•9 years ago
|
||
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.
Flags: needinfo?(cdawson)
Assignee | ||
Comment 16•9 years ago
|
||
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.
Flags: needinfo?(cdawson)
Comment 17•9 years ago
|
||
(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
Assignee | ||
Comment 18•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
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•9 years ago
|
||
> 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!
Assignee | ||
Updated•9 years ago
|
Attachment #8660782 -
Flags: review?(cdawson)
Comment 21•9 years ago
|
||
Looks like the needinfo for me is already answered by jfrench. I'll move on to the review.
Flags: needinfo?(cdawson)
Comment 22•9 years ago
|
||
Sorry I didn't get to this review today. I will do so tomorrow after the Treeherder staff meeting.
Comment 23•9 years ago
|
||
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•9 years ago
|
||
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. :)
Attachment #8660782 -
Flags: review?(cdawson) → review+
Comment 25•9 years ago
|
||
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•9 years ago
|
||
Marking fixed per above merge. Thank you :goma! This should be on the stage/prod instances in the coming days.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Component: Treeherder: Log Viewer → TreeHerder
You need to log in
before you can comment on or make changes to this bug.
Description
•