Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration option to disable the log UI. #4419

Merged
merged 5 commits into from Jun 21, 2023

Conversation

dd32
Copy link
Contributor

@dd32 dd32 commented Feb 12, 2023

What does this implement/fix?

This provides a configuration option to allow hiding the log from the UI by default.
It does not alter the websocket stream, so logs are still accessible, this only affects the UI.
This should not be considered to provide any security, only a more non-tech UI when more appropriate.

A PR for the Javascript for this is available here: esphome/esphome-webserver#28

There's two changes here:

  • Implementation of the option (Default: Log enabled)
  • Addition of a "preload" of the configuration inline on the index page output.

The second is used to allow the base configuration of the UI to be aware of the options ahead of first render, so that it can either show or hide the Log UI without a paint refresh.
This is seemingly very not-required for local LAN access, but when ESPHome is behind a proxy the websocket delay can be significant and delay loading both the Title and configuration of which UI elements should be visible. (Note: The Vite development environment for esphome/esphome-webserver is a good example of this, but importantly, this preloading isn't enabled there as it serves a local index, rather than proxying the index from the esphome device.)

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes esphome/feature-requests/issues/2025

Pull request in esphome-docs with documentation (if applicable): PR Will be added if maintainers indicate feature is to be accepted.

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040

I have not tested with ESP8266 but can if requested.

Example entry for config.yaml:

# Example config.yaml
web_server:
  log: false

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

If we are going to do this, I would like to see the callback below not even subscribed to so the logs are not sent via the websocket to reduce network load.

Ideally, the frontend should be able to just subscribe to logs so they are not sent unless requested.

A config option like the one you have added could be used as the default by the frontend as to whether it should subscribe or not when opened.

@dd32
Copy link
Contributor Author

dd32 commented Feb 15, 2023

Thanks for the review @jesserockz!

I would like to see the callback below not even subscribed to so the logs are not sent via the websocket to reduce network load.

I completely missed that following logger call, I hadn't dug very deep to figure out where it came from..
I've updated it above to skip sending logs via the websocket when disabled.

I also wasn't fully aware of how the esphome native api logs vs web_server event stream differed; obvious in retrospect.

Ideally, the frontend should be able to just subscribe to logs so they are not sent unless requested.

I read this as, "Ideally the frontend should just be able to subscribe to /events?logs=false" as in, the server component doesn't necesarily need to include the logic, the connected client should specify it at connect time.

Unfortunately, I can't see any easy way to achieve that with the current AsyncEventSource without having a duplicate instance dedicated to logs (and the additional memory that uses) or without having to add slightly more complicated cases to the Javascript. With this as it is, the newer javascript works work older devices no problem.


I'm not super invested in this, I'm happy for the PRs to not be accepted, I just needed something to cut my teeth on with C and contributing to esphome, and the feature request seemed a good place to start.

This change, if only needed from a UI perspective by others, could be achieved through a CSS include, which might be enough of a reason to say no to the support.

@dd32 dd32 requested a review from jesserockz February 15, 2023 11:05
jesserockz
jesserockz previously approved these changes Feb 16, 2023
Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Ah yes I see the issue there with the events object. Happy then with this as it is.
Please add a docs PR with this option added targeting next

Comment on lines 321 to 323
stream->print(F("<script id='config' type='application/json'>"));
stream->print(this->get_config_json().c_str());
stream->print(F("</script>"));
Copy link
Member

Choose a reason for hiding this comment

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

Actually one question, why is this object needed to be injected in when it will be sent with the ping?

@beikeland
Copy link

Tried this without the section Jesse was asking about and seems to render heartbeats in both cases and the optional log as set by .yaml

Took a stab at doing a PR for the docs.

@nagyrobi nagyrobi added the not-stale Won't go stale label Jun 10, 2023
@jesserockz jesserockz merged commit 1cc7428 into esphome:dev Jun 21, 2023
25 checks passed
@jesserockz jesserockz added this to the 2023.6.0b6 milestone Jun 21, 2023
@jesserockz jesserockz mentioned this pull request Jun 21, 2023
jesserockz added a commit that referenced this pull request Jun 21, 2023
Co-authored-by: Jesse Hills <3060199+jesserockz@users.noreply.github.com>
@jesserockz jesserockz mentioned this pull request Jun 22, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2023
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.

Make log optional in web_server
4 participants