Harden recorder statistics against bad integration data#165221
Draft
gronke wants to merge 3 commits intohome-assistant:devfrom
Draft
Harden recorder statistics against bad integration data#165221gronke wants to merge 3 commits intohome-assistant:devfrom
gronke wants to merge 3 commits intohome-assistant:devfrom
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the recorder statistics compilation pipeline so that misbehaving integrations/entities don’t crash statistics collection for unrelated platforms.
Changes:
- Add per-platform exception isolation around recorder
compile_statisticscallbacks. - Add per-entity exception isolation around statistics metadata update/insertion.
- Validate
unit_of_measurementtype in sensor statistics normalization and skip offending entities with a clear log message. - Add tests covering bad platform callbacks, bad entity insertion, and non-string sensor units.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
homeassistant/components/recorder/statistics.py |
Adds per-platform and per-entity exception isolation while compiling/inserting statistics. |
homeassistant/components/sensor/recorder.py |
Adds type validation for unit_of_measurement before statistics unit normalization proceeds. |
tests/components/recorder/test_statistics.py |
Adds tests ensuring a crashing platform/entity doesn’t block other platforms/entities. |
tests/components/sensor/test_recorder.py |
Updates expectation for platform-level error logging and adds test for non-string unit handling. |
You can also share your feedback on Copilot code review. Take the survey.
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
Member
|
Likely fixes #158494 Actually I misremembered, that one was an issue with type of |
Isolate per-platform and per-entity errors in the statistics compilation pipeline so a single misbehaving integration (e.g. non-string unit_of_measurement) no longer crashes all statistics collection. - Wrap platform compile_statistics callbacks in try/except - Wrap per-entity statistics insertion in try/except - Validate unit_of_measurement type in sensor recorder before it enters metadata - Add tests verifying error isolation (pass with fix, fail without) - Update test assertion to match new error handling behavior
gronke
force-pushed
the
harden-recorder-statistics
branch
from
7dd542d to
7be11a0
Compare
Proves our per-platform error isolation catches the TypeError from unhashable state_class values, preventing it from crashing the entire statistics pipeline.
Wrap update_statistics_issues callbacks in try/except in both _compile_statistics (minute=50 path) and the standalone function. Add tests for the new error isolation and for home-assistant#158494 at minute=50.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
A misbehaving integration (e.g. Victron passing a Python object as
unit_of_measurementinstead of a string, see sfstar/hass-victron#424) can crash the entire statistics pipeline, killing statistics collection for all integrations — not just the offending one. The user sees a blank statistics dashboard with no indication of which integration caused it.This PR adds three layers of error isolation:
compile_statisticscallbacks — a crashing platform no longer blocks other platforms.unit_of_measurementin the sensor recorder — a non-string value is caught early with a clear error log naming the offending entity and integration, instead of propagating into SQLite and crashing there.All error paths produce clear log messages identifying the offending entity or platform domain.
Type of change
Checklist
ruff format homeassistant tests)To help with the load of incoming pull requests: