-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
Lower scan interval for OpenSky #92593
Conversation
I am also thinking of adding a check if the status of the api call is not 200, make the sensor unavailable. Would that be a good practice? |
Co-authored-by: epenet <[email protected]>
Yes, that would be good practise (in a separate PR). A couple of other good practises that should also be considered:
If you are planning to do more work on OpenSky you should consider adding yourself as a code owner. |
Maybe later, just wanted to fix this bug haha. First going to work on LastFM and Twitch :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Hold up, I think I am making a mistake here. Just happened to have the opensky page open, and it says that requesting /states/all (without any location details) costs 4 credits, so that would mean 100 requests per day. So I think for the integration to actually make sense it should change functionality (as it now just fetches all flights and looks up the distance and then checks if that distance is under the _radius) (Also note, I did test this by just keeping it running locally and open debugger when the response wasn't 200, but I did not test it for a full day) |
@joostlek converted it to draft. Mark it ready for review when it's ready. An initial step for a hot fix would be to just hit the /all endpoint with an interval that is within limits, and upgrade what is fetched for the next major release in a follow-up PR. |
Co-authored-by: epenet <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
* Lower scan interval for opensky to avoid hitting rate limit * Lower scan interval for opensky to avoid hitting rate limit * Update homeassistant/components/opensky/sensor.py Co-authored-by: epenet <[email protected]> * Update homeassistant/components/opensky/sensor.py Co-authored-by: epenet <[email protected]> --------- Co-authored-by: epenet <[email protected]>
Whilst this fixes the issue of the component exceeding the api limit it makes it less than useful as only checking for aircraft every 15mins isn’t very useful. As this PR was approved it would be great if mine, that was closed, could be reconsidered as it fixes this issue properly (although I do note it doesn’t meet the new component architecture requirements but I hope some latitude could be given for existing components) #91162 edit: I have actually improved the code further since that PR and it is working well and capturing such things as server unavailable as was mentioned above as well as only pulling the necessary data from the server (instead of every flight in the world) |
Twitch have more better integration on HACS |
Proposed change
Lower the rate limit for OpenSky. Currently it polls every 4 minutes, but that triggers a 429 giving a not valid JSON back. According to the docs, anonymous users have up to 400 credits per day to give. 400/24 = ~16,7 requests per hour. To be safe the rate limit is put to every 4 minutes.
Type of change
Additional information
Checklist
black --fast 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: