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.

Sign up for GitHub

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

Jump to bottom

feat(transports): Category-based Rate Limiting #354

Merged
merged 7 commits into from
May 21, 2021
Merged

feat(transports): Category-based Rate Limiting #354

merged 7 commits into from
May 21, 2021

Conversation

rhcarvalho
Copy link
Contributor

This adds support for parsing the X-Sentry-Rate-Limits and rate limiting errors and transactions independently.

@rhcarvalho
ci: Disable lll linter
4a8bb54 
It has not given us much value. Instead of adding 'nolint' lines for the
few cases where we do need long lines, better not have this linter and
keep the code base free of linter pragmas.
@rhcarvalho
Copy link
Contributor Author

Fixing the linter failure by disabling the lll linter in #355.

AbhiPrasad
AbhiPrasad reviewed May 20, 2021
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Table tests 🎉

I only have small formatting concerns and some questions (mostly questions 😄), this looks really good. Will approve after comments get addressed.

internal/ratelimit/deadline.go Show resolved Hide resolved
internal/ratelimit/map.go Show resolved Hide resolved
internal/ratelimit/map.go Outdated Show resolved Hide resolved
internal/ratelimit/retry_after.go Show resolved Hide resolved
transport.go Show resolved Hide resolved
transport.go Outdated Show resolved Hide resolved
transport_test.go Show resolved Hide resolved
@rhcarvalho
fixup: Remove unnecessary name on return argument
cf3ff6d
@rhcarvalho
fix(HTTPSyncTransport): Synchronize rate limit updates
ccd00a7 
If the same transport is shared across goroutines, we need to
synchronize reads and writes to the rate limit map.

The transport is shared and used concurrently when we clone and use hubs
from multiple goroutines.

Note that previously there was a data race updating the `disabledUntil`
field.
@rhcarvalho
fix(HTTPTransport): Merge rate limit updates instead of replacing
3c12f31
@rhcarvalho
test: Ensure transports respect rate limits
dce71db
@rhcarvalho
ref: io.ReadAll => ioutil.ReadAll
eb391f0 
io.ReadAll was added in Go 1.16, but we support older versions too.
We have to use the one from ioutil for compatibility.
AbhiPrasad
AbhiPrasad approved these changes May 21, 2021
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

After sleeping on it, I think the current file structure makes the most sense.

internal/ratelimit/retry_after.go Show resolved Hide resolved
@rhcarvalho rhcarvalho merged commit 3b083ad into master May 21, 2021
@rhcarvalho rhcarvalho deleted the rhcarvalho/category-rate-limits branch May 21, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

@kamilogorek kamilogorek Awaiting requested review from kamilogorek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rhcarvalho @AbhiPrasad