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

Augment history panel with Long Term Statistics #18213

Merged
merged 13 commits into from
Nov 28, 2023

Conversation

karwosts
Copy link
Contributor

Proposed change

Was thinking over some ideas for how to better view long term statistics, and I was thinking one possibility is to use them to fill out the history panel for time ranges that are not present in the recorder, so you could easily look at the history of a sensor in the history panel much longer than the typical recorder purge window.

Basically I'm grabbing both the history and statistics data for an entity in history panel, and then merging them together into a single plot, where we use the history data in the near history, and the hourly statistics from the long term history.

This probably isn't fully finished and I'm sure still has some bugs and rough edges to clean up, but just thought I'd solicit for some feedback and see if this is a style and direction that we'd like to go in before spending too much more time on it.

Check out the screengrab and let me know what you think,

long-term-history-panel

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

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

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@ildar170975
Copy link
Contributor

ildar170975 commented Oct 13, 2023

Imho this is a brilliant idea - "augmented" history.
Thanks a lot!

Proposals:

  1. Cannot see a reason to hide/show history/statistics separately; probably a legend should not contain 2 separate labels for 1 entity.
  2. In case of choosing a "show 2 separate labels" way: a legend's label is composed as friendly_name + (Statistics), this is too long (at least in English). May be we should make it shorter like (stat.)? (probably a new localized string should be added - a short name for "Statistics").
  3. As far as I understood, there are several types of statistics - "5 minutes" (which seems to be kept as long as a "normal" history), "hourly", "daily", "weekly", "monthly". Are you "adding" an "hourly" statistics on this page?

@KTibow
Copy link
Contributor

KTibow commented Oct 14, 2023

As far as I understood, there are several types of statistics - "5 minutes" (which seems to be kept as long as a "normal" history), "hourly", "daily", "weekly", "monthly". Are you "adding" an "hourly" statistics on this page?

Now I don't know that much about statistics either but I think you're misunderstanding. All statistics are recorded every 5 minutes, regardless of what time period they get reset at. However, this PR displays the hourly mean.

@ildar170975
Copy link
Contributor

All statistics are recorded every 5 minutes, regardless of what time period they get reset at

Add a new Statistics graph card, choose a period (5minute, hour, ...) and see a difference.
This is exactly what I am asking about.
Ofc it is recorded every 5 minutes - and min/max/mean/... values are recalculated for these diff. periods and then may be shown as graphs.

@karwosts
Copy link
Contributor Author

Yeah I agree there's not really a need to toggle them on/off independently, it's just like that because:

  • seemed the simplest way to hack this into the existing chart component, for handling multiple series of data
  • thought there should be some way to differentiate for the user the source of the datapoint (recorder vs lts)

And yes it is fixed to hourly aggregated stats.

@KTibow
Copy link
Contributor

KTibow commented Oct 14, 2023

My only comment is that it should be smoothed like it is in the more info panel if possible

@ildar170975
Copy link
Contributor

ildar170975 commented Oct 14, 2023

@karwosts

thought there should be some way to differentiate for the user the source of the datapoint (recorder vs lts)

Imho a slightly different coloring which you offered is rather sufficient. But may be there are some other ideas; would be great to see them. (I also agree that 2 different graphs for LTS & history could be simpler to implement as you said).

@KTibow

it should be smoothed like it is in the more info panel if possible

Could you elaborate?
Do you mean "LTS curves should be smoothed"?
History graphs are "steplined" (which is good imho).
If a smoothed LTS curve is continued with a "steplined" history curve - will it be looking good?
Besides, I am not sure that smoothing LTS even in more-info is a good idea...


As for more-info.
I am absolutely sure that a normal History should be shown in more-info (like it was earlier); LTS graphs should be shown on a different tab.
May be there should be a switch in Settings like "show LTS in more-info" (local settings) - a user with a weak client device may select to show LTS by default (then History should be available on a different tab).

@KTibow
Copy link
Contributor

KTibow commented Oct 14, 2023

You seem to not understand what I mean.
Look at the statistics here. The measurements are connected smoothly, which accurately represents the fact that it was measured as an average.
image

@ildar170975

This comment was marked as off-topic.

@KTibow

This comment was marked as off-topic.

@ildar170975

This comment was marked as off-topic.

@bramkragten bramkragten marked this pull request as draft October 16, 2023 09:00
@bramkragten
Copy link
Member

I think this is a great idea! In the long run we still need to completely overhaul out history and statistics panel, but for now this a great addition.

