diff --git a/CHANGELOG.md b/CHANGELOG.md index 5acb9f05d..2545ea1fb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 5eb44fc8a..5dccffdc9 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -49,16 +49,17 @@ 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 @@ -66,7 +67,7 @@ def capture_event(event, scope, hint = {}) 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 diff --git a/sentry-ruby/lib/sentry/envelope.rb b/sentry-ruby/lib/sentry/envelope.rb index 6d9ff56ff..b446d6903 100644 --- a/sentry-ruby/lib/sentry/envelope.rb +++ b/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 diff --git a/sentry-ruby/lib/sentry/transport.rb b/sentry-ruby/lib/sentry/transport.rb index 1d4366a95..326dcbf35 100644 --- a/sentry-ruby/lib/sentry/transport.rb +++ b/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 diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb index d562f287a..86098a469 100644 --- a/sentry-ruby/spec/sentry/client/event_sending_spec.rb +++ b/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 diff --git a/sentry-ruby/spec/sentry/transport/http_transport_rate_limiting_spec.rb b/sentry-ruby/spec/sentry/transport/http_transport_rate_limiting_spec.rb index 28fd444ec..ac83e3b16 100644 --- a/sentry-ruby/spec/sentry/transport/http_transport_rate_limiting_spec.rb +++ b/sentry-ruby/spec/sentry/transport/http_transport_rate_limiting_spec.rb @@ -26,9 +26,9 @@ "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 @@ -36,16 +36,16 @@ "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 @@ -53,13 +53,13 @@ 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 diff --git a/sentry-ruby/spec/sentry/transport_spec.rb b/sentry-ruby/spec/sentry/transport_spec.rb index 23bf3322a..7c90f4be7 100644 --- a/sentry-ruby/spec/sentry/transport_spec.rb +++ b/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 diff --git a/sentry-ruby/spec/spec_helper.rb b/sentry-ruby/spec/spec_helper.rb index 917871a3a..1d4a6ff15 100644 --- a/sentry-ruby/spec/spec_helper.rb +++ b/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