Skip to content

Conversation

@st0012
Copy link
Collaborator

@st0012 st0012 commented Mar 13, 2021

Sentry rate limits different types of events. And when rate limiting is enabled, it sends back a 429 response to the SDK. Currently, the SDK would then raise an error like this:

Unable to record event with remote Sentry server (Sentry::Error - the server responded with status 429
body: {"detail":"event rejected due to rate limit"}):

This change improves the SDK's handling on such responses by:

  • Not treating them as errors, so you don't see the noise anymore.
  • Halting event sending for a while according to the duration provided in the response. And warns you with messages like:
Envelope [event] not sent: Excluded by random sample

Closes #1072

@st0012 st0012 added this to the 4.4.0 milestone Mar 13, 2021
@st0012 st0012 self-assigned this Mar 13, 2021
@st0012 st0012 requested a review from jan-auer March 14, 2021 05:40
@codecov-io
Copy link

codecov-io commented Mar 18, 2021

Codecov Report

Merging #1336 (1126dcd) into master (86dcf3c) will increase coverage by 0.63%.
The diff coverage is 99.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1336      +/-   ##
==========================================
+ Coverage   98.08%   98.71%   +0.63%     
==========================================
  Files         208      114      -94     
  Lines        9173     5538    -3635     
==========================================
- Hits         8997     5467    -3530     
+ Misses        176       71     -105     
Impacted Files Coverage Δ
sentry-ruby/spec/sentry/hub_spec.rb 100.00% <ø> (ø)
...try/transport/http_transport_rate_limiting_spec.rb 99.17% <99.17%> (ø)
sentry-ruby/lib/sentry/transport.rb 98.41% <100.00%> (+1.19%) ⬆️
sentry-ruby/lib/sentry/transport/http_transport.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/configuration_spec.rb 98.61% <100.00%> (ø)
sentry-ruby/spec/sentry/transport_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry_spec.rb 100.00% <100.00%> (ø)
sentry-raven/spec/raven/processors/cookies_spec.rb
sentry-raven/lib/raven/context.rb
sentry-raven/lib/raven/processor/cookies.rb
... and 93 more

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 86dcf3c...1126dcd. Read the comment docs.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Thank you! Please see the comments below. There is one request for change in the code checking for cached limits, and a few suggestions and clarifying questions.

@st0012 st0012 requested a review from jan-auer March 26, 2021 11:10
st0012 added 9 commits April 10, 2021 15:40
If Sentry's 200 responses also contain rate limit related headers, we
should store and update the SDK's rate limits like we see them in 429
responses.

The only difference is that when receiving 429 responses the SDK should
add default 60s delay to all event categories, which must not happen to
200 responses.
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.

[4.0] Category-based Ratelimiting

4 participants