Skip to content

Conversation

@LukasJenicek
Copy link
Contributor

No description provided.

@github-actions
Copy link

Benchmark Results

goos: linux
goarch: amd64
pkg: github.com/go-chi/httprate
cpu: AMD EPYC 7763 64-Core Processor                
               │ master.txt  │            pr.txt             │
               │   sec/op    │   sec/op     vs base          │
LocalCounter-4   19.90m ± 1%   19.92m ± 1%  ~ (p=0.853 n=10)

               │  master.txt  │             pr.txt             │
               │     B/op     │     B/op      vs base          │
LocalCounter-4   2.830Mi ± 0%   2.829Mi ± 0%  ~ (p=0.739 n=10)

               │ master.txt  │            pr.txt             │
               │  allocs/op  │  allocs/op   vs base          │
LocalCounter-4   121.4k ± 0%   121.4k ± 0%  ~ (p=0.753 n=10)

"golang.org/x/time/rate"
)

func RateLimitedRequest(limit rate.Limit, burst int) func(http.RoundTripper) http.RoundTripper {
Copy link
Contributor

Choose a reason for hiding this comment

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

HTTP rate-limit outgoing requests. Nice 👍

This is a good start. Few notes:

  • Could we reuse the existing LimitCounter implementations (same as for incoming requests), if that's possible? Meaning we would reuse the existing in-memory counter and redis counter.
  • Let's hide "golang.org/x/time/rate" from the pkg API (FYI, this package doesn't use limit/burst - it uses sliding window counter instead)
  • Can you think of the package API a bit?
    • Do we need to rate-limit 1) a specific domain (e.g. key: httprate:example.com); 2) or all outgoing requests made through the *http.Client? -- is (2) enough?
    •   package httprate
        
        func Client(c *http.Client, requestLimit int, windowLength time.Duration, key string, ...opts) *httprate.LimitClient 
        
        func Transport(rt http.RoundTripper, requestLimit int, windowLength time.Duration, key string, ...opts) http.RoundTripper

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