Fix statistic_during_period wrongly prioritizing ST statistics over LT #115291
Conversation
home-assistant
bot
added
bugfix
cla-signed
core
integration: recorder
small-pr
|
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
|
|
@karwosts Could you please write a test case which fails without your changes and pass with it, the new test case should be added to tests/components/recorder/test_statistics.py I'd be happy to discuss on Discord if you need some help with the test case |
|
I might be willing to take a look at a test at some point but it seems like quite a significant learning curve, could be a little while. I think all that's needed need is just a mocked statistics with overlapping long term and short term statistics like the following, and then check statistic_during_period for the window that straddles the switchover from LT to ST stats.
I believe the current function is only returning the change starting from the oldest short term stat to the end of the request window, ignoring the change between the start of the request window and the oldest short term stat. |
|
I've made the start of a test, but as I add a lot of corner cases to the test, I'm still seeing some oddness I don't understand, and results that don't seem like what I would expect either before or after my fix. I've left some annotations in the test if you want to look it over and see if it's making any sense to you, perhaps there are more bugs yet to be found in the code. |
| ) | ||
|
|
||
| # Six minutes of growth in STS-only | ||
| # 5-minute Change includes start times exactly on or before a statistics start, but end times are not counted unless they are greater than start. |
There was a problem hiding this comment.
Please break long comments around max 88 characters per line.
#115291) * Fix statistic_during_period wrongly prioritizing ST statistics over LT * comment * start of a test * more testcases * fix sts insertion range * update from review * remove unneeded comments * update logic * min/mean/max testing
Proposed change
I believe the statistic_during_period WS API function has a bug where it may prioritize a short term statistic as the oldest datapoint over a long term statistic.
If the requested period is not hour aligned, we first look for the oldest short term statistic matching the head range and then return this as the oldest datapoint, but there may be older datapoints in long term statistics that are ignored in favor of returning this ST statistic.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: