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' 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/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