core icon indicating copy to clipboard operation
core copied to clipboard

Add uptime sensor to Glances

Open wittypluck opened this issue 1 year ago • 1 comments

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:

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 running python3 -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:

wittypluck avatar Feb 25 '24 18:02 wittypluck

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.

home-assistant[bot] avatar Feb 25 '24 18:02 home-assistant[bot]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Mar 05 '24 20:03 home-assistant[bot]

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! 👍

Learn more about our pull request process.

gjohansson-ST avatar Mar 10 '24 17:03 gjohansson-ST

Thanks, coverage and conflicts should be solved now.

wittypluck avatar Mar 10 '24 18:03 wittypluck

Thanks, coverage and conflicts should be solved now.

thank you so much for this :)

oneseventhree avatar Mar 11 '24 10:03 oneseventhree

Hello, would it be possible to validate/merge this PR for the April release? Thanks 😊

wittypluck avatar Mar 20 '24 18:03 wittypluck

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 the GlancesUptimeSensor class completely
  • uptime sensor is now handled like all other sensors, through the data structure in GlancesDataUpdateCoordinator
  • moved update value code in GlancesSensor from the native_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 !

wittypluck avatar Mar 25 '24 18:03 wittypluck

One test is failing but is not related to this PR (ruff-format tests/components/light/common.py)

wittypluck avatar Mar 26 '24 00:03 wittypluck

Hello @gjohansson-ST , I think I have addressed all comments, let me know if you need more information. Thanks!

wittypluck avatar Mar 26 '24 11:03 wittypluck

I have created the documentation update PR

wittypluck avatar Mar 26 '24 17:03 wittypluck

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): image Second server with a longer polling loop in Glances (120 seconds): image

So not really better than the implementation based on utcnow().

wittypluck avatar Mar 27 '24 18:03 wittypluck

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 ?

wittypluck avatar Mar 27 '24 19:03 wittypluck

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.

gjohansson-ST avatar Mar 28 '24 12:03 gjohansson-ST

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.

wittypluck avatar Mar 28 '24 19:03 wittypluck

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.

wittypluck avatar Mar 29 '24 14:03 wittypluck

Great, thank you @gjohansson-ST !

wittypluck avatar Mar 31 '24 20:03 wittypluck

Great, thank you @gjohansson-ST !

You are the BEST wittypluck. Thank you so much for your hard work :)

oneseventhree avatar Mar 31 '24 21:03 oneseventhree