core
core copied to clipboard
Add uptime sensor to Glances
Proposed change
This PR adds the uptime sensor to Glances Integration. It is based on this older, stale PR #95517 with the following updates (which take into account the comments done during the previous PR review):
- update to latest changes on dev branch
- replace custom date parsing by dt.parse_duration
- add check to avoid value flapping (reject small updates to the value)
- update tests to new snapshots method
Type of change
- [ ] Dependency upgrade
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [x] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
- This PR fixes or closes issue: this PR can replace #95517 and #103168
- This PR is related to issue:
- Link to documentation pull request:
Checklist
- [x] The code change is tested and works locally.
- [x] Local tests pass. Your PR cannot be merged unless tests pass
- [x] There is no commented out code in this PR.
- [x] I have followed the development checklist
- [x] I have followed the perfect PR recommendations
- [x] The code has been formatted using Ruff (
ruff format homeassistant tests
) - [x] Tests have been added to verify that the new code works.
If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated for www.home-assistant.io
If the code communicates with devices, web services, or third-party tools:
- [ ] The manifest file has all fields filled out correctly.
Updated and included derived files by running:python3 -m script.hassfest
. - [ ] New or updated dependencies have been added to
requirements_all.txt
.
Updated by runningpython3 -m script.gen_requirements_all
. - [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [ ] Untested files have been added to
.coveragerc
.
To help with the load of incoming pull requests:
- [ ] I have reviewed two other open pull requests in this repository.
Hey there @engrbm87, mind taking a look at this pull request as it has been labeled with an integration (glances
) you are listed as a code owner for? Thanks!
Code owner commands
Code owners of glances
can trigger bot actions by commenting:
-
@home-assistant close
Closes the pull request. -
@home-assistant rename Awesome new title
Renames the pull request. -
@home-assistant reopen
Reopen the pull request. -
@home-assistant unassign glances
Removes the current integration label and assignees on the pull request, add the integration domain after the command. -
@home-assistant add-label needs-more-information
Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request. -
@home-assistant remove-label needs-more-information
Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:
I've marked this PR as draft. CI is failing (test coverage needed) and also there are conflicts that needs to be fixed. Please un-draft it once it is ready for review again by clicking the "Ready for review" button.
Thanks! 👍
Thanks, coverage and conflicts should be solved now.
Thanks, coverage and conflicts should be solved now.
thank you so much for this :)
Hello, would it be possible to validate/merge this PR for the April release? Thanks 😊
Hello @gjohansson-ST , I have reworked the code based on your suggestions:
- moved uptime sensor code to
GlancesDataUpdateCoordinator
- removed most of the code specific to the uptime sensor in the
sensor.py
file, and removed theGlancesUptimeSensor
class completely - uptime sensor is now handled like all other sensors, through the data structure in
GlancesDataUpdateCoordinator
- moved update value code in
GlancesSensor
from thenative_value()
getter to_handle_coordinator_update()
and added initialization of value in__init__()
- added unit tests for uptime sensor
All in all the code looks much simpler, thanks !
One test is failing but is not related to this PR (ruff-format tests/components/light/common.py)
Hello @gjohansson-ST , I think I have addressed all comments, let me know if you need more information. Thanks!
I have created the documentation update PR
So I have tested the implementation with the now
sensor locally, and unfortunately it does not solve the question of the variation of uptime in Home Assistant.
There is an internal polling mechanism in Glances (server side) and several factors resulting in variations once converted back to a datetime in HA.
A few examples :
First server with a short polling loop in Glances (30 seconds):
Second server with a longer polling loop in Glances (120 seconds):
So not really better than the implementation based on utcnow()
.
Could you maybe explain a bit more what your concern is with the implementation ?
There are several other uptime sensors in HA based on utcnow()
such as :
https://github.com/home-assistant/core/blob/d1209934776d343d9e16959126f47373608066c9/homeassistant/components/unifi/sensor.py#L92
https://github.com/home-assistant/core/blob/d1209934776d343d9e16959126f47373608066c9/homeassistant/components/freebox/router.py#L192
https://github.com/home-assistant/core/blob/d1209934776d343d9e16959126f47373608066c9/homeassistant/components/wled/sensor.py#L75
https://github.com/home-assistant/core/blob/d1209934776d343d9e16959126f47373608066c9/homeassistant/components/synology_dsm/sensor.py#L418
There are also several implementations which add a test to avoid unwanted small variations of the value : https://github.com/home-assistant/core/blob/d1209934776d343d9e16959126f47373608066c9/homeassistant/components/shelly/coordinator.py#L378 https://github.com/home-assistant/core/blob/d1209934776d343d9e16959126f47373608066c9/homeassistant/components/shelly/utils.py#L174 https://github.com/home-assistant/core/blob/3d4733a02668dd1fa3550e5cd997752cd70a52a7/homeassistant/components/fritz/sensor.py#L47
For me, a good uptime sensor does not need to be precise (people look at uptime in days, or maybe hours, not seconds) but it does need to be stable, to avoid spamming the logs and database. If the concern is about performance, I think we can find a solution to have a fast test, or set the seconds to 0.
What do you think ?
My problem is that it's not stable (using utcnow()
or anything else doesn't solve it as it's the uptime
value from the glance api which is the issue) so I think the best path forward would be to set it once during initialization and then only again set it if the server has been restarted (however we now can identify that).
You can compare with system_monitor
integration which also sets boot_time
only once in the coordinator but that's for the local system so it doesn't have the issue of a possible reboot or such that you have here as it can be a remote server.
Thanks for the quick reply !
In this case, I think the best way to detect a reboot is to check the uptime duration (provided by the API), and only update uptime
if the new duration is smaller than the previous duration.
So the logic would be something like this :
if previous_uptime_duration is None
or new_uptime_duration < previous_uptime_duration
then
uptime = utcnow() - new_uptime_duration
I'll test and confirm.
Hello @gjohansson-ST , as discussed I have changed the logic to update uptime only on startup or when the remote server restarts. Looks good to me now, no small variations, and no need for an arbitrary minimum allowed variation.
Great, thank you @gjohansson-ST !
Great, thank you @gjohansson-ST !
You are the BEST wittypluck. Thank you so much for your hard work :)