core icon indicating copy to clipboard operation
core copied to clipboard

Allow to disable state write suppressing for MQTT sensors

Open jbouwh opened this issue 9 months ago • 7 comments

Proposed change

Integrations can suppress unchanged state writes to not burden the state machine and recorder, MQTT is an integration which does that. Suppressing unchanged state writes however breaks the last_reported functionality which is used by sensor helpers.

This PR adds an optional flag to MQTT binary_sensor and MQTT sensor which disables the suppression of unchanged state writes.

This PR is an alternative to #140001.

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: fixes #
  • This PR is related to issue: #139989
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/37908
  • Link to developer documentation pull request:
  • Link to frontend 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.

To help with the load of incoming pull requests:

jbouwh avatar Mar 10 '25 20:03 jbouwh

Hey there @emontnemery, @bdraco, mind taking a look at this pull request as it has been labeled with an integration (mqtt) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of mqtt 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 mqtt 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 Mar 10 '25 20:03 home-assistant[bot]

I'm not sure about this, it doesn't seem like something integrations should need to concern themselves about.

The PR which introduced the state filtering in MQTT, https://github.com/home-assistant/core/pull/100438, is older than the PR introducing the STATE_REPORTED event, https://github.com/home-assistant/core/issues/113511. If the order was reversed, I'm not sure we'd went ahead with such a rigid state filtering implemented in MQTT.

I'd rather disable the MQTT state filtering for numerical MQTT sensors, and maybe also non-numerical sensors and binary sensors, and evaluate mitigations if there are performance regressions.

Related:

  • We should investigate the feasibility of a fast happy path for state writes implemented in C or Rust (although I realize that's not likely to be implemented any time soon)
  • We should add some kind of dashboard tracking entities with many state writes so the user can disable them
  • We should decrease the database retention period of sensors (that doesn't solve the CPU consumption caused by spammy sensors, but it improves the disk usage)

emontnemery avatar Mar 11 '25 08:03 emontnemery

We should add some kind of dashboard tracking entities with many state writes so the user can disable them

That would be really nice regardless of this PR, I used to run a query once in a while to list the top 10 entities which update states, I think having something like this under the System -> Repairs would be enough, similar to the integration startup times.

thecode avatar Mar 11 '25 13:03 thecode

We should add some kind of dashboard tracking entities with many state writes so the user can disable them

That would be really nice regardless of this PR, I used to run a query once in a while to list the top 10 entities which update states, I think having something like this under the System -> Repairs would be enough, similar to the integration startup times.

I think this would be great to give users visibility into the amount of state writes. We have the same problem with lack of visibility as to how often they are updated in the database as well.

One challenge I don't have a good answer for is what happens when the user has an important entity that is doing too many state writes. In previous issues and troubleshooting, we sometimes reach the conclusion that a specific entity is writing state too often, but the user does not want to disable it because they rely on it for an automation, or energy reporting. These tend to be MQTT entities fed by something external where they don't have control over how frequently the updates come in.

bdraco avatar Mar 11 '25 23:03 bdraco

We should add some kind of dashboard tracking entities with many state writes so the user can disable them

That would be really nice regardless of this PR, I used to run a query once in a while to list the top 10 entities which update states, I think having something like this under the System -> Repairs would be enough, similar to the integration startup times.

I think this would be great to give users visibility into the amount of state writes. We have the same problem with lack of visibility as to how often they are updated in the database as well.

One challenge I don't have a good answer for is what happens when the user has an important entity that is doing too many state writes. In previous issues and troubleshooting, we sometimes reach the conclusion that a specific entity is writing state too often, but the user does not want to disable it because they rely on it for an automation, or energy reporting. These tend to be MQTT entities fed by something external where they don't have control over how frequently the updates come in.

I don't have a good answer for this and last time I run the query for the top entities one of them was indeed Shelly 3EM which I decided to keep as it is since it is the most useful one for me. Depends on how many entities can be listed as top, users can decide to let them stay at the top, so for example if the page list the top 50 entities users can have visibility of what is writing states but decide to ignore the first 8 entitles because they do want them, by "ignore" I don't mean any special filtering method, just that the user knows that these top 8 are bad but he wants them. I think once you have more than 30 it is irrelevant if you can see entities that are not listed since you are already writing large number of states by these top entities. When this page is created it would be possible to get feedback from the community and see if it needs extra features, top number of state writes can also be added to diagnostics, page doesn't need to be too much sophisticated, something like the integration startup times page can serve for this purpose.

image just replacing the list with entities instead of integrations and the time with states per hour

thecode avatar Mar 11 '25 23:03 thecode

Honestly, I really dislike this option. This is not something an external party should control or have concerns about.

My vote is to no add this.

../Frenck

frenck avatar Mar 25 '25 08:03 frenck

I also don't like it, as per my previous answer. I set the PR to draft.

emontnemery avatar Mar 25 '25 13:03 emontnemery

We discussed this in our core meetings and decided not to move forward with this proposal.

The reason we understand, but at the same time, we are forcing external software to fix an issue on our end. This should not be a concern of external platforms.

../Frenck

frenck avatar Mar 30 '25 14:03 frenck

Only left this one open as alternative for: https://github.com/home-assistant/core/pull/140001 I think that PR needs to be closed as well.

jbouwh avatar Mar 30 '25 16:03 jbouwh