Use state_reported events in Riemann sum sensor #113869

emontnemery · 2024-03-20T08:55:36Z

Proposed change

Use state_reported instead of state_changed events in Riemann sum sensor.

Unlike state_changed events, state_reported events are fired every time an integration writes the sensor state, even when state and attributes are unchanged.

This fixes bugs where the calculated integral is incorrect for sensors which don't set force_update to True

Test PRs which should be merged first:

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

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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:

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

I have reviewed two other open pull requests in this repository.

home-assistant · 2024-03-20T08:55:43Z

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

Code owner commands

Code owners of integration 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 integration 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.

homeassistant/components/integration/sensor.py

emontnemery · 2024-03-21T09:22:13Z

@ronweikamp FYI

ronweikamp · 2024-03-21T12:10:31Z

In your review of #110685 you requested to extract the refactoring. Although not strictly necessary, it might be a better base for this PR as well #113932 .

homeassistant/components/integration/sensor.py

ronweikamp · 2024-05-04T09:58:13Z

What issues does this PR address or what is the reason of this PR?

Can we close this in favor of max_sub_interval #110685? In that PR people started to discuss whether state_reported or max_sub_interval is a better solution for the issue that the integral doesn't update when the source is constant.

rkistner · 2024-05-04T18:55:00Z

What issues does this PR address or what is the reason of this PR?

Can we close this in favor of max_sub_interval #110685? In that PR people started to discuss whether state_reported or max_sub_interval is a better solution for the issue that the integral doesn't update when the source is constant.

#110685 can solve the same issue, but requires additional configuration.

This PR solves it without any additional configuration, which means the default behavior won't be broken anymore. It only works for cases where the sensor reports values even if unchanged, so there is value in having both.

dgomes · 2024-05-17T23:03:13Z

This PR needs a rebase

elupus · 2024-06-18T21:14:47Z

I think this is still valid. But need rebase.

emontnemery · 2024-06-24T07:53:33Z

Rebased, but some new tests are needed

emontnemery · 2024-06-25T09:26:40Z

@ronweikamp @dgomes @elupus I think this PR is ready, can you take a look please?

dgomes

We should add a test that clearly benefits from state_reported

dgomes · 2024-06-25T19:27:20Z

homeassistant/components/integration/sensor.py

-    def validate_states(
-        self, left: State, right: State
-    ) -> tuple[Decimal, Decimal] | None:
+    def validate_states(self, left: str, right: str) -> tuple[Decimal, Decimal] | None:


Why the move from State to str ?

It's because we don't have two complete state objects when only the reported time is bumped

elupus · 2024-06-25T20:11:46Z

homeassistant/core.py

+    old_state: NotRequired[State | None]
+    new_state: State | None
+
+


I was mucking around with this in another change. The reported event is actually a union of changed or reported structures. It was somewhat tricky to get the typing correct.

The EVENT_STATE_REPORTED constant should really be set up go map to the event definition, but was tricky to right with a union of types. Maybe that works with this squashed class.

If we have a squashed class, maybe there should be some doc string to indicate that it contains the full changed event or the reported event?

Were not fully how i expected it to be resolved. Somewhat easy to break this if the changed event data is modified. Typing wont catch that reported event piggy backs on the same data as changed.
But im okey with this.

elupus

I think this looks alright. I have some itches with how the reported event have some rather weird overlap with changed event, but doesn't need to block this.

elupus · 2024-06-25T20:44:58Z

homeassistant/core.py

+    old_state: NotRequired[State | None]
+    new_state: State | None
+
+


Were not fully how i expected it to be resolved. Somewhat easy to break this if the changed event data is modified. Typing wont catch that reported event piggy backs on the same data as changed.
But im okey with this.

homeassistant/components/integration/sensor.py

elupus

Looks 👍

bdraco · 2024-06-26T22:13:56Z

homeassistant/components/integration/sensor.py

-            async_track_state_change_event(
-                self.hass,
-                [self._sensor_source_id],
+            self.hass.bus.async_listen(
+                EVENT_STATE_CHANGED,
                handle_state_change,
+                event_filter=callback(
+                    lambda event_data: event_data["entity_id"] == self._sensor_source_id
+                ),
+                run_immediately=True,
+            )
+        )
+        self.async_on_remove(
+            self.hass.bus.async_listen(
+                EVENT_STATE_REPORTED,
+                handle_state_report,
+                event_filter=callback(
+                    lambda event_data: event_data["entity_id"] == self._sensor_source_id
+                ),
+                run_immediately=True,


I'm getting profiles from beta users this lambda being called 1M/min

I think we need to switch the first one back to async_track_state_change_event and than make an async_track_state_change_reported so we can dispatch based on entity id since we now have 2 * X integration entities top level listeners with this change

#120621

Small snapshot of the lambdas being called.

emontnemery requested review from dgomes and a team as code owners March 20, 2024 08:55

home-assistant bot added cla-signed core integration: integration labels Mar 20, 2024

emontnemery marked this pull request as draft March 20, 2024 08:55

home-assistant bot assigned dgomes Mar 20, 2024

home-assistant bot added the Quality Scale: internal label Mar 20, 2024

emontnemery mentioned this pull request Mar 20, 2024

Add State.last_reported #113511

Merged

20 tasks

dgomes reviewed Mar 20, 2024

View reviewed changes

homeassistant/components/integration/sensor.py Outdated Show resolved Hide resolved

homeassistant/components/integration/sensor.py Outdated Show resolved Hide resolved

emontnemery force-pushed the integration_sensor_last_reported branch from 87a2917 to 6d06fdb Compare March 21, 2024 09:21

emontnemery marked this pull request as ready for review March 21, 2024 09:21

dgomes reviewed Mar 21, 2024

View reviewed changes

homeassistant/components/integration/sensor.py Outdated Show resolved Hide resolved

homeassistant/components/integration/sensor.py Outdated Show resolved Hide resolved

emontnemery force-pushed the integration_sensor_last_reported branch from c471798 to e8e6776 Compare March 28, 2024 11:55

ronweikamp mentioned this pull request May 4, 2024

Switch to use reported events for integral #116742

Closed

20 tasks

This was referenced May 4, 2024

Switch to left method as default for rieman sum #116736

Closed

Riemann Sum integration uses broken defaults #92404

Closed

emontnemery force-pushed the integration_sensor_last_reported branch from e8e6776 to 0cbf6d6 Compare June 24, 2024 07:53

frenck added this to the 2024.7.0b0 milestone Jun 24, 2024

dgomes reviewed Jun 25, 2024

View reviewed changes

emontnemery force-pushed the integration_sensor_last_reported branch from 5e4d9d2 to a16738d Compare June 25, 2024 20:09

elupus reviewed Jun 25, 2024

View reviewed changes

elupus approved these changes Jun 25, 2024

View reviewed changes

dgomes approved these changes Jun 25, 2024

View reviewed changes

emontnemery added 2 commits June 26, 2024 11:26

Modify integration sensor

0da50c3

Update tests

2058777

emontnemery force-pushed the integration_sensor_last_reported branch from dea2581 to 2058777 Compare June 26, 2024 09:29

emontnemery added 2 commits June 26, 2024 11:48

Subscribe also to EVENT_STATE_CHANGED

9cb8b5b

Refactor event handlers

423f4c5

elupus approved these changes Jun 26, 2024

View reviewed changes

frenck merged commit a36c40a into dev Jun 26, 2024
25 checks passed

frenck deleted the integration_sensor_last_reported branch June 26, 2024 11:35

elupus mentioned this pull request Jun 26, 2024

integration calls async_listen with run_immediately which is deprecated #120585

Closed

bdraco reviewed Jun 26, 2024

View reviewed changes

bdraco mentioned this pull request Jun 27, 2024

Fix performance regression in integration from state_reported #120621

Merged

20 tasks

github-actions bot locked and limited conversation to collaborators Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use state_reported events in Riemann sum sensor #113869

Use state_reported events in Riemann sum sensor #113869

emontnemery commented Mar 20, 2024 •

edited

Loading

home-assistant bot commented Mar 20, 2024

emontnemery commented Mar 21, 2024

ronweikamp commented Mar 21, 2024

ronweikamp commented May 4, 2024

rkistner commented May 4, 2024

dgomes commented May 17, 2024

elupus commented Jun 18, 2024

emontnemery commented Jun 24, 2024

emontnemery commented Jun 25, 2024

dgomes left a comment

dgomes Jun 25, 2024

emontnemery Jun 25, 2024

elupus Jun 25, 2024

elupus Jun 25, 2024 •

edited

Loading

elupus left a comment

elupus Jun 25, 2024 •

edited

Loading

elupus left a comment

bdraco Jun 26, 2024 •

edited

Loading

bdraco Jun 26, 2024

Use state_reported events in Riemann sum sensor #113869

Use state_reported events in Riemann sum sensor #113869

Conversation

emontnemery commented Mar 20, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Mar 20, 2024

emontnemery commented Mar 21, 2024

ronweikamp commented Mar 21, 2024

ronweikamp commented May 4, 2024

rkistner commented May 4, 2024

dgomes commented May 17, 2024

elupus commented Jun 18, 2024

emontnemery commented Jun 24, 2024

emontnemery commented Jun 25, 2024

dgomes left a comment

Choose a reason for hiding this comment

dgomes Jun 25, 2024

Choose a reason for hiding this comment

emontnemery Jun 25, 2024

Choose a reason for hiding this comment

elupus Jun 25, 2024

Choose a reason for hiding this comment

elupus Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

elupus left a comment

Choose a reason for hiding this comment

elupus Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

elupus left a comment

Choose a reason for hiding this comment

bdraco Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

bdraco Jun 26, 2024

Choose a reason for hiding this comment

emontnemery commented Mar 20, 2024 •

edited

Loading

elupus Jun 25, 2024 •

edited

Loading

elupus Jun 25, 2024 •

edited

Loading

bdraco Jun 26, 2024 •

edited

Loading