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 rate throttling logic to PlatformAPI #103

Merged
merged 5 commits into from
Apr 24, 2020

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Apr 20, 2020

This is a continuation of #96 but is done from within the same repo so the tests can run


By default when you get a rate-limit event while using the platform-api the method call will fail. Since there's no feedback built into the client, another call might be attempted (for example if the API is being hit via a background worker). This is bad for Heroku because it means we now are seeing a stream of requests that will never complete (because of the rate limit) and it is bad for the end-user because they have a flurry of errors that are unexpected and unhandled.

This PR builds on top of the project https://github.com/zombocom/rate_throttle_client which was built out of the research from https://github.com/schneems/rate-limit-gcra-client-demo to automatically find a value that the client can sleep for so that it can maximize throughput while still minimizing the number of requests that are rate limited.

At a high-level when the client sees a 429 response from the server, it will sleep and then retry the response. If it gets another 429 then the sleep amount will be multiplicatively increased until it can make a successful request.

As the client is able to make successful requests the amount of sleep time is reduced by a subtractive amount based on the current number of requests allowed (as reported by the server), and it's the current value.

In simulations, over time we end up seeing from 2-3% of requests rate limited.

Graph from README of the simulation

This PR has been built on the work of other changes added to heroics and platform-api

This PR was paired with @lolaodelola

Lola Odelola and others added 2 commits April 20, 2020 14:22
By default when you get a rate-limit event while using the platform-api the method call will fail. Since there's no feedback built into the client, another call might be attempted (for example if the API is being hit via a background worker). This is bad for Heroku because it means we now are seeing a stream of requests that will never complete (because of the rate limit) and it is bad for the end user because they have a flurry of errors that are unexpected and unhandled.

This PR builds on top of the project https://github.com/schneems/rate-limit-gcra-client-demo to automatically find a value that the client can sleep for so that it can maximize throughput while still minimizing the number of requests that are rate limited.

At a high level when it sees a 429 response from the server, it will sleep and then retry the response. If it gets another 429 then the sleep amount will be multiplicatively increased until it can make a successful request.

As the client is able to make successful requests the amount of sleep time is reduced by a subtractive amount based off of the current number of requests allowed (as reported by the server), the amount of time since the last rate limit event, and it's current value.

This logic somewhat mirrors TCP "slow start" behavior (though in reverse).

In simulations over time we end up seeing from 2-3% of requests rate limited.

![](https://github.com/schneems/rate-limit-gcra-client-demo/blob/master/chart.png)

> Graph from README of the simulation readme

This PR has been built on the work of other changes added to heroics:

- interagent/heroics#95
- interagent/heroics#96

## Discussion

In addition to the implementation, the one last unknown is what the default logging behavior should be. While rate-throttling by default provides a good experience, we need to provide feedback to the user letting them know that it is happening.

This PR was paired with @lolaodelola
This commit adds logging when a client is rate throttled. It emits a log to stdout by default and there is a configurable logging block that is made available so that developers can push to a custom endpoint such as honeycomb.


Co-authored-by: Lola Odelola <damzcodes@gmail.com>
@schneems schneems force-pushed the schneems-lola/rate_throttle branch 8 times, most recently from 3d212cf to a22e79e Compare April 21, 2020 16:44
Instead of maintaining a single purpose rate throttle client wrapper, I ported the code over to a gem `rate_throttle_client` that has it's own set of tests and metrics generating code. This will make it easier to maintain and other libraries can use the code now as well.

I'm now also testing configuring the rate throttling logic.
@schneems schneems force-pushed the schneems-lola/rate_throttle branch from a22e79e to ae4c3af Compare April 21, 2020 16:59
@schneems
Copy link
Contributor Author

In a slack conversation:

One thing I'm concerned about is making the new rate limit logic the default, rather than making it opt-in. Gem users may be surprised by the change in behavior, especially if they added their own rate limit handling somehow

To mitigate this concern, I'm defaulting the behavior to be "off" by default. The user can either disable the rate throttling logic by following steps outlined in the warning and README, or they can upgrade to version 3.0.0 to opt-in to the new behavior.

We can release this version 2.3.0 that has a warning/deprecation at the same time as a version 3.0.0 of this gem that sets a default rate throttle strategy.

The warning will look like this the first time that developers make an API request:

```
[Warning] Starting in PlatformAPI version 3+, requests will include rate throttling logic
to opt-out of this behavior set: `PlatformAPI.rate_throttle = RateThrottleClient::Null.new`
to silence this warning and opt-in to this logic, upgrade to PlatformAPI version 3+
```

If someone sets their own strategy, then they won't get this message.

If that sounds good, once this change is accepted. I'll make another PR to rev the version and update the behavior. Once that is approved we can release 2.3.0, then merge in the behavior change and release 3.0.0 shortly after.
@schneems schneems force-pushed the schneems-lola/rate_throttle branch from 1b87f40 to b88e769 Compare April 22, 2020 20:49
Copy link

@stevenharman stevenharman left a comment

Choose a reason for hiding this comment

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

I have one suggestion for organizing some "helper" methods for specs. But it's not a blocker, so 👍 from me.

spec/spec_helper.rb Outdated Show resolved Hide resolved
@schneems
Copy link
Contributor Author

Going to merge this in and prep the 3.0 release PR. I'm currently aiming to release next week.

@schneems schneems merged commit cc3d0b9 into master Apr 24, 2020
schneems added a commit that referenced this pull request Apr 24, 2020
As a followup from #103 this PR bumps the major platform-api version and enables rate-throttling by default.
schneems added a commit that referenced this pull request Apr 24, 2020
As a followup from #103 this PR bumps the major platform-api version and enables rate-throttling by default.
schneems added a commit that referenced this pull request Apr 24, 2020
As a followup from #103 this PR bumps the major platform-api version and enables rate-throttling by default.
@schneems
Copy link
Contributor Author

The followup PR is #104

@stevenharman stevenharman deleted the schneems-lola/rate_throttle branch April 24, 2020 15:04
schneems added a commit that referenced this pull request Apr 30, 2020
As a followup from #103 this PR bumps the major platform-api version and enables rate-throttling by default.
schneems added a commit that referenced this pull request May 4, 2020
As a followup from #103 this PR bumps the major platform-api version and enables rate-throttling by default.
schneems added a commit that referenced this pull request May 5, 2020
As a followup from #103 this PR bumps the major platform-api version and enables rate-throttling by default.
schneems added a commit that referenced this pull request May 5, 2020
As a followup from #103 this PR bumps the major platform-api version and enables rate-throttling by default.
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.

2 participants