Bug 1315808 - New Log Viewer integration #1975

helfi92 · 2016-11-07T23:57:14Z

Main Features

Be able to highlight different error lines
Get rid of "double hashing" when linking to a specific line number
Smooth scrolling
Highlight single and multiple lines
Follow log (option to scroll to the bottom of the log)

This change is

helfi92 · 2016-11-08T00:47:49Z

I only commented the lines which were not making the code pass the linting tests. Most of the old implementation is still uncommented, but simply not used.

wlach · 2016-11-08T14:52:20Z

@helfi92 it looks like the ui tests are failing, could you have a look? http://treeherder.readthedocs.io/ui/installation.html#running-the-unit-tests

Also feel free to just delete any old code that is unused. I'd rather not have it sitting around in the repository, as it's going to confuse people.

helfi92 · 2016-11-08T15:44:10Z

@wlach Sounds good. I will delete any old code that is unused and then push a commit. By the way, grunt checkjs and ./tests/ui/scripts/test.sh both execute successfully. But when I push my commit, I get a Firefox 31.0.0 (Linux 0.0.0) ERROR. Does that mean I will need to download a Firefox version 31 in order to fix that issue locally?

wlach · 2016-11-08T16:00:47Z

@helfi92 you shouldn't, no. hmm. could you rebase on master and reinstall the node dependencies? @jgraham updated the karma firefox dep (which we use to start the browser) in b379a10

helfi92 · 2016-11-08T16:17:17Z

@wlach I did merge master with my branch in both of my commits and installed the new dependencies, so that's probably not the problem. I have a feeling that it might be happening because of me using ES6 features. Should I just change my code to be using ES5?

wlach · 2016-11-08T16:27:07Z

@helfi92 In theory es6 should be fine, we're already using features from it in various places. If you could try to figure it out that would be good, if you've tried and are still stuck, let me know and I can take a peek.

eliperelman · 2016-11-08T18:18:56Z

ui/js/controllers/logviewer.js

@@ -376,6 +401,21 @@ logViewerApp.controller('LogviewerCtrl', [
            });
        }

