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

Add 'reset' option to reset limit counter at the end of each interval. #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kulesa
Copy link

@kulesa kulesa commented Dec 18, 2012

In particular that would be useful for LinkedIn API who reset daily counters at midnight UTC.

@gevans
Copy link
Owner

gevans commented Dec 19, 2012

Thanks for the pull request. Do you know of other APIs that reset their counters as well?

I'll have to think about this one a bit before merging. I'm not 100% sure a reset option is necessary.

@kulesa
Copy link
Author

kulesa commented Dec 19, 2012

Sure. I never used any other API that reset its counters, but googling for "api rate limit reset at midnight" shows that there are some other APIs like this, though not a lot.

Anyway, I just have to deal with LinkedIn API, and if the counter is reset at midnight, it doesn't make sense to delay any throttled request for full 24 hours.

@gevans
Copy link
Owner

gevans commented Dec 19, 2012

Right. I hadn't thought of the issue of throttled jobs when a :period is greater than a few minutes. There definitely are a couple other APIs (I probably should have Googled first) that reset their counters though they seem to have varying timezones. In this case, the solution I had in mind was to add logic to RateLimit so:

sidekiq_options throttle: { threshold: 100_000, period: 1.day }

Would be reduced to:

sidekiq_options throttle: { threshold: 125, period: 108.seconds }

It has the added benefit of spreading out job executions rather than creating a massive flood of activity then idling until the end of a long period.

The only issue I have with the addition of a :reset option is that it may be confusing for other developers, some APIs use different timezones, and it adds a bit more complexity. On a side note, I like the #end_of_period method you wrote. :)

Let me know if this works for you. I might be completely crazy.

@kulesa
Copy link
Author

kulesa commented Dec 19, 2012

I agree the option is confusing. I'll try how it works first.

And let me also tell you few other things about LinkedIn API. I don't know if these features would be in great demand by users of other APIs though.

  1. There are 'per application' and 'per user' limits. Each API call adds to both limits.
  2. Bulk requests are throttled like multiple individual requests. It'd be good to have a way to specify increment count in throttle options of a job. For example, 'People Search' API call returns detailed profiles of user's connections. It allows bulk request up to 100 profiles, so I'd use somehitng like throttle: { increment: 100 } in job options.
  3. It could happen that API limit is exceeded outside of Sidekiq realm. In this case LinkedIn returns 403 HTTP status code with some details in response body. There must be a way to manually tell Sidekiq that rate limit is exceeded. I believe this is how most other APIs work as well.
  4. Some daily limits are very low. You can check details here: https://developer.linkedin.com/documents/throttle-limits, but for example, user limit for sending messages is 10 messages per day. Reducing it to { threshold: 1, period: 8640.seconds} wouldn't make sense. I have users who are sending 12 or 15 messages once a day, and instead of spreading these requests through the day, it is better send 10 now and remaining 2 or 5 on the next day .

I hope this makes sense. :)

@bobbrez
Copy link

bobbrez commented Mar 4, 2014

I'm also battling the LinkedIn API right now and am working around the same limitations.

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.

3 participants