Closed Bug 1108764 Opened 6 years ago Closed 5 years ago
Allow easy sharing of links to a specific log line in the log viewer
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
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
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
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.
User Story: (updated)
Whiteboard: [good next bug]
I'd like to work on this bug. Could you guys provide me more information about it? What files should I look?
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
Goma-- Well... what he said. :) Thanks Jon, for answering the question. That sounds right to me as well.
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?
(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.
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 :)
(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.
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
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?
(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.
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.
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.
(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
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 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??
> 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!
Attachment #8660782 - Flags: review?(cdawson)
Looks like the needinfo for me is already answered by jfrench. I'll move on to the review.
Sorry I didn't get to this review today. I will do so tomorrow after the Treeherder staff meeting.
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 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+
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
Marking fixed per above merge. Thank you :goma! This should be on the stage/prod instances in the coming days.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Verified fixed on stage.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.