+        function displayErrorLines(errors) {
+            var css = errors
+              .reduce((line, err) => `${line}a[id="${err.line_number + 1}"]+span, `, '')


What does line mean here?

@eliperelman line contains the selector (e.g, a[id="1829"]+span, a[id="4490"]+span, a[id="4492"]+span, a[id="4538"]+span). Want me to rename it to selector

Oh derp, didn't notice the .reduce. Nevermind.

Combined with my other comment then:

errors .map(({ line_number }) => `a[id="${line_number + 1}"] + span`) .join(', ')

@eliperelman would it be smart to remove all the spaces to make the string smaller? No spaces between the + sign and after the comma ,

var css = errors .map(({ line_number }) => `a[id="${line_number + 1}"]+span`) .join(',')

eliperelman · 2016-11-08T18:19:06Z

ui/js/controllers/logviewer.js

+        function displayErrorLines(errors) {
+            var css = errors
+              .reduce((line, err) => `${line}a[id="${err.line_number + 1}"]+span, `, '')
+              .slice(0, -2)


What does the .slice here do?

@eliperelman it slices the last space character with the last comma. But now that you pointed this out, i can simply not add a space between each selector and simply slice the last comma before doing a concat with the style object. Thoughts?

~~Ah I see now. How about this?~~ See other comment.

errors .map((line, err) => `${line}a[id="${err.line_number + 1}"] + span`) .join(', ')

eliperelman · 2016-11-08T18:20:18Z

ui/js/directives/treeherder/log_viewer.js

+            var searchPart = () => {
+                var q = $location.search();
+
+                return "&highlightStart=" + q.highlightStart + "&highlightEnd=" + q.highlightEnd + "&lineNumber=" +


Can we not use template strings here?

@eliperelman Yes we can, but the literal will have a couple of backslashes since template literals records the newline character.

If it were me, I'd either prefer the slashes or array string-building over lots of concatenation. You could also get fancy since the key names match:

return [ "highlightStart", "highlightEnd", "lineNumber", "wrapLines", "showLineNumbers" ].reduce((qs, key) => `${qs}&${key}=${q[key]}`, '');

I like the array-building proposition. I'll use that one. Thanks!

helfi92 · 2016-11-08T18:34:33Z

@wlach I'm still stuck. It can't be the use of ES6, because in one of the files I was touching, there was already some usage of ES6. I don't know what other option could be causing it when I can't really reproduce the problem. Can you try take a quick look to see if you might know?

eliperelman · 2016-11-08T19:00:33Z

So, I there may be one issue here which is live logs. Right now the unified logviewer gives preference to non-live logs by attempting to load them as a binary arraybuffer first, and then gives up after 10 seconds and tries again as plain text. If we can know in Treeherder when loading the log whether it is still live or not, we can add a flag to the Unified Logviewer to skip loading as array buffer and go straight to text. Thoughts @helfi92 @wlach?

wlach · 2016-11-08T19:14:06Z

@helfi92 Everything passes here after rebasing on master. Could you push a branch rebased on master and see if that fixes travis?

P.S. To test locally you need to install a version of Firefox. It'll just use whatever version you have installed.

helfi92 · 2016-11-08T19:53:03Z

@eliperelman from my understanding, the current implementation of treeherder does not allow you to view live logs, only completed ones.

…r issue

helfi92 · 2016-11-08T21:38:10Z

@wlach I rebased on master but I am still getting the same errors. It doesn't show any errors on my machine though, just on travis

eliperelman · 2016-11-09T17:02:02Z

ui/js/controllers/logviewer.js

+            var css = errors
+              .map(({ line_number }) => `a[id="${line_number + 1}"]+span`)
+              .join(',')
+              .concat('{background-color:red;color:white}');


I also think that background-color: pink; color: red; has a more appealing look (for an error).

wlach · 2016-11-10T15:11:34Z

@helfi92 It just occured to me that the version of Firefox in the travis environment is rather old (31). Maybe there's an es6 feature it doesn't properly support or so.

Do you want to try upgrading it to the latest and see if it helps? Should be a minor tweak to .travis.yml in the root directory. https://docs.travis-ci.com/user/firefox/

helfi92 · 2016-11-10T16:01:48Z

@wlach That seems to be the fix. the ui-tests are now passing :) Thanks for taking a look and helping. I will go ahead now and delete the unused code from the old logviewer then it should be ready for review

helfi92 · 2016-11-17T14:53:28Z

@wlach I removed the unused code from the old log viewer. Code is now ready to reviewed

wlach · 2016-11-18T15:59:18Z

Haven't yet looked at the code in detail but a few problems I'm noticing in testing:

The highlighting looks wrong on my Linux nightly (notice same on Chrome):
Highlighting doesn't work at all if you enable line wrapping (which we probably want to enable by default, by the way)
It looks like the logviewer urls are now ridiculously long by default and expose a lot of internal state:

http://localhost:8000/logviewer.html#?job_id=39449331&repo=mozilla-inbound&highlightStart=&highlightEnd=&lineNumber=&wrapLines=false&showLineNumbers=&jumpToHighlight=false&followLog=false

These links are frequently shared so we should try to shorten them if at all possible. The default link should be something like http://localhost:8000/logviewer.html#?job_id=39449331&repo=mozilla-inbound. I think the only really relevant piece of extra state that we might also want to pass in is a directive to jump to a specific line number.
I don't think the "follow log" option is relevant by default for treeherder, could we configure it out somehow?

Overall though, this looks like a great step forward. Thanks for doing this awesome work!

…ecks if the element is a string. if it is, it will parse it to an object. this change is an attempt into making line jumping faster

…ssages

helfi92 · 2016-12-14T19:20:40Z

@wlach, what do you think about the new changes. I added some css to make it look like the current treeherder logviewer. Changes were pushed to gh-pages branch for quick access :)

wlach · 2016-12-19T16:39:11Z

@helfi92 this looks great! it seems more responsive now as well.

It seems like the hover effect for highlighting the error line quicklinks got broken though? Compare hovering over them here with your example: https://treeherder.mozilla.org/logviewer.html#?job_id=40233990&repo=mozilla-inbound&lineNumber=25195

I feel like functionally this is good enough to merge. I still need to review the code, will do so after the above is addressed.

helfi92 · 2016-12-22T03:54:06Z

@wlach i addressed your comments regarding highlighting. Let me know if there's anything else you want me to change in the ui. otherwise, it should be ready for review.

wlach · 2016-12-22T18:45:34Z

@helfi92 testing locally I'm still not seeing the hover effect on the error line links that I'm expecting. here's a screenshot of the current behaviour for clarity (note how line 28226 is highlighted)

wlach · 2016-12-22T18:45:52Z

(apologies if my previous comment was unclear)

wlach

The code all looks pretty good to me.

Could you try to switch the directive to a component? I don't think it should be too much effort. Other than that and the highlighting issue, I'd say we can land this soon.

wlach · 2016-12-22T18:51:23Z

ui/js/services/main.js

@@ -29,7 +29,7 @@ treeherder.factory('thUrl', [
                }, "");
            },
            getLogViewerUrl: function(job_id) {
-                return "logviewer.html#?job_id=" + job_id + "&repo=" + $rootScope.repoName;
+                return "logviewer.html#?job_id=" + job_id + "&repo=" + $rootScope.repoName + "&lineNumber=";


Is it actually necessary to specify &lineNumber= here?

you're right. i don't think it's necessary to specify that here. i'll remove it

wlach · 2016-12-22T18:54:27Z

ui/js/directives/treeherder/log_viewer.js

@@ -0,0 +1,34 @@
+'use strict';
+
+treeherder.directive('thLogViewer', ['$sce', '$location', ($sce, $location) => {


Since we're here, we might want to switch to using an angular component instead of a directive, which is a bit simpler... there's a few examples in ui/js/components/

helfi92 · 2016-12-23T05:08:46Z

@wlach Yes there was a misunderstanding. I thought you were referring to the log itself but now I see what you mean. Nevertheless, the log highlighting should now look exactly like the current implementation so that's at least good. I'll now work on the log viewer step highlight and change the directive to a component. Thanks for your comments :)

helfi92 · 2016-12-23T21:07:58Z

@wlach I switched the directive into a component and I made sure highlighting works.

wlach · 2016-12-28T18:40:10Z

@helfi92 Awesome! Looks great. Unfortunately I noticed one last feature that didn't make its way from the old logviewer implementation to the new. In the old one, it would auto-jump to the first failure line on load, this doesn't seem to be the case with the new one. E.g. compare when running a local instance:

http://localhost:8000/logviewer.html#?job_id=41099231&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=41099231&repo=mozilla-inbound

For reference, the original bug where this functionality was implemented was: https://bugzilla.mozilla.org/show_bug.cgi?id=1245760

After that's done, we can merge this into master for reals.

…o be using the on load event. behaviour is now as expected

helfi92 · 2016-12-29T04:44:56Z

@wlach My bad. I just realized when I made the transition from the directive to the component, the iframe's on load event wasn't firing at the right moment. I made the required change and it seems to be acting accordingly.

By the way, I noticed when you open a log at first, it adds a certain offset to the lineNumber but then upon clicking different error lines from the log viewer steps , it removes that offset and so line jumping acts like the current implementation of this PR. Is that a wanted behaviour? I am assuming the added offset was used so that a user sees the lines right before the error. Do you want me to add this feature or is this not needed anymore since there won't be any scrolling issues with the new logviewer.

wlach · 2016-12-29T20:02:04Z

I'm pretty sure the old behavior was just an accident of implementation, let's just leave it the way it is for now and see if people complain. :)

I think this is good to merge now! Thank you so much for your work on this. There are some conflicts with what's currently on master, so I'll fix that up first.

wlach · 2016-12-29T20:06:12Z

Created PR #2050 for actually merging this.

helfi92 changed the title ~~New Log Viewer Integration~~ Bug 1315808 - New Log Viewer integration Nov 7, 2016

eliperelman reviewed Nov 8, 2016

View reviewed changes

helfi92 added 5 commits November 8, 2016 15:18

Bug 1315808 - New Log Viewer integration

b267e32

remove logUrl from url

e9fd2d4

fix linting issues

d411730

change const to var because travis was giving errors

868f8ef

add eli suggestion and attempt to rebase on master to fix travis erro…

717eba4

…r issue

helfi92 force-pushed the new-logviewer-integration branch from d8f9e5b to 717eba4 Compare November 8, 2016 20:22

indent lint fix

1f5627a

eliperelman reviewed Nov 9, 2016

View reviewed changes

update travis to use latest firefox

34d5e1e

helfi92 added 2 commits November 16, 2016 20:56

remove unused code and add small nits

9906b5f

fix linting and merge with master

d5c41de

helfi92 added 4 commits December 14, 2016 11:54

unified logviewer sends a string instead of an object. this commit ch…

6691057

…ecks if the element is a string. if it is, it will parse it to an object. this change is an attempt into making line jumping faster

adjust font to be like current treeherder implementation

d820fc4

attempt to make log css load faster by reducing the number of post me…

91b26ef

…ssages

add space after if

51a046d

css perf

76a83b0

helfi92 added 4 commits December 21, 2016 16:45

make error highlighting like current treeherder logviewer

43ae07e

remove old commented code and clean logviewer css

e06c816

remove extra spaces from parentheses

0355152

make line number bold when you hover on top of it

ebb50d6

wlach reviewed Dec 22, 2016

View reviewed changes

helfi92 added 2 commits December 22, 2016 20:23

fix log viewer steps highlighting

1bbf0e9

remove lineNumber query from getLogViewerUrl method

be9a73e

helfi92 added 3 commits December 23, 2016 15:48

switch logviewer directive to component

07e48d1

re-add old css for log viewer step

dc516e9

component logviewer linting

07f2d79

iframe ng-init was not behaving like expected. changed ng-init prop t…

2b43597

…o be using the on load event. behaviour is now as expected

add spacing to old code

08e953a

wlach pushed a commit to wlach/treeherder that referenced this pull request Dec 29, 2016

Bug 1315808 - New Log Viewer integration (mozilla#1975)

74b9476

wlach closed this Dec 29, 2016

		@@ -0,0 +1,34 @@
		'use strict';

		treeherder.directive('thLogViewer', ['$sce', '$location', ($sce, $location) => {

Bug 1315808 - New Log Viewer integration #1975

Bug 1315808 - New Log Viewer integration #1975

Conversation

helfi92 commented Nov 7, 2016 • edited by camd

helfi92 commented Nov 8, 2016

wlach commented Nov 8, 2016

helfi92 commented Nov 8, 2016

wlach commented Nov 8, 2016

helfi92 commented Nov 8, 2016

wlach commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman Nov 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman Nov 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman Nov 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfi92 commented Nov 8, 2016

eliperelman commented Nov 8, 2016

wlach commented Nov 8, 2016

helfi92 commented Nov 8, 2016

helfi92 commented Nov 8, 2016

Choose a reason for hiding this comment

wlach commented Nov 10, 2016

helfi92 commented Nov 10, 2016

helfi92 commented Nov 17, 2016

wlach commented Nov 18, 2016 • edited

helfi92 commented Dec 14, 2016

wlach commented Dec 19, 2016

helfi92 commented Dec 22, 2016

wlach commented Dec 22, 2016

wlach commented Dec 22, 2016

wlach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfi92 commented Dec 23, 2016

helfi92 commented Dec 23, 2016

wlach commented Dec 28, 2016

helfi92 commented Dec 29, 2016

wlach commented Dec 29, 2016

wlach commented Dec 29, 2016

helfi92 commented Nov 7, 2016 •

edited by camd

eliperelman Nov 8, 2016 •

edited

eliperelman Nov 8, 2016 •

edited

eliperelman Nov 8, 2016 •

edited

wlach commented Nov 18, 2016 •

edited