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

fix: [PSERV-2110] skip_n_calls take amount accounts for current call #50

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

LeweyM
Copy link

@LeweyM LeweyM commented Dec 6, 2023

Description

This PR fixes an issue where skip_n_calls would inaccurately limit received requests.

Local Test Results

Tests generated locally using testing tools added here: https://github.com/auth0/platform-core-services-tools/pull/24/files

The graphs below show the number of conformant requests under the following conditions:

- 10 nodes making requests at 330 RPS (3300 total)
- global rate limit configured to 1000 RPS

Before Fix

image

Notice that it is limiting to 1.4k RPS instead of 1k RPS

After Fix

The graph below shows the number of conformant requests under the following conditions:

- 10 nodes making requests at 330 RPS (3300 total)
- global rate limit configured to 1000 RPS

image

Notice that it is now correctly limiting to 1k RPS

Other Local Test Case Results

Nodes Limit Per Second skip_n_calls Rough Estimate of Allowed RPS
10 1000 0 997.6567443255674
10 1000 10 1014.9835699962139
10 1000 20 1031.8201798598202
10 2000 0 2008.1796153717962
10 2000 10 1975.5858597281813
10 2000 20 2009.101272813409
20 1000 0 1004.0967691042822
20 1000 10 1026.260158211675
20 1000 20 967.3338773895454
20 2000 0 2002.9174408932795
20 2000 20 1986.750242652974
30 1000 10 951.6324449810605

Root Cause:

When taking tokens after skipping calls to redis, we were taking tokens to account for the skipped calls, but not for the current call.

This off-by-one error explains why the results were more prominent when `n` is small.

image

Examples

When skip_n_calls is 1 we would do the following:

Before After
(bucket = 3) (bucket = 3)
call 1 take 1 token from redis - (bucket = 2) take 1 token from redis (bucket = 2)
call 2 skip (bucket = 2) skip (bucket = 2)
call 3 take n tokens from redis. n = 1. (bucket = 1) take n + 1 tokens from redis; one for the previously skipped call, and one for this call. (bucket = 0)

When skip_n_calls is 3 we would do the following:

Before After
(bucket = 5) (bucket = 5)
call 1 take 1 token from redis - (bucket = 4) take 1 token from redis (bucket = 4)
call 2 skip (bucket = 4) skip (bucket = 4)
call 3 skip (bucket = 4) skip (bucket = 4)
call 4 skip (bucket = 4) skip (bucket = 4)
call 5 take n tokens from redis. n = 3. (bucket = 1) take n + 1 tokens from redis; one for the previously skipped call, and one for this call. (bucket = 0)

References

Slack Discussion
Jira

Testing

  • Integration testing
  • Dev Environment Load Testing

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not the default branch

@LeweyM LeweyM changed the title fix: skip_n_calls take amount accounts for current call fix: [PSERV-2110] skip_n_calls take amount accounts for current call Dec 6, 2023
@LeweyM LeweyM marked this pull request as ready for review December 6, 2023 15:58
@LeweyM LeweyM requested a review from a team as a code owner December 6, 2023 15:58
@LeweyM LeweyM merged commit 8e92fbf into master Dec 12, 2023
2 checks passed
@LeweyM LeweyM mentioned this pull request Dec 12, 2023
3 tasks
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