New Log Viewer integration

RESOLVED FIXED

Status

Tree Management
Treeherder: Log Viewer
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: hassan, Assigned: hassan)

Tracking

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)

Comment 1

a year ago
Created attachment 8808414 [details] [review]
[treeherder] helfi92:new-logviewer-integration > mozilla:master
Attachment #8808414 - Flags: feedback+
Assignee: nobody → helfi92
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?
Status: NEW → ASSIGNED
(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

a year ago
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

11 months ago
Created attachment 8822502 [details] [review]
[treeherder] wlach:logviewer-integration > mozilla:master

Comment 6

11 months ago
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
Attachment #8808414 - Flags: review+
This is now merged onto stage!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED

Comment 8

11 months ago
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 :-)
(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

11 months ago
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).

Updated

11 months ago
Duplicate of this bug: 1066734

Updated

11 months ago
Duplicate of this bug: 1209157

Updated

11 months ago
Blocks: 1328469

Comment 13

11 months ago
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 :-)

Updated

11 months ago
Duplicate of this bug: 1275909

Updated

11 months ago
Duplicate of this bug: 1275221

Comment 16

11 months ago
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! :-)
Flags: needinfo?(eperelman)

Updated

11 months ago
Duplicate of this bug: 1077807

Updated

11 months ago
Duplicate of this bug: 1059980

Updated

11 months ago
Blocks: 1275221

Comment 19

11 months ago
(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!
Flags: needinfo?(eperelman)

Comment 20

11 months ago
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

11 months ago
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.

Updated

11 months ago
Depends on: 1328710

Updated

11 months ago
Depends on: 1328908

Comment 22

11 months ago
(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.

Updated

11 months ago
Depends on: 1328880

Updated

11 months ago
Depends on: 1329735

Comment 23

11 months ago
Created attachment 8825883 [details] [review]
[treeherder] helfi92:modify-history-entry-without-creating-new-one > mozilla:master

Updated

11 months ago
Attachment #8825883 - Attachment is obsolete: true

Updated

11 months ago
Duplicate of this bug: 1282345

Updated

10 months ago
Duplicate of this bug: 1203716

Updated

10 months ago
Depends on: 1331190

Updated

10 months ago
Duplicate of this bug: 1087317
You need to log in before you can comment on or make changes to this bug.