I would also not show 2 labels in the legend, and have the statistics graph follow the visibility of the history graph (probably needs some changes to chart-base, to hide per dataset.

@karwosts
Copy link
Contributor Author

In the long run we still need to completely overhaul out history and statistics panel

Just curious, is there a vision for what this should look like? I'm not opposed to delivering this current PR, but it does feel a bit of a kludge, so if there's a better long term solution in mind, maybe it's better to invest the time just doing that instead.

@bramkragten
Copy link
Member

No, there is no design or vision yet, but that is something that is on the list to work on...

@karwosts
Copy link
Contributor Author

I think I'll keep it to one label per entity, and keep the same colors/styling for stats vs history, but I'll include the data attribution in the tooltip. I think that's fairly unobtrusive, while still conveying the desired information.

(The extra source detail will only be shown when LTS are included in the graph)

long-term-history-panel-2

@ildar170975
Copy link
Contributor

ildar170975 commented Oct 17, 2023

the same colors/styling for stats vs history, but I'll include the data attribution in the tooltip

Imho a slightly different color (as you did in the 1st version) is better.
Also, I would suggest to replace the "Source: Long Term Statistics" by just "Statistics" + not show anything for History.

@karwosts
Copy link
Contributor Author

I think I'm happy for now with the current code, it's probably ready for a closer review.

Some more testing would be helpful as well.

@karwosts karwosts marked this pull request as ready for review October 17, 2023 20:42
@frenck frenck added the Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Oct 23, 2023
@bdraco
Copy link
Member

bdraco commented Oct 24, 2023

We could add keep_days to

export interface RecorderInfo {
and only augment with stats when they ask for history longer than keep_days so we don't make requests for data we wouldn't need

https://www.home-assistant.io/integrations/recorder#purge_keep_days

@karwosts
Copy link
Contributor Author

We could add keep_days to

export interface RecorderInfo {

and only augment with stats when they ask for history longer than keep_days so we don't make requests for data we wouldn't need

https://www.home-assistant.io/integrations/recorder#purge_keep_days

Is it worth to consider the case that some user may not have the same keep days for every entity? If for example there is any manual purges happening of select entities, starting the statistics after keep_days would leave a hole in the graph.

I don't know if that case is worth designing for or not.

If overfetching too much data is a concern, could also consider serializing the fetches instead of doing them in parallel.

@bdraco
Copy link
Member

bdraco commented Nov 20, 2023

Is it worth to consider the case that some user may not have the same keep days for every entity? If for example there is any manual purges happening of select entities, starting the statistics after keep_days would leave a hole in the graph.

I don't know if that case is worth designing for or not.

I'm not sure we should try to accommodate manual purges as it will make performance worse for the most common cases to service a few rare cases.

If overfetching too much data is a concern, could also consider serializing the fetches instead of doing them in parallel.

Thats a good idea.. It turned out not to matter so much so parallel is better for the user

@bramkragten bramkragten merged commit b6a7581 into home-assistant:dev Nov 28, 2023
13 checks passed
@karwosts karwosts deleted the long-term-history-panel branch November 29, 2023 02:55
@bdraco
Copy link
Member

bdraco commented Nov 29, 2023

This works great. The backend still hits the database for times were we know we have no history since history never ran before the end date of the query. We can probably optimize that away on the backend

@bdraco
Copy link
Member

bdraco commented Nov 29, 2023

I've made the backend short circuit impossible history queries home-assistant/core#104724

@bdraco
Copy link
Member

bdraco commented Nov 29, 2023

Did performance testing with 1 year of stats and everything is solid. I don't see any places we can make it faster as its almost all sqlalchemy or database overhead (and I've already helped optimize sqlalchemy as much as I think it can be for now)

@@ -265,6 +338,7 @@ class HaPanelHistory extends SubscribeMixin(LitElement) {
changedProps.has("_areaDeviceLookup"))))
) {
this._getHistory();
this._getStats();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we call this as part of refreshing data, shouldn't we also have this be called when a user clicks the refresh button?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember if I considered this in the initial pass, but wouldn't refreshing statistics just return the same data a second time? The statistics is not really live updating like the history is, so there's never any "new" data to get. So would we really want to bother querying and fetching it all again?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the one case where it's useful is the edge case where a user has the dashboard open permanently, on a period that relies on both statistics and states and hits refresh once a day without ever refreshing the page. So yeah, probably not a big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hacktoberfest Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants