Skip to content

ref(envelopes): Move rate limiting logic to each item in envelope#1742

Merged
st0012 merged 2 commits intomasterfrom
ref-envelope-rate-limit
Feb 22, 2022
Merged

ref(envelopes): Move rate limiting logic to each item in envelope#1742
st0012 merged 2 commits intomasterfrom
ref-envelope-rate-limit

Conversation

@sl0thentr0py
Copy link
Copy Markdown
Member

This makes the envelope logic a bit more consistent to other SDKs because the rate limits can apply to each item in the envelope separately. This is necessary to be able to work with session envelopes in #1715.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 21, 2022

Codecov Report

Merging #1742 (d940f73) into master (8446668) will increase coverage by 0.02%.
The diff coverage is 100.00%.

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1742      +/-   ##
==========================================
+ Coverage   98.39%   98.42%   +0.02%     
==========================================
  Files         141      141              
  Lines        8030     8046      +16     
==========================================
+ Hits         7901     7919      +18     
+ Misses        129      127       -2     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/envelope.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transport.rb 98.87% <100.00%> (+0.05%) ⬆️
sentry-ruby/spec/sentry/rake_spec.rb 100.00% <100.00%> (ø)
...try/transport/http_transport_rate_limiting_spec.rb 99.11% <100.00%> (ø)
...-ruby/spec/sentry/transport/http_transport_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/transport_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry_spec.rb 99.74% <100.00%> (ø)
sentry-resque/lib/sentry/resque.rb 97.14% <0.00%> (-2.86%) ⬇️
sentry-ruby/lib/sentry/breadcrumb.rb 100.00% <0.00%> (+3.70%) ⬆️
sentry-ruby/lib/sentry/background_worker.rb 100.00% <0.00%> (+5.40%) ⬆️

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 8446668...e8e8bf7. Read the comment docs.

[item_header, item_payload]
end

def process_rate_limits(envelope)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think reject_rate_limited_items gives a better intention here. Or it's possible to extend this method in the future?

def process_rate_limits(envelope)
envelope.items.reject! do |item|
if is_rate_limited?(item.type)
log_info("[Transport] Envelope item [#{item.type}] not sent: rate limiting")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not related to this PR: I'm thinking downgrading most of the info logs to debug level. Because from my experience, we rarely need to know the sent events, not even in debugging. Wdyt?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yea makes sense, will do

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hmm let's do this in a separate PR, I'll need to change a lot of specs otherwise :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I didn't mean we should change it here. And we should also change some other places instead of just here..

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Feb 22, 2022

@sl0thentr0py btw we'll need a changelog entry for this as well.

@st0012 st0012 merged commit 75c066b into master Feb 22, 2022
@st0012 st0012 deleted the ref-envelope-rate-limit branch February 22, 2022 17:35
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