Consolidate client report and rate limit handling with data categories

CHANGELOG.md

@@ -3,6 +3,7 @@
 ### Features
 
 - Update key, unit and tags sanitization logic for metrics [#2292](https://github.com/getsentry/sentry-ruby/pull/2292)
+- Consolidate client report and rate limit handling with data categories [#2294](https://github.com/getsentry/sentry-ruby/pull/2294)
 
 ### Bug Fixes
 

sentry-ruby/lib/sentry/client.rb

@@ -49,24 +49,25 @@ def capture_event(event, scope, hint = {})
  return unless configuration.sending_allowed?
 
  if event.is_a?(ErrorEvent) && !configuration.sample_allowed?
- transport.record_lost_event(:sample_rate, 'event')
+ transport.record_lost_event(:sample_rate, 'error')
  return
  end
 
  event_type = event.is_a?(Event) ? event.type : event["type"]
+ data_category = Envelope::Item.data_category(event_type)
  event = scope.apply_to_event(event, hint)
 
  if event.nil?
  log_debug("Discarded event because one of the event processors returned nil")
- transport.record_lost_event(:event_processor, event_type)
+ transport.record_lost_event(:event_processor, data_category)
  return
  end
 
  if async_block = configuration.async
  dispatch_async_event(async_block, event, hint)
  elsif configuration.background_worker_threads != 0 && hint.fetch(:background, true)
  queued = dispatch_background_event(event, hint)
- transport.record_lost_event(:queue_overflow, event_type) unless queued
+ transport.record_lost_event(:queue_overflow, data_category) unless queued
  else
  send_event(event, hint)
  end
@@ -166,13 +167,14 @@ def event_from_transaction(transaction)
  # @!macro send_event
  def send_event(event, hint = nil)
  event_type = event.is_a?(Event) ? event.type : event["type"]
+ data_category = Envelope::Item.data_category(event_type)
 
  if event_type != TransactionEvent::TYPE && configuration.before_send
  event = configuration.before_send.call(event, hint)
 
  if event.nil?
  log_debug("Discarded event because before_send returned nil")
- transport.record_lost_event(:before_send, 'event')
+ transport.record_lost_event(:before_send, data_category)
  return
  end
  end
@@ -182,7 +184,7 @@ def send_event(event, hint = nil)
 
  if event.nil?
  log_debug("Discarded event because before_send_transaction returned nil")
- transport.record_lost_event(:before_send, 'transaction')
+ transport.record_lost_event(:before_send, data_category)
  return
  end
  end
@@ -193,7 +195,7 @@ def send_event(event, hint = nil)
  event
  rescue => e
  log_error("Event sending failed", e, debug: configuration.debug)
- transport.record_lost_event(:network_error, event_type)
+ transport.record_lost_event(:network_error, data_category)
  raise
  end
 

sentry-ruby/lib/sentry/envelope.rb

@@ -18,6 +18,23 @@ def type
  @headers[:type] || 'event'
  end
 
+ # rate limits and client reports use the data_category rather than envelope item type
+ def self.data_category(type)
+ case type
+ when 'session', 'attachment', 'transaction', 'profile' then type
+ when 'sessions' then 'session'
+ when 'check_in' then 'monitor'
+ when 'statsd', 'metric_meta' then 'metric_bucket'
+ when 'event' then 'error'
+ when 'client_report' then 'internal'
+ else 'default'
+ end
+ end
+
+ def data_category
+ self.class.data_category(type)
+ end
+
  def to_s
  [JSON.generate(@headers), @payload.is_a?(String) ? @payload : JSON.generate(@payload)].join("\n")
  end

sentry-ruby/lib/sentry/transport.rb

@@ -88,18 +88,9 @@ def serialize_envelope(envelope)
  [data, serialized_items]
  end
 
- def is_rate_limited?(item_type)
+ def is_rate_limited?(data_category)
  # check category-specific limit
- category_delay =
- case item_type
- when "transaction"
- @rate_limits["transaction"]
- when "sessions"
- @rate_limits["session"]
- else
- @rate_limits["error"]
- end
-
+ category_delay = @rate_limits[data_category]
  # check universal limit if not category limit
  universal_delay = @rate_limits[nil]
 
@@ -160,11 +151,11 @@ def envelope_from_event(event)
  envelope
  end
 
- def record_lost_event(reason, item_type)
+ def record_lost_event(reason, data_category)
  return unless @send_client_reports
  return unless CLIENT_REPORT_REASONS.include?(reason)
 
- @discarded_events[[reason, item_type]] += 1
+ @discarded_events[[reason, data_category]] += 1
  end
 
  def flush
@@ -184,11 +175,7 @@ def fetch_pending_client_report(force: false)
  return nil if @discarded_events.empty?
 
  discarded_events_hash = @discarded_events.map do |key, val|
- reason, type = key
-
- # 'event' has to be mapped to 'error'
- category = type == 'event' ? 'error' : type
-
+ reason, category = key
  { reason: reason, category: category, quantity: val }
  end
 
@@ -206,9 +193,9 @@ def fetch_pending_client_report(force: false)
 
  def reject_rate_limited_items(envelope)
  envelope.items.reject! do |item|
- if is_rate_limited?(item.type)
+ if is_rate_limited?(item.data_category)
  log_debug("[Transport] Envelope item [#{item.type}] not sent: rate limiting")
- record_lost_event(:ratelimit_backoff, item.type)
+ record_lost_event(:ratelimit_backoff, item.data_category)
 
  true
  else

sentry-ruby/spec/sentry/client/event_sending_spec.rb

@@ -35,7 +35,7 @@
  it "doesn't send the event when it's not sampled" do
  allow(Random).to receive(:rand).and_return(0.51)
  subject.capture_event(event, scope)
- expect(subject.transport).to have_recorded_lost_event(:sample_rate, 'event')
+ expect(subject.transport).to have_recorded_lost_event(:sample_rate, 'error')
  expect(subject.transport.events.count).to eq(0)
  end
  end
@@ -171,7 +171,7 @@
  allow(Sentry.background_worker).to receive(:perform).and_return(false)
 
  subject.capture_event(event, scope)
- expect(subject.transport).to have_recorded_lost_event(:queue_overflow, 'event')
+ expect(subject.transport).to have_recorded_lost_event(:queue_overflow, 'error')
 
  expect(subject.transport.events.count).to eq(0)
  sleep(0.2)
@@ -319,7 +319,7 @@
  it "discards the event and logs a info" do
  expect(subject.capture_event(event, scope)).to be_nil
 
- expect(subject.transport).to have_recorded_lost_event(:event_processor, 'event')
+ expect(subject.transport).to have_recorded_lost_event(:event_processor, 'error')
  expect(string_io.string).to match(/Discarded event because one of the event processors returned nil/)
  end
  end
@@ -360,7 +360,7 @@
  it "swallows and logs Sentry::ExternalError (caused by transport's networking error)" do
  expect(subject.capture_event(event, scope)).to be_nil
 
- expect(subject.transport).to have_recorded_lost_event(:network_error, 'event')
+ expect(subject.transport).to have_recorded_lost_event(:network_error, 'error')
  expect(string_io.string).to match(/Event sending failed: Failed to open TCP connection/)
  expect(string_io.string).to match(/Event capturing failed: Failed to open TCP connection/)
  end
@@ -383,7 +383,7 @@
  expect(subject.capture_event(event, scope)).to be_a(Sentry::ErrorEvent)
  sleep(0.2)
 
- expect(subject.transport).to have_recorded_lost_event(:network_error, 'event')
+ expect(subject.transport).to have_recorded_lost_event(:network_error, 'error')
  expect(string_io.string).to match(/Event sending failed: Failed to open TCP connection/)
  end
 
@@ -471,7 +471,7 @@
 
  it "records lost event" do
  subject.send_event(event)
- expect(subject.transport).to have_recorded_lost_event(:before_send, 'event')
+ expect(subject.transport).to have_recorded_lost_event(:before_send, 'error')
  end
  end
 

sentry-ruby/spec/sentry/envelope_spec.rb

@@ -0,0 +1,23 @@
+require "spec_helper"
+
+RSpec.describe Sentry::Envelope::Item do
+ describe '.data_category' do
+ [
+ ['session', 'session'],
+ ['sessions', 'session'],
+ ['attachment', 'attachment'],
+ ['transaction', 'transaction'],
+ ['profile', 'profile'],
+ ['check_in', 'monitor'],
+ ['statsd', 'metric_bucket'],
+ ['metric_meta', 'metric_bucket'],
+ ['event', 'error'],
+ ['client_report', 'internal'],
+ ['unknown', 'default']
+ ].each do |item_type, data_category|
+ it "maps item type #{item_type} to data category #{data_category}" do
+ expect(described_class.data_category(item_type)).to eq(data_category)
+ end
+ end
+ end
+end

sentry-ruby/spec/sentry/transport/http_transport_rate_limiting_spec.rb

@@ -26,40 +26,40 @@
  "transaction" => Time.now + 60,
  "session" => Time.now + 60)
 
- expect(subject.is_rate_limited?("event")).to eq(true)
+ expect(subject.is_rate_limited?("error")).to eq(true)
  expect(subject.is_rate_limited?("transaction")).to eq(true)
- expect(subject.is_rate_limited?("sessions")).to eq(true)
+ expect(subject.is_rate_limited?("session")).to eq(true)
  end
 
  it "returns false for passed limited category" do
  subject.rate_limits.merge!("error" => Time.now - 10,
  "transaction" => Time.now - 10,
  "session" => Time.now - 10)
 
- expect(subject.is_rate_limited?("event")).to eq(false)
+ expect(subject.is_rate_limited?("error")).to eq(false)
  expect(subject.is_rate_limited?("transaction")).to eq(false)
- expect(subject.is_rate_limited?("sessions")).to eq(false)
+ expect(subject.is_rate_limited?("session")).to eq(false)
  end
 
  it "returns false for not listed category" do
  subject.rate_limits.merge!("transaction" => Time.now + 10)
 
- expect(subject.is_rate_limited?("event")).to eq(false)
- expect(subject.is_rate_limited?("sessions")).to eq(false)
+ expect(subject.is_rate_limited?("error")).to eq(false)
+ expect(subject.is_rate_limited?("session")).to eq(false)
  end
  end
 
  context "with only universal limits" do
  it "returns true when still limited" do
  subject.rate_limits.merge!(nil => Time.now + 60)
 
- expect(subject.is_rate_limited?("event")).to eq(true)
+ expect(subject.is_rate_limited?("error")).to eq(true)
  end
 
  it "returns false when passed limit" do
  subject.rate_limits.merge!(nil => Time.now - 10)
 
- expect(subject.is_rate_limited?("event")).to eq(false)
+ expect(subject.is_rate_limited?("error")).to eq(false)
  end
  end
 
@@ -70,14 +70,14 @@
  nil => Time.now - 10
  )
 
- expect(subject.is_rate_limited?("event")).to eq(true)
+ expect(subject.is_rate_limited?("error")).to eq(true)
 
  subject.rate_limits.merge!(
  "error" => Time.now - 60,
  nil => Time.now + 10
  )
 
- expect(subject.is_rate_limited?("event")).to eq(true)
+ expect(subject.is_rate_limited?("error")).to eq(true)
  end
  end
  end

sentry-ruby/spec/sentry/transport_spec.rb

@@ -494,7 +494,7 @@
 
  it "records lost event" do
  subject.send_event(event)
- expect(subject).to have_recorded_lost_event(:ratelimit_backoff, 'event')
+ expect(subject).to have_recorded_lost_event(:ratelimit_backoff, 'error')
  end
  end
  end

sentry-ruby/spec/spec_helper.rb

@@ -50,9 +50,9 @@
  skip("skip rack related tests") unless defined?(Rack)
  end
 
- RSpec::Matchers.define :have_recorded_lost_event do |reason, type|
+ RSpec::Matchers.define :have_recorded_lost_event do |reason, data_category|
  match do |transport|
- expect(transport.discarded_events[[reason, type]]).to be > 0
+ expect(transport.discarded_events[[reason, data_category]]).to be > 0
  end
  end
 end