Skip to content

Hass.io: Show ANSI color codes in logs#2155

Merged
balloob merged 6 commits intohome-assistant:devfrom
OttoWinter:hassio-ansi
Dec 2, 2018
Merged

Hass.io: Show ANSI color codes in logs#2155
balloob merged 6 commits intohome-assistant:devfrom
OttoWinter:hassio-ansi

Conversation

@OttoWinter
Copy link
Copy Markdown
Member

@OttoWinter OttoWinter commented Nov 30, 2018

Home-Assistant PR to forward ANSI color codes in HTTP view: home-assistant/core#18834

Show colors in Hass.io logs. Example:

Hass.io add-on

(source: https://gist.github.com/OttoWinter/88ec233c3bd714c00f86cb70018aa7af)

Hass.io supervisor logs

  • Obviously there's a lot of duplication going on between the supervisor and add-on logs; Of course these should be refactored into one class. However I don't know how to create such a class + I have no idea how imports work here 😅 Help is welcome!
  • One problem with refactoring is that the supervisor logs are slightly different. The supervisor uses green for almost all log messages (info), so green is converted to black in the supervisor logs.
  • The colors are not perfect yet. Especially the yellow color is a bit too bright to read. I spent some time trying to find a working color but ultimately failed. The color schema is based on the ubuntu colors here: https://en.wikipedia.org/wiki/ANSI_escape_code#Colors
  • Probably we could strip down the list of supported ANSI escape codes here. For the initial patch, I just went ahead and implemented all to see how it looks. Probably something like background isn't too necessary, but I'm sure somebody could make some creative design using that.

pre {
overflow-x: auto;
white-space: pre-wrap;
overflow-wrap: break-word;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enables wrapping in the logs. I personally find it easier to read but I can also remove it again

screen shot 2018-11-30 at 11 07 51

(before)

screen shot 2018-11-30 at 11 07 39

(after)

while (this.$.content.lastChild) {
this.$.content.removeChild(this.$.content.lastChild);
}
this.$.content.appendChild(HassioAddonLogs.parseLogsToPre(text));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.$.content.appendChild(HassioAddonLogs.parseLogsToPre(text));
this.$.content.appendChild(this.parseLogsToPre(text));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseLogsToPre is a static function.

},
() => {
this.log = "Error fetching logs";
while (this.$.content.lastChild) {
Copy link
Copy Markdown
Member

@bramkragten bramkragten Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to do all this, just put the error message in the div:

Suggested change
while (this.$.content.lastChild) {
this.$.content.innerHTML = "Error fetching logs";

while (this.$.content.lastChild) {
this.$.content.removeChild(this.$.content.lastChild);
}
this.$.content.appendChild(HassioSupervisorLog.parseLogsToPre(text));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.$.content.appendChild(HassioSupervisorLog.parseLogsToPre(text));
this.$.content.appendChild(this.parseLogsToPre(text));

@bramkragten
Copy link
Copy Markdown
Member

We should create a helper function to convert an ANSI log to HTML, that can be called inside these files.

@bramkragten
Copy link
Copy Markdown
Member

bramkragten commented Nov 30, 2018

One problem with refactoring is that the supervisor logs are slightly different. The supervisor uses green for almost all log messages (info), so green is converted to black in the supervisor logs.

I don't think this is a frontend problem, the supervisor or backend should fix that?

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Nov 30, 2018

I love the green output 👍

@OttoWinter
Copy link
Copy Markdown
Member Author

I love the green output 👍

screen shot 2018-11-30 at 14 33 58

I find this hard to read. Yes this is probably more of a frontend problem, though when viewing the supervisor docker logs it is nice to have the green text - for the frontend it is IMHO not so nice because readability goes down a lot.

We should create a helper function to convert an ANSI log to HTML, that can be called inside these files.

Yes, that's true. But I don't know how 😇 "However I don't know how to create such a class + I have no idea how imports work here" - Care to explain where such a file would be created (I can't find a util directory) and how to import it? Thanks!

@bramkragten
Copy link
Copy Markdown
Member

bramkragten commented Nov 30, 2018

For readability, you could make the background of the log screen dark, as most of those colors were designed for a dark background?

You should create a new file ansi-to-html.ts (in the root at this point I think...):

export function createHtml(text) {
  ...
  return formatted;
}

You can then import them like:

import { createHtml } from "../ansi-to-html";

You can then just use that function.

@OttoWinter
Copy link
Copy Markdown
Member Author

OttoWinter commented Nov 30, 2018

Thanks! ❤️

For the black background: yes for readability that would be ideal (also the way it's done in my base implementation, the esphomeyaml dashboard).
But I'm not so sure it looks good in the Hass.io dashboard. The Home Assistant frontend is mostly white and having a big black blob suddenly appear feels weird. Especially if it's in the middle of a white card. One option would be to make the whole card background dark, but that would also look quite weird I think.

Example:

screen shot 2018-11-30 at 14 50 32

screen shot 2018-11-30 at 15 15 55

@bramkragten
Copy link
Copy Markdown
Member

bramkragten commented Nov 30, 2018

I think it looks better, but the entire card should be dark.
I suggest using these colors:
image
https://github.com/carloscuesta/materialshell/tree/master/shell-color-themes#manual

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Nov 30, 2018

We have no control about the ASCII output. Home Assistant and Supervisor use python with ANSI color logging.

@frenck @balloob any suggestion?

@OttoWinter
Copy link
Copy Markdown
Member Author

@pvizeli If you're asking about the green INFO log level messages in the supervisor: That's quite simple, just remove this line

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Nov 30, 2018

Yes, we could do that. Currently, we follow Home Assistant style.

@OttoWinter
Copy link
Copy Markdown
Member Author

Yes, that was also my concern before. Speaking for myself, I like the green color output when viewing the docker logs in the terminal - especially when showing the logs from multiple containers at once.

For the frontend however I think the green output just hurts readability. Therefore my initial idea was to do remove the green INFO level in the frontend layer. That way we get nice colored logs in the terminal plus nice readable messages in the supervisor logs.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Nov 30, 2018

I think that sounds good. We can start and change things later if they are a problem.

${ANSI_HTML_STYLE}
<paper-card heading="Log">
<div class="card-content"><pre>[[log]]</pre></div>
<div class="card-content" id="content"></div>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<div class="card-content" id="content"></div>
<div class="card-content">[[_computeLog(log)]]</div>

With:

  _computeLog(log) {
    return parseTextToColoredPre(log);
  }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I misunderstand how the html function works, I believe this way would be quite inefficient.

parseTextToColoredPre parses the log and returns a DOM element. With the method that is implemented here, that generated DOM element must then just be appended - done. So API response -> DOM element (parseTextToColoredPre) -> append

With _computeLog however, the generated DOM tree would need to be serialized to HTML after parseTextToColoredPre and then be de-serialized again when it is inserted into the <template>. So API response -> DOM element (parseTextToColoredPre) -> Serialize -> Insert into <template> -> De-serialize by browser. Also, _computeLog would need to look something like this:

_computeLog(log) {
  return parseTextToColoredPre(log).outerHTML;
}

In the application where this code is taken from, this really became a performance problem with longer logs (I used innerHTML, but the speed would be similar). Especially once the Hass.io logs are streamed using websockets re-computing the entire log every time a new line appears would be quite inefficient IMHO.

Copy link
Copy Markdown
Member

@bramkragten bramkragten Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be right... let's use lit, that will fix it all :-P

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Polymer databinding doesn't allow setting HTML content, just text.

type: String,
observer: "addonSlugChanged",
},
log: String,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this

.then((info) => {
this.log = info;
.then((text) => {
while (this.$.content.lastChild) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
while (this.$.content.lastChild) {
this.log = text;

};

/* eslint-disable no-constant-condition */
while (true) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
while (true) {
let match;
while ((match = re.exec(text)) !== null) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@balloob balloob merged commit f7e3f4a into home-assistant:dev Dec 2, 2018
@ghost ghost removed the in progress label Dec 2, 2018
@OttoWinter OttoWinter deleted the hassio-ansi branch December 2, 2018 09:44
@balloob balloob mentioned this pull request Dec 5, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants