1147963 - tc-tools: Copying text from log viewer (Inspect Task) has very poor UX

Status: RESOLVED FIXED

(Taskcluster :: UI, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Spun off from bug 1147867 comment #0.

No logs for a job, so I click on Inspect Task hoping to figure out what went wrong. See useful-looking error. Ctrl+C - doesn't work. Right click, copy - works. Holy useless whitespace.

Can we please improve this especially given bug 1147867 and Inspect Task is the only log we have.

Comment 1

[Jonas Finnemann Jensen (:jonasfj)]

At the moment the log viewer is not that great.
It uses a terminal emulator written for the browser (term.js).

One can still click the public/logs/live.log link under artifacts on the same page and view
it as plain/text in the browser.
 - But you won't get pretty colors :)

Anyways, I agree the in-browser log-viewer should be faster and more user friendly.

Comment 2

[James Lal [:lightsofapollo]]

Jonas can you find some hack around this? (I will take a shot if you don't have any ideas)

Comment 3

[Pete Moore [:pmoore][:pete]]

Is the log viewing in travis.org open source/reusable?

Comment 4

[Pete Moore [:pmoore][:pete]]

https://github.com/travis-ci/travis-logs
https://github.com/travis-ci/travis-web

Comment 5

[Pete Moore [:pmoore][:pete]]

http://blog.travis-ci.com/2013-08-08-solving-the-puzzle-of-scalable-log-processing/

Comment 6

[Jonas Finnemann Jensen (:jonasfj)]

I'm looking into a dirty hack...

@pmoore, feel free to investigate... We have a tty stream, currently we use term.js to display it. I've looked pretty far and wide for alternatives.

In some ways we want a view port so the browser isn't overloaded..

Comment 7

[Jonas Finnemann Jensen (:jonasfj)]

I pushed a fix for the inspector on docs.taskcluster.net to at least support ctrl+c.
It's a very very bad hack... Also I wasn't able to strip trailing whitespace.

Going forward maybe we build our own viewport and use:
https://www.npmjs.com/package/screen-buffer
I suspect using scrollBack: 50000 in term.js is the same as using resize(rows = 50000, cols) in
screen-buffer.
Though performance might be different and we would need to make our own viewport.

Comment 8

[Armen [:armenzg]]

Ryan: Has this improved?

What else would you like to see beyond the Ctrl+C issue?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Nothing's changed since I filed this.

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

This bug drives me nuts, too.  And ctrl-C still doesn't work despite the dirty hack.

I think the focus on terminal emulators is intended to get terminal colors right, but I don't think we're expecting anything else -- no cursor positioning or anything like that.  I think we can have both without resorting to any particularly obscene DOM tricks, by just interpreting the color codes.

Buildbot has a nicely performant version of this, based on the backend's ability to provide requested lines from a log -- so it just loads the tail to start, but sets an appropriate scroll range, and then updates as scrolling occurs.
  https://github.com/buildbot/buildbot/blob/master/www/base/src/app/builders/log/logviewer/logviewer.directive.coffee
it uses ui-scroll (https://github.com/angular-ui/ui-scroll) which is unfortunately an angular component.

https://github.com/seatgeek/react-infinite looks useful.

I'll play with this to see what I can make work.

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

react-infinite won't install on the latest node.  And since we get the output from the beginning anyway, maybe that's not worth worrying about.

https://github.com/drudru/ansi_up can help interpret the color codes.

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Interesting -- even a dumb 'ol "dump it in a <pre> tag" implementation has about a 10s pause for paint/layout on a full log.  Thankfully, the same thing happens with the existing implementation!

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

I got a neat bit of something working with react-infinite, just splitting the data by line and adding <p> children of the infinite scroller.  It loads in half the time of the existing, terminal-based view, without any pauses.  So that's great.  And it doesn't mess with keyboard focus.  However, it does copy/paste with extra whitespace, I suspect because of the way React formats text elements (judging by "show source", it adds a trailing newline).

Also, I've noticed that react-infinite will happily delete invisible elements even if they're selected, meaning that you can't copy/paste more than a screenful with this implementation.

So, remaining to do here:
 * fix copy/pasting
 * interpret ANSI color codes (I may need to write some stream-based implementation of ansi_up, if we're keeping with react-infinite)
 * automatic tailing, disabled after scrolling to a position other than the bottom

To be honest, the best fix may be to just chunk up the DOM modifications to avoid browser jank, starting with the end of the log and working backward.  That may mean inserting a series of blank, 1000-line <pre>'s, then filling those in one by one.

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

note to self, GBPU4QrLRLmXENpZmaY_1Q has some terminal colors (since nothing else really uses colors)

Comment 15

[Dustin J. Mitchell [:dustin] (he/him)]

Well, I tried writing out a series of blank <pre> with a fixed height (which should be pretty easy to lay out!), and that was still janky.

At Jonas's advice, I tried using CodeMirror, but it hangs browsers.

I'm going to shelve this one -- this is beyond my meager browser-hacking capabilities.

Comment 16

[Dustin J. Mitchell [:dustin] (he/him)]

Hang on, Treeherder's log viewer is super smooth.. let's kidnap one of their developers and make them tell us how they did it.  Or just look at the source.

Comment 17

[Jonas Finnemann Jensen (:jonasfj)]

treeherder seems to load lines as they are needed...
We already do this assume you wish to display the first part of the log initially, otherwise we could perhaps make a hack using "range" header to get the last 1kb of the log.. and then use "range" header
for each subsequent request after that.

My 2 cents says it's not the streaming part that is expensive, but the rendering.

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

You're right that it's the rendering -- but treeherder manages to do that part correctly, too.  I think we could build a component that loads the logs (from the beginning) and emits events as chunks of lines become available.  Then connect that to a separate component that initially just renders the visible lines, but fills in other lines as they become available.

The part I don't understand is how to do the latter smoothly -- since even the empty pre's were slow to render.

Comment 19

[Jonas Finnemann Jensen (:jonasfj)]

I took a crack at getting it to work with codemirror, stripping \r did the trick:
https://github.com/taskcluster/taskcluster-tools/tree/fix-log-viewer

We could go 3 ways now:
 1) strip-ansi escape sequences in a webworker (using workerify)
 2) stream ANSI log through ansi-html-stream (on a webworker), identify chunks and
    display with infinity.js
 3) Write a render for codeMirror that uses markText to hide ansi escape sequences and renders colors.

If we go with (1) it's possible that ace (brace) offers better performance.
If we go with (3) we should take inspiration from firepad which does rich text editing with codemirror.

Comment 20

[Dustin J. Mitchell [:dustin] (he/him)]

Stripping \r's, huh.  Was that causing the codemirror crashes?  Or was it causing the slowness?  Or both?

I haven't seen color in any of the logs I've been working with, so I'm tempted to say we should do (1) for now and leave an open bug for someone to do (2) or (3) later.

Comment 21

[Dustin J. Mitchell [:dustin] (he/him)]

(tries it) .. Ooh that's looking good!  Snappy loading, good copy/pasta, and decent scrolling on a long log.

Comment 22

[Jonas Finnemann Jensen (:jonasfj)]

> Stripping \r's, huh.  Was that causing the codemirror crashes?
Crashes in codemirror.

Comment 23

[Pete Moore [:pmoore][:pete]]

Also livelog view does not render on Safari on OS X. Instead, it says:

> [task-inspector] Failed to fetch log!

Note, I couldn't copy and paste that, I needed to type it by hand, lol :p

Comment 24

[Selena Deckelmann :selenamarie :selena]

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #19)
> I took a crack at getting it to work with codemirror, stripping \r did the
> trick:
> https://github.com/taskcluster/taskcluster-tools/tree/fix-log-viewer

>  1) strip-ansi escape sequences in a webworker (using workerify)

Can we merge this and open bugs to do further improvements later?

Comment 25

[Jonas Finnemann Jensen (:jonasfj)]

> Can we merge this and open bugs to do further improvements later?

I fear it might have bit-rotted a tiny bit after the bootstrap/react upgrades.
So it needs some love to land..

Comment 27

[[github robot]]

Commit pushed to master at https://github.com/taskcluster/taskcluster-tools

https://github.com/taskcluster/taskcluster-tools/commit/6aade50255dfbd5a14f1c7c95eeb187d6f71d967
Merge pull request #103 from andreadelrio/new-terminal

Bug 1147963 - [Fix] Log viewer (Inspect Task) has very poor UX