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

feat(api): add jitter to cron jobs, paging support, and backoff handling #33

Merged

Conversation

IceHamster
Copy link
Contributor

@IceHamster IceHamster commented Dec 19, 2024

Hello, Zaptec devs are here!

We see some issues with the current implementation of the app from our side (for example scheduled fetch of the data is failing).

We would like to help with a reliability of the plugin for Homey users, and to do so, the following changes are proposed:

  • Random jitter to cron jobs to reduce simultaneous endpoint querying — this will help with 4xx and 5xx errors users are experiencing (at scale it looks like mini DoS and will be rate limited heavily).
  • Implemented paging support for charging history API calls — this will help to get better throughput at scale.
  • Introduced backoff mechanism for handling 429 Too Many Requests responses.
  • Added custom User-Agent header to API requests for better tracking — in the future empty user agent may prevent Homey users from using the API.
  • Refactored device initialization to include app version.

These changes enhance API robustness and prevent potential overloading while ensuring compatibility with paginated responses. We hope that these changes will be beneficial for all Homey users and will improve the overall experience with the Zaptec app.

We deeply appreciate the time and expertise that go into maintaining and improving the Homey plugin, and we're excited to do a small contribution to its continued success.

@IceHamster IceHamster closed this Dec 19, 2024
@IceHamster IceHamster reopened this Dec 19, 2024
@IceHamster IceHamster marked this pull request as ready for review December 19, 2024 15:14
Copy link
Collaborator

@philipostli philipostli left a comment

Choose a reason for hiding this comment

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

Seems legit! Not tested locally yet. Will try later.

Jitter only for slowPoll (every 7+-2 minutes) that gathers yearly energy. Tried to get 5000 pages all at once at same clockhours. I had no trouble with this, but it is good to filter out noise in the API and also have reliable data every 7 minutes.
Should there be jitter on pollValues too? They are much more frequent.

Love the backoff logic.

Good work!

@@ -36,7 +37,13 @@ export class GoCharger extends Homey.Device {
this.cronTasks.push(
cron.schedule('0,30 * * * * *', () => this.pollValues()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be a jitter on pollVales too? Run every whole and half minute. Same for all users I think.

Choose a reason for hiding this comment

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

Hi, jitter is helpful on every job that is running on the schedule, but it makes more difference on any request that potentially has big response body (just due to the memory constraints on the backend when you have too many in-flight requests).

We will have a look there after we inspect the workload from Homey more closely.

Thanks for the good work and merry Christmas!

@@ -257,11 +265,11 @@ export class GoCharger extends Homey.Device {
ChargerId: this.getData().id,
From: new Date(year, 0, 1, 0, 0, 1).toJSON(),
DetailLevel: 0,
PageSize: 5000,
PageSize: 50,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good. This halves the response time.

@PatrickE94
Copy link
Owner

Hello everyone!

Very nice to see some contributions here. This looks all nice and dandy to me. I've lost access to Zaptec devices so I'm unable to verify it myself locally. However, I can't see any issues with this :)

I'll merge and push!

@PatrickE94 PatrickE94 merged commit 1ecb023 into PatrickE94:main Dec 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants