Unavailable sensor fix #33
Conversation
| @@ -23,7 +23,7 @@ def __init__(self, hass, *, api_key, account_id, off_peak_start, off_peak_end): | |||
| # Name of the data. For logging purposes. | |||
| name="Octopus Intelligent", | |||
| # Polling interval. Will only be polled if there are subscribers. | |||
| update_interval=timedelta(seconds=300), | |||
| update_interval=timedelta(seconds=120), | |||
There was a problem hiding this comment.
Thanks for the PR. Much appreciated.
I'm a bit reluctant to increase the amount of requests we make to octopus. Given the recent noise on this and the fact most were happy with 300s before, I'm going to revert this change but keep the timeout increase.
There was a problem hiding this comment.
I looked into that and they allow 10 requests a min, so well within that with the 2 min poll; the issue is their sometimes long responses. Maybe they have locking issues, not sure. 2 mins is working fine for me.
There was a problem hiding this comment.
It is true that some users jump to conclusions and incorrectly assume that any API failures are a result of rate limiting. On the other hand, response times of 30+ seconds may be a sign of server overload. I note that the proposed increased timeout of 60 seconds is 50% of the proposed reduced polling interval of 120 seconds. Think about it: If the servers took 60 seconds to reply to a request, hitting it with another request just 60 seconds later “feels a bit mean.” (Although Octopus should get their act together on server capacity and introduce real bitting rate limits if that’s an issue for them.)
My 2 cents would be for this “Unavailable sensor fix” PR to focus on increasing the timeout only, which is what fixes the issue. The polling interval in my opinion should be configurable instead of hardcoded (hass config flow).
| @@ -48,7 +48,7 @@ async def _async_update_data(self): | |||
| try: | |||
| # Note: asyncio.TimeoutError and aiohttp.ClientError are already | |||
| # handled by the data update coordinator. | |||
| async with async_timeout.timeout(30): | |||
| async with async_timeout.timeout(60): | |||
There was a problem hiding this comment.
How about something along the following lines (may need amending):
| async with async_timeout.timeout(60): | |
| timeout = max(30, min(300, update_interval / 2)) | |
| async with async_timeout.timeout(timeout): |
Where update_interval is the value from the __init__ method (maybe self.update_interval, not sure), which used to be 5 minutes and this PR currently proposes amending. The intention being: The timeout should be half the update interval, capped at a minimum of 30s and a maximum of 300s (5 minutes).
Rationale: Given that this integration runs 24/7 and automations depend on it, as a user I would rather face request timeouts as crazy long as, say, 5 minutes rather than facing “unavailable” errors.
If the update interval was configurable by the user as suggested in another comment, the 30s minimum timeout would imply a minimum polling interval of 60s.
| @@ -23,7 +23,7 @@ def __init__(self, hass, *, api_key, account_id, off_peak_start, off_peak_end): | |||
| # Name of the data. For logging purposes. | |||
| name="Octopus Intelligent", | |||
| # Polling interval. Will only be polled if there are subscribers. | |||
| update_interval=timedelta(seconds=300), | |||
| update_interval=timedelta(seconds=120), | |||
There was a problem hiding this comment.
It is true that some users jump to conclusions and incorrectly assume that any API failures are a result of rate limiting. On the other hand, response times of 30+ seconds may be a sign of server overload. I note that the proposed increased timeout of 60 seconds is 50% of the proposed reduced polling interval of 120 seconds. Think about it: If the servers took 60 seconds to reply to a request, hitting it with another request just 60 seconds later “feels a bit mean.” (Although Octopus should get their act together on server capacity and introduce real bitting rate limits if that’s an issue for them.)
My 2 cents would be for this “Unavailable sensor fix” PR to focus on increasing the timeout only, which is what fixes the issue. The polling interval in my opinion should be configurable instead of hardcoded (hass config flow).
|
I've just updated the timeout value and published a new release. Thanks |
I found the issue causing the sensors to become unavailable was due to some very choppy performance with the Octopus APIs moving form a call time of 1-3 seconds to 30 seconds or greater at times. This longer call duration then exceeded to timeout that was defined for the HA poll of 30 secs and therefore left the sensors unavailable. Fix is simple increase HA async task timeout to 60 seconds. Also changed the polling to every 2 mins and all is good.