1315808 - New Log Viewer integration

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect)

Comment 1

[GitHub Autolander Bot]

Attachment: [treeherder] helfi92:new-logviewer-integration > mozilla:master

Comment 2

[Jonathan French (:jfrench)]

Should the User Story for this bug be filled out? I see some bullet points at the top of the PR but no context/rationale anywhere for all the color, background color, font size and other changes.

Just my take given I did a bunch of the work to the log viewer at request of the engineers (and made it consciously similar to GitHub) I suspect there are many users who like the current GitHub style black on white line presentation with alternate row background for readability. At most (imo) there would be a toggle switch between its current appearance, an an inverted white on black presentation, and a stored cookie preference for it.

I also don't see any rationale presented for the font/line size changes which now consume more screen space?

Comment 3

[William Lachance (:wlach)]

(In reply to Jonathan French (:jfrench) from comment #2)
> Should the User Story for this bug be filled out? I see some bullet points
> at the top of the PR but no context/rationale anywhere for all the color,
> background color, font size and other changes.
> 
> Just my take given I did a bunch of the work to the log viewer at request of
> the engineers (and made it consciously similar to GitHub) I suspect there
> are many users who like the current GitHub style black on white line
> presentation with alternate row background for readability. At most (imo)
> there would be a toggle switch between its current appearance, an an
> inverted white on black presentation, and a stored cookie preference for it.
> 
> I also don't see any rationale presented for the font/line size changes
> which now consume more screen space?

Yeah, my intuition was that we should keep the styling the same absent a compelling reason to do otherwise. That the current scheme was decided on after consultation with developers just reinforces that sense.

Comment 4

[:Eli Perelman]

As stated in the PR, we created an API for the logviewer to pass in custom CSS in order to match the theme. Remember that the Unified Logviewer project is one that is shared across any site and is not exclusive to Treeherder. It drew inspiration from the Travis logviewer for styling. The default style scheme is the one in use on TaskCluster, so just passing in more CSS to customize should fix it. I just want it to be clear that the Unified Logviewer is agnostic to Treeherder and TaskCluster, so stylistic changes should be addressed through the customization API. 

Upon looking at the logviewer integration into Treeherder, I do find the line height a bit dense for reading, but that can be addressed in the future.

Comment 5

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:logviewer-integration > mozilla:master

Comment 6

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/bfa3def90f616bfc2c4f56005b19359736996a03
Bug 1315808 -  New log viewer (#2050)

* Written in react.js for speed
* Does not require custom treeherder backend API's
* Improved scrolling
* Fixes issues in URL when linking directly to line numbers

Comment 7

[William Lachance (:wlach)]

This is now merged onto stage!

Comment 8

[Ed Morley [:emorley]]

Once this sticks will we be removing the logslice API and things like the file based logs cache? This would fix things like bug 1277574 too :-)

Comment 9

[William Lachance (:wlach)]

(In reply to Ed Morley (Mostly away until 3rd Jan) [:emorley] from comment #8)
> Once this sticks will we be removing the logslice API and things like the
> file based logs cache? This would fix things like bug 1277574 too :-)

Yup, for sure! That's a nice side benefit of this work.

Comment 10

[Ed Morley [:emorley]]

It looks like this bug also fixed a pre-existing off by one error with the old log viewer, since logs now correctly start at line 1 not line zero (the Treeherder API has always returned a zero-based line number which isn't perhaps that intuitive, I've filed bug 1328455 to adjust).

Comment 13

[Ed Morley [:emorley]]

I've been playing around with the new log viewer - awesome work here!

A couple of things I did spot:

* Are there plans to host the unified log viewer somewhere other than github, for security/uptime/performance? eg: Github pages uses a max-age of 600s which is quite short (though I think they do honour If-Modified-Since, so it's only a round trip not a fill download)
* On page load the web console shows: "Failed to execute 'postMessage' on 'DOMWindow': The target origin provided ('https://taskcluster.github.io') does not match the recipient window's origin ('https://treeherder.allizom.org')."
* On page load the "Loading" spinner background is grey rather than white, so causes the page to flash once the log loads.

I've filed bug 1328469 for removing the now unused /logslice/ API which closes out a handful more bugs regarding it :-)

Comment 16

[Ed Morley [:emorley]]

I think it's worth creating a new Bugzilla Taskcluster::* component for the unified log viewer, so we have somewhere to move bugs filed against the Treeherder log viewer that are really about the iframe within.

Eli, would you mind picking a component name / making the other choices at:
https://wiki.mozilla.org/BMO/Requesting_Changes#Components

...and filing a bug at:
https://bugzilla.mozilla.org/enter_bug.cgi?product=bugzilla.mozilla.org&component=Administration

It may then likely be worth turning off GitHub issues for:
https://github.com/taskcluster/unified-logviewer

Thanks! :-)

Comment 19

[:Eli Perelman]

(In reply to Ed Morley [:emorley] from comment #13)
> * Are there plans to host the unified log viewer somewhere other than
> github, for security/uptime/performance? eg: Github pages uses a max-age of
> 600s which is quite short (though I think they do honour If-Modified-Since,
> so it's only a round trip not a fill download)

We haven't made any specific plans, but I'd be for hosting this on Heroku with nginx or something like that to improve serving and cacheability.

> * On page load the web console shows: "Failed to execute 'postMessage' on
> 'DOMWindow': The target origin provided ('https://taskcluster.github.io')
> does not match the recipient window's origin
> ('https://treeherder.allizom.org')."

The logviewer sends its message to the parent using "*" so any domain may accept. It should probably be double-checked that the Treeherder code either uses this or has the correct domain specified. If the domain changes (maybe if we move to Heroku), that will also mean a change to Treeherder code would be needed (if not using "*").

> * On page load the "Loading" spinner background is grey rather than white,
> so causes the page to flash once the log loads.

This is because the default theme for the logviewer is dark background and white foreground, which is what is in use on TaskCluster. Treeherder is using the postMessage API to pass in overriding styles, but this isn't done immediately. Until there is a better styling API, there will be a flash.

(In reply to Ed Morley [:emorley] from comment #16)
> I think it's worth creating a new Bugzilla Taskcluster::* component for the
> unified log viewer, so we have somewhere to move bugs filed against the
> Treeherder log viewer that are really about the iframe within.

This will be staying on GitHub since this project is TaskCluster/Treeherder agnostic, and will probably be moved off the TaskCluster GitHub org this quarter.

Thanks for the feedback!

Comment 20

[Ed Morley [:emorley]]

Thank you for the replies.

> This will be staying on GitHub since this project is TaskCluster/Treeherder agnostic, and will probably be moved off the TaskCluster GitHub org this quarter.

We really do need a Bugzilla component for this - I don't mind where it is, but I think it's not viable to just rely entirely on the GitHub issues for that project. For example:
* Someone files a bug against the Treeherder log viewer, but it's about the part in the iframe, however unless there's a Bugzilla component I can't then move the bug and have to close it and get them to file another one.
* If a Treeherder feature relies on a unified log viewer change, I can't add a bug dependency for the Treeherder bug on a GitHub issue.

If you really want to keep the GitHub issues section open in addition (and have the overhead of trying to keep them in sync or transpose things one way or another), then I won't fight that, but I think it's a must to have a Bugzilla component.

Comment 21

[:Eli Perelman]

Maybe it would help to think about the unified logviewer as any other vendor-based component, just like all the npm packages in use by Treeherder. Bugs in these packages do not each create their own component, but may have associated bugs in Bugzilla for tracking purposes. In this way, the unified logviewer is also an external component, but will not have a separate Bugzilla component.

Comment 22

[Ed Morley [:emorley]]

(In reply to Ed Morley [:emorley] from comment #13)
> * Are there plans to host the unified log viewer somewhere other than
> github, for security/uptime/performance? eg: Github pages uses a max-age of
> 600s which is quite short (though I think they do honour If-Modified-Since,
> so it's only a round trip not a fill download)

Filed:
https://github.com/taskcluster/unified-logviewer/issues/27
https://github.com/taskcluster/unified-logviewer/issues/28

> * On page load the web console shows: "Failed to execute 'postMessage' on
> 'DOMWindow': The target origin provided ('https://taskcluster.github.io')
> does not match the recipient window's origin
> ('https://treeherder.allizom.org')."

Filed bug 1328908.

Comment 23

[GitHub Autolander Bot]

Attachment: [treeherder] helfi92:modify-history-entry-without-creating-new-one > mozilla:master