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

Allow to disable state write suppressing for MQTT sensors #140318

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

jbouwh
Copy link
Contributor

@jbouwh jbouwh commented Mar 10, 2025

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!)
  • 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

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • 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:

@home-assistant
Copy link

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.

@jbouwh jbouwh force-pushed the mqtt-last_report-sensor-opt-in branch from edd9549 to c649101 Compare March 10, 2025 20:42
@jbouwh jbouwh changed the title Allow opt-in for last report on mqtt sensors Allow to disable state write suppressing for MQTT sensors Mar 10, 2025
@jbouwh jbouwh marked this pull request as ready for review March 10, 2025 20:49
@emontnemery emontnemery marked this pull request as draft March 11, 2025 07:21
@emontnemery
Copy link
Contributor

emontnemery commented Mar 11, 2025

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, #100438, is older than the PR introducing the STATE_REPORTED event, #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)

@jbouwh jbouwh marked this pull request as ready for review March 11, 2025 09:57
@thecode
Copy link
Member

thecode commented Mar 11, 2025

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.

@jbouwh jbouwh force-pushed the mqtt-last_report-sensor-opt-in branch from 0f8b631 to f6ebf6a Compare March 11, 2025 17:19
@bdraco
Copy link
Member

bdraco commented Mar 11, 2025

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.

@thecode
Copy link
Member

thecode commented Mar 11, 2025

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

@frenck
Copy link
Member

frenck commented Mar 25, 2025

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

@emontnemery emontnemery marked this pull request as draft March 25, 2025 13:01
@emontnemery
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants