Add sleep to prevent API throttling #15331

metbril · 2018-07-06T20:38:28Z

Description:

Related issue (if applicable): fixes #15271

This PR builds in some sleep time to prevent the API from throttling. Although the API has no daily, weekly or monthly limits, it limits the number of calls in a smaller (minutely?) timeframe. This prevents creating more than a few sensors at startup and/or updating those at the required interval.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here> n/a

Example entry for `configuration.yaml` (if applicable):

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

houndci-bot · 2018-07-06T20:38:33Z

homeassistant/components/sensor/alpha_vantage.py

@@ -218,3 +225,5 @@ def update(self):
            from_currency=self._from_currency, to_currency=self._to_currency)
        _LOGGER.debug("Received new data for forex %s - %s",
                      self._from_currency, self._to_currency)
+        _LOGGER.debug("Sleep %s seconds to prevent API throttling...", 10)
+        time.sleep(10)


undefined name 'time'

I added this in the second commit.

houndci-bot · 2018-07-06T20:38:33Z

homeassistant/components/sensor/alpha_vantage.py

@@ -162,6 +167,8 @@ def update(self):
        all_values, _ = self._timeseries.get_intraday(self._symbol)
        self.values = next(iter(all_values.values()))
        _LOGGER.debug("Received new values for symbol %s", self._symbol)
+        _LOGGER.debug("Sleep %s seconds to prevent API throttling...", 10)
+        time.sleep(10)


undefined name 'time'

I added this in the second commit.

houndci-bot · 2018-07-06T20:38:33Z

homeassistant/components/sensor/alpha_vantage.py

@@ -101,6 +104,8 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
            _LOGGER.debug("Configuring forex %s - %s", from_cur, to_cur)
            forex.get_currency_exchange_rate(
                from_currency=from_cur, to_currency=to_cur)
+            _LOGGER.debug("Sleep %s seconds to prevent API throttling...", 10)
+            time.sleep(10)


undefined name 'time'

I added this in the second commit.

houndci-bot · 2018-07-06T20:38:33Z

homeassistant/components/sensor/alpha_vantage.py

    for symbol in symbols:
        try:
            _LOGGER.debug("Configuring timeseries for symbols: %s",
                          symbol[CONF_SYMBOL])
            timeseries.get_intraday(symbol[CONF_SYMBOL])
-        except ValueError:
+            _LOGGER.debug("Sleep %s seconds to prevent API throttling...", 10)
+            time.sleep(10)


undefined name 'time'

I added this in the second commit.

metbril · 2018-07-06T20:43:20Z

cc @fabaff

rytilahti · 2018-07-07T18:52:57Z

homeassistant/components/sensor/alpha_vantage.py

    dev = []
+
+    timeseries = TimeSeries(key=api_key)
    for symbol in symbols:
        try:
            _LOGGER.debug("Configuring timeseries for symbols: %s",
                          symbol[CONF_SYMBOL])
            timeseries.get_intraday(symbol[CONF_SYMBOL])


Is it necessary to do I/O already here?

This was in the original source already. So I wouldn't know. I only move these lines below dev = [] in line with the rest of the code further down.

Ok, I don't know either, but having a sleep inside the setup is not acceptable (as it will make everything else in homeassistant to wait, too). Maybe it's fine to do an initial fetching here, and then just throttle the updates 👍

I fully understand that a sleep isn't the most elegant way to do this, but I can't think of another way taking into account how this API is working (accepting only an unknown but very lownumber of calls per second).

Hi,
i wrote #15258 which also contains sleep statements, although in a more random fashion.
In order to ban the sleeps from the setup method, i think it is necessary to have only one query against the api. The current implementation runs a query for every configured stock/currencyexchange. When you simply want to check if the api key is valid, one query is enough.
Or not running queries in Setup() at all, because later in the update() methods one get's crash-log-entries anyway when the key is bad.

When we don't make api calls at setup, a sensor will always be created regardless if a valid symbol is used or not. How would you reflect those errors on updates? Keep the state of the sensor at 'None' and send a 'critical' error?

That would be the negative effect, that there can be a certain amount error messages, if you have many stocks to query. On the other side, if the api key is incorrect, then it should be corrected asap. If a symbol is incorrect, i also find it ok, if a system "screams", because it also needs to be fixed. The error messages an be switched off by simply maintaining a proper configuration.
And yes, returning None will make the symbols disappear and then the user will definively have a deeper look on what's wrong.

rytilahti · 2018-07-07T18:54:27Z

homeassistant/components/sensor/alpha_vantage.py

@@ -162,6 +168,8 @@ def update(self):
        all_values, _ = self._timeseries.get_intraday(self._symbol)
        self.values = next(iter(all_values.values()))
        _LOGGER.debug("Received new values for symbol %s", self._symbol)
+        _LOGGER.debug("Sleep %s seconds to prevent API throttling...", 10)
+        time.sleep(10)


Instead of doing I/O on setup_platforms it should be done inside update. Instead of sleeping you should use homeassistant.util.Throttle decorator (grep for Throttle on the codebase to see examples).

Well. It (also) needs to be done in some way during setup of the platforms. The API is being hit in the setup_platforms as well as in the update.

Can a Throttle be used for setup_platforms, too?

I will look into the Throttle for the update and add a commit.

setup_platform is only called once when the platform is initialized, so there's no need to throttle that.

Well, that's exactly my point. The reason I started thinking about a PR in the first place was the platform setup. If you have more that a few ( 1 or 2) symbols, then the current platform setup will show errors because of throttling. The setup will call the API for each symbol. See line 90:

timeseries.get_intraday(symbol[CONF_SYMBOL])

5, 6 or 10 of these calls and the API will complain (and those sensors will not be created at all).

metbril · 2018-07-07T19:39:30Z

@rytilahti Thanks for the quick review. I'm a fairly novice (Python) developer, and tried to apply a quick fix with my limited knowledge.

metbril · 2018-07-07T20:41:29Z

I have been doing some thinking and testing. The Throttle only works for 1 specific sensor for 1 specific quote. This brings back the throttling messages. The sleep did not, when I was testing it.

balloob · 2018-07-12T09:51:34Z

homeassistant/components/sensor/alpha_vantage.py

    for symbol in symbols:
        try:
            _LOGGER.debug("Configuring timeseries for symbols: %s",
                          symbol[CONF_SYMBOL])
            timeseries.get_intraday(symbol[CONF_SYMBOL])
-        except ValueError:
+            _LOGGER.debug("Sleep %s seconds to prevent API throttling...", 10)
+            time.sleep(10)


time.sleep is never allowed.

I was already aware of that. The basic approach is

Get rid of API calls at platform setup

Use Throttle for the update() function.

@balloob Only question is if the throttle will work for 1 specific sensor or across sensors.

@metbril the proper way to do this is to have the main component polling & updating some shared data-structure with the polled contents. The sensors themselves are set to require no polling, and are informed by the main component when the values are updated. Unfortunately I don't know what's a simple example using this structure, only that xiaomi_aqara is doing it like that. I hope someone else will chime in on that!

Platforms can also be based around a main data structure (client) class that interacts with an api. This structure is set up when the platform is first set up and stored in hass.data. Subsequent platform set ups should check for existing client in hass.data. The client instance pulls all data from the api, in a single request, in the client update method which is throttled, and stores the data in an instance attribute. Entities call the client update method in their update methods and access the client data attribute when needed.

metbril · 2018-07-12T19:30:02Z

I really appreciate all your good suggestions, but this is getting way over my head. I am not an experienced coder so I will run into a lot of issues before (if) ever completing this PR. Unless someone else wants to continue working on this, I think I will close the PR.

Thing is, the component is not working properly when used with more then a few sensors. And it's the only API I know that also supports European stocks.

Add sleep to prevent API throttling

c656675

homeassistant added platform: sensor.alpha_vantage cla-signed small-pr PRs with less than 30 lines. labels Jul 6, 2018

houndci-bot reviewed Jul 6, 2018

View reviewed changes

ghost added the in progress label Jul 6, 2018

Import time

fbc1b18

homeassistant added the cla-signed label Jul 6, 2018

rytilahti reviewed Jul 7, 2018

View reviewed changes

balloob requested changes Jul 12, 2018

View reviewed changes

balloob closed this Aug 9, 2018

ghost removed the in progress label Aug 9, 2018

home-assistant locked and limited conversation to collaborators Dec 10, 2018

ghost added integration: alpha_vantage and removed platform: sensor.alpha_vantage labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sleep to prevent API throttling #15331

Add sleep to prevent API throttling #15331

metbril commented Jul 6, 2018

houndci-bot Jul 6, 2018

metbril Jul 6, 2018

houndci-bot Jul 6, 2018

metbril Jul 6, 2018

houndci-bot Jul 6, 2018

metbril Jul 6, 2018

houndci-bot Jul 6, 2018

metbril Jul 6, 2018

metbril commented Jul 6, 2018

rytilahti Jul 7, 2018

metbril Jul 7, 2018

rytilahti Jul 7, 2018

metbril Jul 8, 2018

lausser Jul 8, 2018

metbril Jul 9, 2018

lausser Jul 9, 2018

rytilahti Jul 7, 2018

metbril Jul 7, 2018

rytilahti Jul 7, 2018

metbril Jul 8, 2018

metbril commented Jul 7, 2018

metbril commented Jul 7, 2018

balloob Jul 12, 2018

metbril Jul 12, 2018

rytilahti Jul 12, 2018

MartinHjelmare Jul 12, 2018 •

edited

metbril commented Jul 12, 2018

Add sleep to prevent API throttling #15331

Add sleep to prevent API throttling #15331

Conversation

metbril commented Jul 6, 2018

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metbril commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metbril commented Jul 7, 2018

metbril commented Jul 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Jul 12, 2018 • edited

Choose a reason for hiding this comment

metbril commented Jul 12, 2018

Example entry for `configuration.yaml` (if applicable):

MartinHjelmare Jul 12, 2018 •

edited