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: throttling #21

Merged
merged 4 commits into from
Apr 13, 2023
Merged

feat: throttling #21

merged 4 commits into from
Apr 13, 2023

Conversation

fracasula
Copy link
Collaborator

Description

I think this library is generic enough to live in the kit. Please provide feedback.

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Patch coverage: 75.22% and project coverage change: +0.89 🎉

Comparison is base (940da1a) 76.31% compared to head (007e367) 77.21%.

❗ Current head 007e367 differs from pull request most recent head e7a13e9. Consider uploading reports for the commit e7a13e9 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   76.31%   77.21%   +0.89%     
==========================================
  Files          41       47       +6     
  Lines        2820     3155     +335     
==========================================
+ Hits         2152     2436     +284     
- Misses        563      594      +31     
- Partials      105      125      +20     
Impacted Files Coverage Δ
testhelper/docker/resource/redis.go 43.75% <43.75%> (ø)
throttling/options.go 68.18% <68.18%> (ø)
throttling/return.go 71.42% <71.42%> (ø)
throttling/throttling.go 71.87% <71.87%> (ø)
throttling/memory_gcra.go 80.00% <80.00%> (ø)
cachettl/cachettl.go 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -0,0 +1,75 @@
package resource
Copy link
Collaborator

Choose a reason for hiding this comment

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

since we are moving this to commons, it would be wise to add support for overriding the docker image tag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, will add that 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done here.


const defaultMaxCASAttemptsLimit = 100

type gcra struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this specific implementation we are effectively using 1+N stores:

  1. One cachettl which holds all different GCRARateLimiterCtxs.
  2. Multiple (throttled) memstores, one for every instance of GCRARateLimiterCtx that we are creating.

It should be possible to use our own implementation of throttled.GCRAStoreCtx e.g. by leveraging cachettl and avoid using throttled's own memstore. Or since each GCRARateLimiterCtxwe are creating is handling a single key, we can make a store that always holds a singular item :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@atzoum what do you think of this approach?

The benchmark gives:

goos: linux
goarch: amd64
pkg: github.com/rudderlabs/rudder-go-kit/throttling
cpu: 12th Gen Intel(R) Core(TM) i9-12900K
BenchmarkInMemoryGCRA
BenchmarkInMemoryGCRA/one_unlimited_store_per_throttler
BenchmarkInMemoryGCRA/one_unlimited_store_per_throttler/single_key
BenchmarkInMemoryGCRA/one_unlimited_store_per_throttler/single_key-24         	 8378788	       141.1 ns/op	       0 B/op	       0 allocs/op
BenchmarkInMemoryGCRA/one_unlimited_store_per_throttler/multiple_keys
BenchmarkInMemoryGCRA/one_unlimited_store_per_throttler/multiple_keys-24      	 6042226	       197.6 ns/op	      46 B/op	       0 allocs/op
BenchmarkInMemoryGCRA/one_unlimited_store_for_all_throttlers
BenchmarkInMemoryGCRA/one_unlimited_store_for_all_throttlers/single_key
BenchmarkInMemoryGCRA/one_unlimited_store_for_all_throttlers/single_key-24    	 8469865	       141.0 ns/op	       0 B/op	       0 allocs/op
BenchmarkInMemoryGCRA/one_unlimited_store_for_all_throttlers/multiple_keys
BenchmarkInMemoryGCRA/one_unlimited_store_for_all_throttlers/multiple_keys-24 	 5978118	       223.6 ns/op	      24 B/op	       0 allocs/op
BenchmarkInMemoryGCRA/one_single_key_store_per_throttler
BenchmarkInMemoryGCRA/one_single_key_store_per_throttler/single_key
BenchmarkInMemoryGCRA/one_single_key_store_per_throttler/single_key-24        	 6954384	       169.8 ns/op	      16 B/op	       1 allocs/op
BenchmarkInMemoryGCRA/one_single_key_store_per_throttler/multiple_keys
BenchmarkInMemoryGCRA/one_single_key_store_per_throttler/multiple_keys-24     	 4416854	       285.8 ns/op	     102 B/op	       3 allocs/op
BenchmarkInMemoryGCRA/custom_store_per_throttler
BenchmarkInMemoryGCRA/custom_store_per_throttler/single_key
BenchmarkInMemoryGCRA/custom_store_per_throttler/single_key-24                	 9134334	       130.0 ns/op	       0 B/op	       0 allocs/op
BenchmarkInMemoryGCRA/custom_store_per_throttler/multiple_keys
BenchmarkInMemoryGCRA/custom_store_per_throttler/multiple_keys-24             	 7789651	       161.9 ns/op	      16 B/op	       0 allocs/op
PASS
ok  	github.com/rudderlabs/rudder-go-kit/throttling	21.661s

The last one (i.e. custom_store_per_throttler) is the new implementation.

@fracasula fracasula merged commit 2423d26 into main Apr 13, 2023
@fracasula fracasula deleted the feat.throttling branch April 13, 2023 15:06
@github-actions github-actions bot mentioned this pull request Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants