From a530085c8f67aefb434278588109fc7cbb3140c0 Mon Sep 17 00:00:00 2001 From: PikachuEXE Date: Tue, 3 May 2022 15:46:43 +0800 Subject: [PATCH 1/3] Fix: Handle exception with large stacktrace without dropping entire item Certain exception type such as `SystemStackError` has long backtrace (thus the stack error) The whole envelope item was dropped due to payload size limit logic This ensures it tries to remove most of the stacktrace frames (except first 10) when payload is too large, so that the envelope item won't be dropped = exception still reported --- sentry-ruby/lib/sentry/transport.rb | 21 +++++ sentry-ruby/spec/sentry/transport_spec.rb | 104 +++++++++++++++++----- 2 files changed, 102 insertions(+), 23 deletions(-) diff --git a/sentry-ruby/lib/sentry/transport.rb b/sentry-ruby/lib/sentry/transport.rb index 9524b8d3d..5904c1d6d 100644 --- a/sentry-ruby/lib/sentry/transport.rb +++ b/sentry-ruby/lib/sentry/transport.rb @@ -9,6 +9,7 @@ class Transport PROTOCOL_VERSION = '7' USER_AGENT = "sentry-ruby/#{Sentry::VERSION}" CLIENT_REPORT_INTERVAL = 30 + STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD = 500 # https://develop.sentry.dev/sdk/client-reports/#envelope-item-payload CLIENT_REPORT_REASONS = [ @@ -82,6 +83,26 @@ def serialize_envelope(envelope) result = item.to_s end + if result.bytesize > Event::MAX_SERIALIZED_PAYLOAD_SIZE + if single_exceptions = item.payload.dig(:exception, :values) + single_exceptions.each do |single_exception| + traces = single_exception.dig(:stacktrace, :frames) + if traces && traces.size > STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD + traces.replace(traces[-STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD..-1]) + end + end + elsif single_exceptions = item.payload.dig("exception", "values") + single_exceptions.each do |single_exception| + traces = single_exception.dig("stacktrace", "frames") + if traces && traces.size > STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD + traces.replace(traces[-STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD..-1]) + end + end + end + + result = item.to_s + end + if result.bytesize > Event::MAX_SERIALIZED_PAYLOAD_SIZE size_breakdown = item.payload.map do |key, value| "#{key}: #{JSON.generate(value).bytesize}" diff --git a/sentry-ruby/spec/sentry/transport_spec.rb b/sentry-ruby/spec/sentry/transport_spec.rb index 7b038fd7f..d8724e675 100644 --- a/sentry-ruby/spec/sentry/transport_spec.rb +++ b/sentry-ruby/spec/sentry/transport_spec.rb @@ -100,40 +100,98 @@ end context "oversized event" do - let(:event) { client.event_from_message("foo") } - let(:envelope) { subject.envelope_from_event(event) } + context "due to breadcrumb" do + let(:event) { client.event_from_message("foo") } + let(:envelope) { subject.envelope_from_event(event) } - before do - event.breadcrumbs = Sentry::BreadcrumbBuffer.new(100) - 100.times do |i| - event.breadcrumbs.record Sentry::Breadcrumb.new(category: i.to_s, message: "x" * Sentry::Event::MAX_MESSAGE_SIZE_IN_BYTES) + before do + event.breadcrumbs = Sentry::BreadcrumbBuffer.new(100) + 100.times do |i| + event.breadcrumbs.record Sentry::Breadcrumb.new(category: i.to_s, message: "x" * Sentry::Event::MAX_MESSAGE_SIZE_IN_BYTES) + end + serialized_result = JSON.generate(event.to_hash) + expect(serialized_result.bytesize).to be > Sentry::Event::MAX_SERIALIZED_PAYLOAD_SIZE end - serialized_result = JSON.generate(event.to_hash) - expect(serialized_result.bytesize).to be > Sentry::Event::MAX_SERIALIZED_PAYLOAD_SIZE - end - it "removes breadcrumbs and carry on" do - data, _ = subject.serialize_envelope(envelope) - expect(data.bytesize).to be < Sentry::Event::MAX_SERIALIZED_PAYLOAD_SIZE + it "removes breadcrumbs and carry on" do + data, _ = subject.serialize_envelope(envelope) + expect(data.bytesize).to be < Sentry::Event::MAX_SERIALIZED_PAYLOAD_SIZE + + expect(envelope.items.count).to eq(1) + + event_item = envelope.items.first + expect(event_item.payload[:breadcrumbs]).to be_nil + end - expect(envelope.items.count).to eq(1) + context "if it's still oversized" do + before do + 100.times do |i| + event.contexts["context_#{i}"] = "s" * Sentry::Event::MAX_MESSAGE_SIZE_IN_BYTES + end + end - event_item = envelope.items.first - expect(event_item.payload[:breadcrumbs]).to be_nil + it "rejects the item and logs attributes size breakdown" do + data, _ = subject.serialize_envelope(envelope) + expect(data).to be_nil + expect(io.string).not_to match(/Sending envelope with items \[event\]/) + expect(io.string).to match(/tags: 2, contexts: 820791, extra: 2/) + end + end end - context "if it's still oversized" do + context "due to stacktrace frames" do + let(:event) { client.event_from_exception(SystemStackError.new("stack level too deep")) } + let(:envelope) { subject.envelope_from_event(event) } + + let(:in_app_pattern) do + project_root = "/fake/project_root" + Regexp.new("^(#{project_root}/)?#{Sentry::Backtrace::APP_DIRS_PATTERN}") + end + let(:frame_list_size) { Sentry::Transport::STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD * 4 } + before do - 100.times do |i| - event.contexts["context_#{i}"] = "s" * Sentry::Event::MAX_MESSAGE_SIZE_IN_BYTES - end + single_exception = event.exception.instance_variable_get(:@values)[0] + new_stacktrace = Sentry::StacktraceInterface.new( + frames: frame_list_size.times.map do |zero_based_index| + Sentry::StacktraceInterface::Frame.new( + "/fake/path", + Sentry::Backtrace::Line.parse("app.rb:#{zero_based_index + 1}:in `/'", in_app_pattern) + ) + end, + ) + single_exception.instance_variable_set(:@stacktrace, new_stacktrace) + + serialized_result = JSON.generate(event.to_hash) + expect(serialized_result.bytesize).to be > Sentry::Event::MAX_SERIALIZED_PAYLOAD_SIZE end - it "rejects the item and logs attributes size breakdown" do + it "keeps some stacktrace frames and carry on" do data, _ = subject.serialize_envelope(envelope) - expect(data).to be_nil - expect(io.string).not_to match(/Sending envelope with items \[event\]/) - expect(io.string).to match(/tags: 2, contexts: 820791, extra: 2/) + expect(data.bytesize).to be < Sentry::Event::MAX_SERIALIZED_PAYLOAD_SIZE + + expect(envelope.items.count).to eq(1) + + event_item = envelope.items.first + frames = event_item.payload[:exception][:values][0][:stacktrace][:frames] + expect(frames.length).to eq(Sentry::Transport::STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD) + expect(frames[-1][:lineno]).to eq(frame_list_size) + expect(frames[-1][:filename]).to eq('app.rb') + expect(frames[-1][:function]).to eq('/') + end + + context "if it's still oversized" do + before do + 100.times do |i| + event.contexts["context_#{i}"] = "s" * Sentry::Event::MAX_MESSAGE_SIZE_IN_BYTES + end + end + + it "rejects the item and logs attributes size breakdown" do + data, _ = subject.serialize_envelope(envelope) + expect(data).to be_nil + expect(io.string).not_to match(/Sending envelope with items \[event\]/) + expect(io.string).to match(/tags: 2, contexts: 820791, extra: 2/) + end end end end From 720862d760b2b699dc91a7ce79f2b04bb0df08fc Mon Sep 17 00:00:00 2001 From: PikachuEXE Date: Tue, 10 May 2022 11:10:59 +0800 Subject: [PATCH 2/3] Update frame truncating to keep first & last N/2 frames instead of just last N frames --- sentry-ruby/lib/sentry/transport.rb | 10 ++++++++-- sentry-ruby/spec/sentry/transport_spec.rb | 21 +++++++++++++++++++-- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/sentry-ruby/lib/sentry/transport.rb b/sentry-ruby/lib/sentry/transport.rb index 5904c1d6d..bfe723147 100644 --- a/sentry-ruby/lib/sentry/transport.rb +++ b/sentry-ruby/lib/sentry/transport.rb @@ -88,14 +88,20 @@ def serialize_envelope(envelope) single_exceptions.each do |single_exception| traces = single_exception.dig(:stacktrace, :frames) if traces && traces.size > STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD - traces.replace(traces[-STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD..-1]) + size_on_both_ends = STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD / 2 + traces.replace( + traces[0..(size_on_both_ends - 1)] + traces[-size_on_both_ends..-1], + ) end end elsif single_exceptions = item.payload.dig("exception", "values") single_exceptions.each do |single_exception| traces = single_exception.dig("stacktrace", "frames") if traces && traces.size > STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD - traces.replace(traces[-STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD..-1]) + size_on_both_ends = STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD / 2 + traces.replace( + traces[0..(size_on_both_ends - 1)] + traces[-size_on_both_ends..-1], + ) end end end diff --git a/sentry-ruby/spec/sentry/transport_spec.rb b/sentry-ruby/spec/sentry/transport_spec.rb index d8724e675..87b2dda2f 100644 --- a/sentry-ruby/spec/sentry/transport_spec.rb +++ b/sentry-ruby/spec/sentry/transport_spec.rb @@ -147,7 +147,8 @@ project_root = "/fake/project_root" Regexp.new("^(#{project_root}/)?#{Sentry::Backtrace::APP_DIRS_PATTERN}") end - let(:frame_list_size) { Sentry::Transport::STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD * 4 } + let(:frame_list_limit) { Sentry::Transport::STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD } + let(:frame_list_size) { frame_list_limit * 4 } before do single_exception = event.exception.instance_variable_get(:@values)[0] @@ -173,10 +174,26 @@ event_item = envelope.items.first frames = event_item.payload[:exception][:values][0][:stacktrace][:frames] - expect(frames.length).to eq(Sentry::Transport::STACKTRACE_FRAME_LIMIT_ON_OVERSIZED_PAYLOAD) + expect(frames.length).to eq(frame_list_limit) + + # Last N lines kept + # N = Frame limit / 2 expect(frames[-1][:lineno]).to eq(frame_list_size) expect(frames[-1][:filename]).to eq('app.rb') expect(frames[-1][:function]).to eq('/') + # + expect(frames[-(frame_list_limit / 2)][:lineno]).to eq(frame_list_size - ((frame_list_limit / 2) - 1)) + expect(frames[-(frame_list_limit / 2)][:filename]).to eq('app.rb') + expect(frames[-(frame_list_limit / 2)][:function]).to eq('/') + + # First N lines kept + # N = Frame limit / 2 + expect(frames[0][:lineno]).to eq(1) + expect(frames[0][:filename]).to eq('app.rb') + expect(frames[0][:function]).to eq('/') + expect(frames[(frame_list_limit / 2) - 1][:lineno]).to eq(frame_list_limit / 2) + expect(frames[(frame_list_limit / 2) - 1][:filename]).to eq('app.rb') + expect(frames[(frame_list_limit / 2) - 1][:function]).to eq('/') end context "if it's still oversized" do From f897b45a2e0d11e16e44b6a78d1fb94285cdd100 Mon Sep 17 00:00:00 2001 From: PikachuEXE Date: Wed, 4 May 2022 09:53:00 +0800 Subject: [PATCH 3/3] Update CHANGELOG --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 614f35abd..e6a32d733 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## Unreleased + +### Features + +- Handle exception with large stacktrace without dropping entire item [#1807](https://github.com/getsentry/sentry-ruby/pull/1807) + ## 5.3.1 ### Bug Fixes