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

Allow configuring custom slack #64

Merged
merged 1 commit into from
Mar 3, 2021
Merged

Allow configuring custom slack #64

merged 1 commit into from
Mar 3, 2021

Conversation

rabbbit
Copy link
Contributor

@rabbbit rabbbit commented Feb 7, 2021

Resolves #20

This is to allow people to configure custom slack - we previously
defaulted to 10, and only allowed to disable it.

The only tricky part about this is handling "per" correctly - adding
a bunch of test cases to cover it.

The tests are large, but when I tried to generate them dynamically
(the same test cases 2*, for both with-per and without-per) it became
even harder to read.

@codecov
Copy link

codecov bot commented Feb 7, 2021

Codecov Report

Merging #64 (8b1d825) into main (7e71567) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #64   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           69        70    +1     
=========================================
+ Hits            69        70    +1     
Impacted Files Coverage Δ
ratelimit.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e71567...baf1224. Read the comment docs.

@rabbbit rabbbit requested a review from cinchurge February 7, 2021 03:23
// - faster limiter running for 1 second
// - slack accumulated by the faster limiter during the two seconds.
// it was blocked by slower limiter.
tests := []struct {
Copy link
Contributor Author

@rabbbit rabbbit Feb 8, 2021

Choose a reason for hiding this comment

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

I also had the same test generated dynamically:

	const (
		slowRate = 10
		fastRate = 100
	)

	// Test for a few Per settings.
	testsPer := []time.Duration{
		// Default.
		{time.Second},
		{time.Millisecond * 500},
	}
	// And a few different slack options
	testsSlack := []int{10, 20, 100, 150}

	for _, per := range testsPer {
		for _, slack := range testsSlack {
			testKey := fmt.String("per-%v-slack-%v", per, slack)
			t.Run(testKey, func(t *testing.T) {
				runTest(t, func(r testRunner) {
					slow := r.createLimiter(slowRate, WithoutSlack)
					fast := r.createLimiter(fastRate, tt.opt...)

					r.startTaking(slow, fast)

					r.afterFunc(2, func() {
						r.startTaking(fast)
						r.startTaking(fast)
					})

					// slow limiter with 10hz dominates here - we're always at it's limit.
					r.assertCountAt(1*time.Second, slowRate)

					// results:
					// - for two seconds we ran a slow rate
					// - for a second we ran a fast rate
					// - and we add the slack
					// rates have to be adjusted for Per
					adjustedSlow := slowRate * int(time.Second/testsPer)
					asjustedFast := fastRate * int(time.Second/testsPer)
					r.assertCountAt(3*time.Second, asjustedSlow+adjustedFast+slack)
				})
			})
		}
	}

but it seems like manual definition is easier to understand at this point.

rabbbit added a commit that referenced this pull request Feb 15, 2021
This is a split off #64 to make #64 smaller/easier.

I believe out current treatment of the default (10) slack doesn't
work very well with Per option.

We configure the limiter with an limit:
- `Limiter(10)` - means, allow 10 requests per second, with 10 requests
  slack - "slack period" is 1 second

However, if we do:
- `Limiter(10, Per(time.Minute))` we get - 10 requests per minute,
  but the slack is still 10 *per second*, meaning the slack period is
  stil 10 second - effectively 1 request.
I believe this is confusing, because with "slack of 10", I'd expect
a 1 minute "slack period".

After this change:
- `Limiter(10, Per(time.Minute))` we get - 10 requests per minute,
  and a slack of 10 requests - so, slack period is 1 minute.

We're basically making it so that both "rate" and "slack" are expressed
in the same unit (ints) and that "Per" applies to both of them.

Note this is currently of minor impact since we're hardcoding the
slack as 10. This will become way more painful/complicated with #64.
@rabbbit rabbbit marked this pull request as draft February 15, 2021 19:24
Copy link

@cinchurge cinchurge left a comment

Choose a reason for hiding this comment

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

LGTM!

rabbbit added a commit that referenced this pull request Feb 25, 2021
This is a split off #64 to make #64 smaller/easier.

I believe out current treatment of the default (10) slack doesn't
work very well with Per option.

We configure the limiter with an limit:
- `Limiter(10)` - means, allow 10 requests per second, with 10 requests
  slack - "slack period" is 1 second

However, if we do:
- `Limiter(10, Per(time.Minute))` we get - 10 requests per minute,
  but the slack is still 10 *per second*, meaning the slack period is
  stil 10 second - effectively 1 request.
I believe this is confusing, because with "slack of 10", I'd expect
a 1 minute "slack period".

After this change:
- `Limiter(10, Per(time.Minute))` we get - 10 requests per minute,
  and a slack of 10 requests - so, slack period is 1 minute.

We're basically making it so that both "rate" and "slack" are expressed
in the same unit (ints) and that "Per" applies to both of them.

Note this is currently of minor impact since we're hardcoding the
slack as 10. This will become way more painful/complicated with #64.
@rabbbit rabbbit force-pushed the slack branch 2 times, most recently from 72c0901 to 7ae41d6 Compare March 1, 2021 15:06
Resolves #20

This is to allow people to configure custom slack - we previously
defaulted to 10, and only allowed to disable it.

The tests are a bit painful to read, I had a stab at generating
the same set of tests for both with, and without per, but that was
even more hard to read.
@rabbbit rabbbit marked this pull request as ready for review March 1, 2021 15:09
@rabbbit rabbbit merged commit 4e70b7f into main Mar 3, 2021
@rabbbit rabbbit deleted the slack branch March 3, 2021 01:31
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.

maxSlack can not be self defined.
2 participants