Skip to content

Implement template rate limits#40667

Merged
balloob merged 10 commits intohome-assistant:devfrom
bdraco:template_ratelimit
Oct 1, 2020
Merged

Implement template rate limits#40667
balloob merged 10 commits intohome-assistant:devfrom
bdraco:template_ratelimit

Conversation

@bdraco
Copy link
Member

@bdraco bdraco commented Sep 27, 2020

Breaking change

In 0.115, support for watching all states and an entire domain of states was added to template tracking. To prevent the system from being overwhelmed when many state changed events coincide, re-evaluation of templates are limited to once per minute when all states or states for an ensure domain are accessed in the template. As before, no rate limit is imposed when the template only access states for specific entities or receives state changes events for specifically referenced entities.

Proposed change

Implement template rate limits from home-assistant/architecture#206

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

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
  • The code has been formatted using Black (black --fast 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@bdraco bdraco linked an issue Sep 27, 2020 that may be closed by this pull request
@bdraco
Copy link
Member Author

bdraco commented Sep 27, 2020

This should get some tests for the template directive outside of test_event.py

@bdraco
Copy link
Member Author

bdraco commented Sep 27, 2020

In a followup: Will need a websocket flag to set a rate limit as well after #40647 so pasting in a template that would overwhelm the system doesn't happen.

@bdraco bdraco marked this pull request as ready for review September 27, 2020 20:49
bdraco added a commit to bdraco/home-assistant that referenced this pull request Sep 27, 2020
After the refactoring in home-assistant#40667 this is no
longer needed.
bdraco added a commit to bdraco/home-assistant that referenced this pull request Sep 27, 2020
After the refactoring in home-assistant#40667 this is no
longer needed.
@bdraco
Copy link
Member Author

bdraco commented Sep 27, 2020

I realized we can drop _last_info to make this simpler. I left it out of this PR too minimize the review overhead and did it in #40685

@bdraco
Copy link
Member Author

bdraco commented Sep 27, 2020

Maybe this should return an empty string

{{ rate_limit(minutes=1) }}{{ states | selectattr('state', 'in', ['unavailable', 'unknown', 'none']) | list | count }}

seems cleaner in contrast with:

{% set delta = rate_limit(minutes=1) %}{{ states | selectattr('state', 'in', ['unavailable', 'unknown', 'none']) | list | count }}

but you could also do

{{ rate_limit(minutes=1) and states | selectattr('state', 'in', ['unavailable', 'unknown', 'none']) | list | count }}

so I guess I'll leave it as-is

@balloob
Copy link
Member

balloob commented Sep 27, 2020

Should we have a default rate limit of 1 per second ?

@bdraco
Copy link
Member Author

bdraco commented Sep 27, 2020

Should we have a default rate limit of 1 per second ?

I thought about doing that, but I figured it would break automations that looking for a template sensor that was aggregating button states (probably remotes) to see if any of them were pressed if the press and release event happens inside the ratelimit window. I don't have a good handle on what the impact would be so I didn't do it.

I'm thinking for the developer tools we should do that so you can test your template without much risk of overwhelming the system.

@bdraco
Copy link
Member Author

bdraco commented Sep 27, 2020

I think async_refresh needs to cancel all pending timers as well.

bdraco added a commit to bdraco/home-assistant that referenced this pull request Sep 27, 2020
After the refactoring in home-assistant#40667 this is no
longer needed.
bdraco added a commit to bdraco/home-assistant that referenced this pull request Sep 28, 2020
After the refactoring in home-assistant#40667 this is no
longer needed.
@bdraco
Copy link
Member Author

bdraco commented Sep 28, 2020

I'm not sure it actually saves anything because of the tight coupling, but maybe a class KeyedRateLimit to abstract the rate limit away in order to avoid making _TrackTemplateResultInfo more complicated.

@bdraco bdraco marked this pull request as draft September 28, 2020 14:02
bdraco added a commit to bdraco/home-assistant that referenced this pull request Sep 28, 2020
After the refactoring in home-assistant#40667 this is no
longer needed.

tweaks
@bdraco bdraco marked this pull request as ready for review September 30, 2020 00:04
@balloob balloob added this to the 0.116.0 milestone Oct 1, 2020
@bdraco
Copy link
Member Author

bdraco commented Oct 1, 2020

Should we have a default rate limit of 1 per second ?

After reviewing some of the initial 0.116 py-spy, I'm thinking that we should set a 1 second rate limit by default if they are accessing all states or watching a whole domain of states. The other case where they are watching for a single sensor or remote key seems unlikely if they are watching everything or all of sensor.*. They can always override it if they really need that level of granularity while tracking all of that

@bdraco bdraco force-pushed the template_ratelimit branch from 1e8619d to 09fc139 Compare October 1, 2020 13:02
@bdraco bdraco linked an issue Oct 1, 2020 that may be closed by this pull request
@balloob balloob merged commit b45215f into home-assistant:dev Oct 1, 2020
@bdraco bdraco changed the title Implement template rate_limit directive Implement template rate limits Oct 4, 2020
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.

Template editor hangs Home Assistant Higher CPU continues in 0.118

4 participants