From c7c8feeaa923877a6b24888e915207eb62c520ec Mon Sep 17 00:00:00 2001 From: st0012 Date: Thu, 25 Feb 2021 18:35:41 +0800 Subject: [PATCH 01/13] Add sentry/exceptions.rb --- sentry-ruby/lib/sentry-ruby.rb | 4 +--- sentry-ruby/lib/sentry/exceptions.rb | 4 ++++ 2 files changed, 5 insertions(+), 3 deletions(-) create mode 100644 sentry-ruby/lib/sentry/exceptions.rb diff --git a/sentry-ruby/lib/sentry-ruby.rb b/sentry-ruby/lib/sentry-ruby.rb index d3d223ebe..2c1382517 100644 --- a/sentry-ruby/lib/sentry-ruby.rb +++ b/sentry-ruby/lib/sentry-ruby.rb @@ -3,6 +3,7 @@ require "time" require "sentry/version" +require "sentry/exceptions" require "sentry/core_ext/object/deep_dup" require "sentry/utils/argument_checking_helper" require "sentry/configuration" @@ -26,9 +27,6 @@ end module Sentry - class Error < StandardError - end - META = { "name" => "sentry.ruby", "version" => Sentry::VERSION }.freeze LOGGER_PROGNAME = "sentry".freeze diff --git a/sentry-ruby/lib/sentry/exceptions.rb b/sentry-ruby/lib/sentry/exceptions.rb new file mode 100644 index 000000000..aa0f9da4f --- /dev/null +++ b/sentry-ruby/lib/sentry/exceptions.rb @@ -0,0 +1,4 @@ +module Sentry + class Error < StandardError + end +end From 35c352a025a2b006a8235d7c967ba90c038a51d3 Mon Sep 17 00:00:00 2001 From: st0012 Date: Thu, 25 Feb 2021 21:09:02 +0800 Subject: [PATCH 02/13] Add Sentry::ExternalError Sentry::Error represents errors SDK raises intentionally. Sentry::ExternalError represents errors by the SDK, but casused by external reasons, like networking issues. --- sentry-ruby/lib/sentry/exceptions.rb | 3 +++ sentry-ruby/lib/sentry/transport/http_transport.rb | 2 +- sentry-ruby/spec/sentry/transport/http_transport_spec.rb | 6 +++--- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/sentry-ruby/lib/sentry/exceptions.rb b/sentry-ruby/lib/sentry/exceptions.rb index aa0f9da4f..a2e06e78e 100644 --- a/sentry-ruby/lib/sentry/exceptions.rb +++ b/sentry-ruby/lib/sentry/exceptions.rb @@ -1,4 +1,7 @@ module Sentry class Error < StandardError end + + class ExternalError < StandardError + end end diff --git a/sentry-ruby/lib/sentry/transport/http_transport.rb b/sentry-ruby/lib/sentry/transport/http_transport.rb index 7f5ed74f6..5ae0b2695 100644 --- a/sentry-ruby/lib/sentry/transport/http_transport.rb +++ b/sentry-ruby/lib/sentry/transport/http_transport.rb @@ -38,7 +38,7 @@ def send_data(data) error_info += " Error in headers is: #{e.response[:headers]['x-sentry-error']}" if e.response[:headers]['x-sentry-error'] end - raise Sentry::Error, error_info + raise Sentry::ExternalError, error_info end private diff --git a/sentry-ruby/spec/sentry/transport/http_transport_spec.rb b/sentry-ruby/spec/sentry/transport/http_transport_spec.rb index d46940f42..7d1012062 100644 --- a/sentry-ruby/spec/sentry/transport/http_transport_spec.rb +++ b/sentry-ruby/spec/sentry/transport/http_transport_spec.rb @@ -95,7 +95,7 @@ end it 'raises an error' do - expect { subject.send_data(data) }.to raise_error(Sentry::Error, /the server responded with status 404/) + expect { subject.send_data(data) }.to raise_error(Sentry::ExternalError, /the server responded with status 404/) stubs.verify_stubbed_calls end @@ -109,7 +109,7 @@ end it 'raises an error' do - expect { subject.send_data(data) }.to raise_error(Sentry::Error, /the server responded with status 500/) + expect { subject.send_data(data) }.to raise_error(Sentry::ExternalError, /the server responded with status 500/) stubs.verify_stubbed_calls end @@ -123,7 +123,7 @@ end it 'raises an error with header' do - expect { subject.send_data(data) }.to raise_error(Sentry::Error, /error_in_header/) + expect { subject.send_data(data) }.to raise_error(Sentry::ExternalError, /error_in_header/) stubs.verify_stubbed_calls end From 0bd5e5f678973f7c84735eba6fb3425e7ce4ae02 Mon Sep 17 00:00:00 2001 From: st0012 Date: Thu, 25 Feb 2021 21:11:03 +0800 Subject: [PATCH 03/13] Transport#send_event shouldn't swallow any error --- sentry-ruby/lib/sentry/transport.rb | 3 -- sentry-ruby/spec/sentry/transport_spec.rb | 36 +++++++++++------------ 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/sentry-ruby/lib/sentry/transport.rb b/sentry-ruby/lib/sentry/transport.rb index 345383998..31f3153db 100644 --- a/sentry-ruby/lib/sentry/transport.rb +++ b/sentry-ruby/lib/sentry/transport.rb @@ -31,9 +31,6 @@ def send_event(event) send_data(encoded_data) event - rescue => e - failed_for_exception(e, event) - nil end def generate_auth_header diff --git a/sentry-ruby/spec/sentry/transport_spec.rb b/sentry-ruby/spec/sentry/transport_spec.rb index a5433b56c..3fd890b14 100644 --- a/sentry-ruby/spec/sentry/transport_spec.rb +++ b/sentry-ruby/spec/sentry/transport_spec.rb @@ -11,10 +11,10 @@ end let(:fake_time) { Time.now } - subject { described_class.new(configuration) } + let(:client) { Sentry::Client.new(configuration) } + subject { client.transport } describe "#encode" do - let(:client) { Sentry::Client.new(configuration) } before do Sentry.init do |config| @@ -121,24 +121,24 @@ end context "when failed" do - before do - allow(subject).to receive(:send_data).and_raise(StandardError) + context "with normal error" do + before do + allow(subject).to receive(:send_data).and_raise(StandardError) + end + + it "raises the error" do + expect do + subject.send_event(event) + end.to raise_error(StandardError) + end end - it "returns nil" do - expect(subject.send_event(event)).to eq(nil) - end - - it "logs correct message" do - subject.send_event(event) - - log = io.string - expect(log).to match( - /WARN -- sentry: Unable to record event with remote Sentry server \(StandardError - StandardError\):/ - ) - expect(log).to match( - /WARN -- sentry: Failed to submit event. Unreported Event: ZeroDivisionError: divided by 0/ - ) + context "with Faraday::Error" do + it "raises the error" do + expect do + subject.send_event(event) + end.to raise_error(Sentry::ExternalError) + end end end end From cae5bedc6e093c52a6fc05eaa0d2b75eb7e3fcc2 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 17 Feb 2021 12:10:41 +0800 Subject: [PATCH 04/13] Move event sending specs to their own file --- .../spec/sentry/client/event_sending_spec.rb | 217 ++++++++++++++++++ sentry-ruby/spec/sentry/client_spec.rb | 188 --------------- 2 files changed, 217 insertions(+), 188 deletions(-) create mode 100644 sentry-ruby/spec/sentry/client/event_sending_spec.rb diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb new file mode 100644 index 000000000..7b7316423 --- /dev/null +++ b/sentry-ruby/spec/sentry/client/event_sending_spec.rb @@ -0,0 +1,217 @@ +require 'spec_helper' + +RSpec.describe Sentry::Client do + let(:configuration) do + Sentry::Configuration.new.tap do |config| + config.dsn = DUMMY_DSN + config.transport.transport_class = Sentry::DummyTransport + end + end + subject { Sentry::Client.new(configuration) } + + describe "#capture_event" do + let(:message) { "Test message" } + let(:scope) { Sentry::Scope.new } + let(:event) { subject.event_from_message(message) } + + context 'with config.async set' do + let(:async_block) do + lambda do |event| + subject.send_event(event) + end + end + + around do |example| + prior_async = configuration.async + configuration.async = async_block + example.run + configuration.async = prior_async + end + + it "executes the given block" do + expect(async_block).to receive(:call).and_call_original + + returned = subject.capture_event(event, scope) + + expect(returned).to be_a(Sentry::Event) + expect(subject.transport.events.first).to eq(event.to_json_compatible) + end + + it "doesn't call the async block if not allow sending events" do + allow(configuration).to receive(:sending_allowed?).and_return(false) + + expect(async_block).not_to receive(:call) + + returned = subject.capture_event(event, scope) + + expect(returned).to eq(nil) + end + + context "with false as value (the legacy way to disable it)" do + let(:async_block) { false } + + it "doesn't cause any issue" do + returned = subject.capture_event(event, scope, { background: false }) + + expect(returned).to be_a(Sentry::Event) + expect(subject.transport.events.first).to eq(event) + end + end + + context "with 2 arity block" do + let(:async_block) do + lambda do |event, hint| + event["tags"]["hint"] = hint + subject.send_event(event) + end + end + + it "serializes hint and supplies it as the second argument" do + expect(configuration.async).to receive(:call).and_call_original + + returned = subject.capture_event(event, scope, { foo: "bar" }) + + expect(returned).to be_a(Sentry::Event) + event = subject.transport.events.first + expect(event.dig("tags", "hint")).to eq({ "foo" => "bar" }) + end + end + + context "when async raises an exception" do + around do |example| + prior_async = configuration.async + configuration.async = proc { raise TypeError } + example.run + configuration.async = prior_async + end + + it 'sends the result of Event.capture_exception via fallback' do + expect(configuration.logger).to receive(:error).with(Sentry::LOGGER_PROGNAME) { "async event sending failed: TypeError" } + expect(configuration.async).to receive(:call).and_call_original + expect(subject).to receive(:send_event) + + subject.capture_event(event, scope) + end + end + end + + context "with background_worker enabled (default)" do + before do + Sentry.background_worker = Sentry::BackgroundWorker.new(configuration) + configuration.before_send = lambda do |event, _hint| + sleep 0.1 + event + end + end + + let(:transport) do + subject.transport + end + + it "sends events asynchronously" do + subject.capture_event(event, scope) + + expect(transport.events.count).to eq(0) + + sleep(0.2) + + expect(transport.events.count).to eq(1) + end + + context "with hint: { background: false }" do + it "sends the event immediately" do + subject.capture_event(event, scope, { background: false }) + + expect(transport.events.count).to eq(1) + end + + context "when there's a Sentry::Error" do + before do + expect(subject.transport).to receive(:send_event).and_raise(Sentry::Error.new("networking error")) + end + + it "swallows the error" do + expect(subject.capture_event(event, scope, { background: false })).to be_nil + end + end + end + end + end + + describe "#send_event" do + let(:event_object) do + subject.event_from_exception(ZeroDivisionError.new("divided by 0")) + end + let(:transaction_event_object) do + subject.event_from_transaction(Sentry::Transaction.new) + end + + shared_examples "Event in send_event" do + context "when there's an exception" do + before do + expect(subject.transport).to receive(:send_event).and_raise(Sentry::Error.new("networking error")) + end + + it "raises the error" do + expect do + subject.send_event(event) + end.to raise_error(Sentry::Error, "networking error") + end + end + it "sends data through the transport" do + expect(subject.transport).to receive(:send_event).with(event) + subject.send_event(event) + end + + it "applies before_send callback before sending the event" do + configuration.before_send = lambda do |event, _hint| + if event.is_a?(Sentry::Event) + event.tags[:called] = true + else + event["tags"]["called"] = true + end + + event + end + + subject.send_event(event) + + if event.is_a?(Sentry::Event) + expect(event.tags[:called]).to eq(true) + else + expect(event["tags"]["called"]).to eq(true) + end + end + end + + it_behaves_like "Event in send_event" do + let(:event) { event_object } + end + + it_behaves_like "Event in send_event" do + let(:event) { event_object.to_json_compatible } + end + + shared_examples "TransactionEvent in send_event" do + it "sends data through the transport" do + subject.send_event(event) + end + + it "doesn't apply before_send to TransactionEvent" do + configuration.before_send = lambda do |event, _hint| + raise "shouldn't trigger me" + end + + subject.send_event(event) + end + end + + it_behaves_like "TransactionEvent in send_event" do + let(:event) { transaction_event_object } + end + + it_behaves_like "TransactionEvent in send_event" do + let(:event) { transaction_event_object.to_json_compatible } + end + end +end diff --git a/sentry-ruby/spec/sentry/client_spec.rb b/sentry-ruby/spec/sentry/client_spec.rb index fefcb2aaf..36b43831f 100644 --- a/sentry-ruby/spec/sentry/client_spec.rb +++ b/sentry-ruby/spec/sentry/client_spec.rb @@ -23,194 +23,6 @@ def sentry_context allow(Time).to receive(:now).and_return fake_time end - describe "#capture_event" do - let(:message) { "Test message" } - let(:scope) { Sentry::Scope.new } - let(:event) { subject.event_from_message(message) } - - context 'with config.async set' do - let(:async_block) do - lambda do |event| - subject.send_event(event) - end - end - - around do |example| - prior_async = configuration.async - configuration.async = async_block - example.run - configuration.async = prior_async - end - - it "executes the given block" do - expect(async_block).to receive(:call).and_call_original - - returned = subject.capture_event(event, scope) - - expect(returned).to be_a(Sentry::Event) - expect(subject.transport.events.first).to eq(event.to_json_compatible) - end - - it "doesn't call the async block if not allow sending events" do - allow(configuration).to receive(:sending_allowed?).and_return(false) - - expect(async_block).not_to receive(:call) - - returned = subject.capture_event(event, scope) - - expect(returned).to eq(nil) - end - - context "with false as value (the legacy way to disable it)" do - let(:async_block) { false } - - it "doesn't cause any issue" do - returned = subject.capture_event(event, scope, { background: false }) - - expect(returned).to be_a(Sentry::Event) - expect(subject.transport.events.first).to eq(event) - end - end - - context "with 2 arity block" do - let(:async_block) do - lambda do |event, hint| - event["tags"]["hint"] = hint - subject.send_event(event) - end - end - - it "serializes hint and supplies it as the second argument" do - expect(configuration.async).to receive(:call).and_call_original - - returned = subject.capture_event(event, scope, { foo: "bar" }) - - expect(returned).to be_a(Sentry::Event) - event = subject.transport.events.first - expect(event.dig("tags", "hint")).to eq({ "foo" => "bar" }) - end - end - - context "when async raises an exception" do - around do |example| - prior_async = configuration.async - configuration.async = proc { raise TypeError } - example.run - configuration.async = prior_async - end - - it 'sends the result of Event.capture_exception via fallback' do - expect(configuration.logger).to receive(:error).with(Sentry::LOGGER_PROGNAME) { "async event sending failed: TypeError" } - expect(configuration.async).to receive(:call).and_call_original - expect(subject).to receive(:send_event) - - subject.capture_event(event, scope) - end - end - end - - context "with background_worker enabled (default)" do - before do - Sentry.background_worker = Sentry::BackgroundWorker.new(configuration) - configuration.before_send = lambda do |event, _hint| - sleep 0.1 - event - end - end - - let(:transport) do - subject.transport - end - - it "sends events asynchronously" do - subject.capture_event(event, scope) - - expect(transport.events.count).to eq(0) - - sleep(0.2) - - expect(transport.events.count).to eq(1) - end - - context "with hint: { background: false }" do - it "sends the event immediately" do - subject.capture_event(event, scope, { background: false }) - - expect(transport.events.count).to eq(1) - end - end - end - end - - describe "#send_event" do - let(:event_object) do - subject.event_from_exception(ZeroDivisionError.new("divided by 0")) - end - let(:transaction_event_object) do - subject.event_from_transaction(Sentry::Transaction.new) - end - - before do - expect(subject.transport).to receive(:send_event).with(event) - end - - shared_examples "Event in send_event" do - it "sends data through the transport" do - subject.send_event(event) - end - - it "applies before_send callback before sending the event" do - configuration.before_send = lambda do |event, _hint| - if event.is_a?(Sentry::Event) - event.tags[:called] = true - else - event["tags"]["called"] = true - end - - event - end - - subject.send_event(event) - - if event.is_a?(Sentry::Event) - expect(event.tags[:called]).to eq(true) - else - expect(event["tags"]["called"]).to eq(true) - end - end - end - - it_behaves_like "Event in send_event" do - let(:event) { event_object } - end - - it_behaves_like "Event in send_event" do - let(:event) { event_object.to_json_compatible } - end - - shared_examples "TransactionEvent in send_event" do - it "sends data through the transport" do - subject.send_event(event) - end - - it "doesn't apply before_send to TransactionEvent" do - configuration.before_send = lambda do |event, _hint| - raise "shouldn't trigger me" - end - - subject.send_event(event) - end - end - - it_behaves_like "TransactionEvent in send_event" do - let(:event) { transaction_event_object } - end - - it_behaves_like "TransactionEvent in send_event" do - let(:event) { transaction_event_object.to_json_compatible } - end - end - describe "#transport" do let(:configuration) { Sentry::Configuration.new } From 5a99959eec7fe0c83cc21633339993572b1324a1 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 17 Feb 2021 12:34:45 +0800 Subject: [PATCH 05/13] Implement error handling on Client#capture_event --- sentry-ruby/lib/sentry/client.rb | 11 +- .../spec/sentry/client/event_sending_spec.rb | 123 +++++++++++++----- 2 files changed, 103 insertions(+), 31 deletions(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 5df2a8ba9..c23f9f3aa 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -38,8 +38,12 @@ def capture_event(event, scope, hint = {}) async_block.call(event_hash) end rescue => e - configuration.logger.error(LOGGER_PROGNAME) { "async event sending failed: #{e.message}" } - send_event(event, hint) + if e.class.name.match?(/Redis/) + configuration.logger.error(LOGGER_PROGNAME) { "async event sending failed: #{e.message}" } + send_event(event, hint) + else + raise + end end else if hint.fetch(:background, true) @@ -52,6 +56,9 @@ def capture_event(event, scope, hint = {}) end event + rescue Sentry::ExternalError => e + configuration.logger.error(LOGGER_PROGNAME) { "event sending failed: #{e.message}" } + nil end def event_from_exception(exception, hint = {}) diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb index 7b7316423..daddf789a 100644 --- a/sentry-ruby/spec/sentry/client/event_sending_spec.rb +++ b/sentry-ruby/spec/sentry/client/event_sending_spec.rb @@ -76,23 +76,6 @@ expect(event.dig("tags", "hint")).to eq({ "foo" => "bar" }) end end - - context "when async raises an exception" do - around do |example| - prior_async = configuration.async - configuration.async = proc { raise TypeError } - example.run - configuration.async = prior_async - end - - it 'sends the result of Event.capture_exception via fallback' do - expect(configuration.logger).to receive(:error).with(Sentry::LOGGER_PROGNAME) { "async event sending failed: TypeError" } - expect(configuration.async).to receive(:call).and_call_original - expect(subject).to receive(:send_event) - - subject.capture_event(event, scope) - end - end end context "with background_worker enabled (default)" do @@ -124,16 +107,6 @@ expect(transport.events.count).to eq(1) end - - context "when there's a Sentry::Error" do - before do - expect(subject.transport).to receive(:send_event).and_raise(Sentry::Error.new("networking error")) - end - - it "swallows the error" do - expect(subject.capture_event(event, scope, { background: false })).to be_nil - end - end end end end @@ -149,13 +122,13 @@ shared_examples "Event in send_event" do context "when there's an exception" do before do - expect(subject.transport).to receive(:send_event).and_raise(Sentry::Error.new("networking error")) + expect(subject.transport).to receive(:send_event).and_raise(Sentry::ExternalError.new("networking error")) end it "raises the error" do expect do subject.send_event(event) - end.to raise_error(Sentry::Error, "networking error") + end.to raise_error(Sentry::ExternalError, "networking error") end end it "sends data through the transport" do @@ -214,4 +187,96 @@ let(:event) { transaction_event_object.to_json_compatible } end end + + describe "integrated error handling testing with HTTPTransport" do + let(:string_io) { StringIO.new } + let(:logger) do + ::Logger.new(string_io) + end + let(:configuration) do + Sentry::Configuration.new.tap do |config| + config.background_worker_threads = 0 + config.dsn = DUMMY_DSN + config.logger = logger + end + end + + let(:message) { "Test message" } + let(:scope) { Sentry::Scope.new } + let(:event) { subject.event_from_message(message) } + + describe "#capture_event" do + around do |example| + prior_async = configuration.async + example.run + configuration.async = prior_async + end + + context "when sending events inline causes error" do + before do + Sentry.background_worker = Sentry::BackgroundWorker.new(configuration) + end + + it "swallows Sentry::ExternalError (caused by transport's networking error)" do + expect(subject.capture_event(event, scope)).to be_nil + + expect(string_io.string).to match(/event sending failed/) + end + + it "doesn't swallow errors caused by the user (like in before_send)" do + configuration.before_send = -> (_, _) { raise TypeError } + + expect do + expect(subject.capture_event(event, scope)).to be_nil + end.to raise_error(TypeError) + end + end + + context "when config.async causes error" do + it "swallows Redis related error and send the event synchronizely" do + class Redis + class ConnectionError < StandardError; end + end + + configuration.async = -> (_, _) { raise Redis::ConnectionError } + + expect(subject).to receive(:send_event) + + subject.capture_event(event, scope) + + expect(string_io.string).to match(/async event sending failed: Redis::ConnectionError/) + end + + it "raises normal exception" do + configuration.async = -> (_, _) { raise TypeError } + + expect do + subject.capture_event(event, scope) + end.to raise_error(TypeError) + end + end + end + + describe "#send_event" do + context "error happens when sending the event" do + it "raises the error" do + expect do + subject.send_event(event) + end.to raise_error(Sentry::ExternalError) + end + end + + context "error happens in the before_send callback" do + it "raises the error" do + configuration.before_send = lambda do |event, _hint| + raise TypeError + end + + expect do + subject.send_event(event) + end.to raise_error(TypeError) + end + end + end + end end From 44892d56304cce20c9ee09fc81cb873f1e6d77dc Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 26 Feb 2021 16:27:12 +0800 Subject: [PATCH 06/13] Fix background worker's logger setup --- sentry-ruby/lib/sentry/background_worker.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/sentry-ruby/lib/sentry/background_worker.rb b/sentry-ruby/lib/sentry/background_worker.rb index d48a9336e..8eb99be6c 100644 --- a/sentry-ruby/lib/sentry/background_worker.rb +++ b/sentry-ruby/lib/sentry/background_worker.rb @@ -1,5 +1,6 @@ require "concurrent/executor/thread_pool_executor" require "concurrent/executor/immediate_executor" +require "concurrent/configuration" module Sentry class BackgroundWorker From 13ed15a73e3228097492116bf9eef94af7b920cc Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 26 Feb 2021 16:29:05 +0800 Subject: [PATCH 07/13] Inline method calls should always swallow errors The code invoked by the SDK itself, whether it's an external service call (like sending events) or a user-defined callback (like async or before_send), shouldn't cause the user's application to crash. Although some may think errors caused by user-defined callbacks should be raised for debugging purpose, it's actually a bad practice in reality. Most of the users only activate the SDK in production and they don't write tests for the callbacks. So if there's any bug in the callbacks, they will usually be spotted in production. Also, because the SDK is usually placed at the farthest layer of the app to catch all the possible exceptions, there won't be anything to capture exceptions triggered by the SDK. So from the SDK's perspective, we can either raise them or swallow them: - If we raise them, the user's customers will see broken apps. Good for spotting bugs, very bad user experience. - If we rescue them, the user's customers won't spot a thing. It may be harder for the user to spot the buggy callback, but at least their customers won't be angry. This is why I decided to swallow any errors happen in the `Client#capture_event`. --- sentry-ruby/lib/sentry/client.rb | 15 ++--- .../spec/sentry/client/event_sending_spec.rb | 65 +++++++++++++++---- 2 files changed, 59 insertions(+), 21 deletions(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index c23f9f3aa..aeab478f2 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -38,12 +38,8 @@ def capture_event(event, scope, hint = {}) async_block.call(event_hash) end rescue => e - if e.class.name.match?(/Redis/) - configuration.logger.error(LOGGER_PROGNAME) { "async event sending failed: #{e.message}" } - send_event(event, hint) - else - raise - end + configuration.logger.error(LOGGER_PROGNAME) { "async event sending failed: #{e.message}" } + send_event(event, hint) end else if hint.fetch(:background, true) @@ -56,8 +52,8 @@ def capture_event(event, scope, hint = {}) end event - rescue Sentry::ExternalError => e - configuration.logger.error(LOGGER_PROGNAME) { "event sending failed: #{e.message}" } + rescue => e + configuration.logger.error(LOGGER_PROGNAME) { "event capturing failed: #{e.message}" } nil end @@ -102,6 +98,9 @@ def send_event(event, hint = nil) transport.send_event(event) event + rescue => e + configuration.logger.error(LOGGER_PROGNAME) { "event sending failed: #{e.message}" } + raise end end end diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb index daddf789a..10b646874 100644 --- a/sentry-ruby/spec/sentry/client/event_sending_spec.rb +++ b/sentry-ruby/spec/sentry/client/event_sending_spec.rb @@ -195,7 +195,6 @@ end let(:configuration) do Sentry::Configuration.new.tap do |config| - config.background_worker_threads = 0 config.dsn = DUMMY_DSN config.logger = logger end @@ -212,27 +211,69 @@ configuration.async = prior_async end + context "when scope.apply_to_event fails" do + before do + scope.add_event_processor do + raise TypeError + end + end + + it "swallows the event and logs the failure" do + expect(subject.capture_event(event, scope)).to be_nil + + expect(string_io.string).to match(/event capturing failed: TypeError/) + end + end + context "when sending events inline causes error" do before do + configuration.background_worker_threads = 0 Sentry.background_worker = Sentry::BackgroundWorker.new(configuration) end - it "swallows Sentry::ExternalError (caused by transport's networking error)" do + it "swallows and logs Sentry::ExternalError (caused by transport's networking error)" do expect(subject.capture_event(event, scope)).to be_nil - expect(string_io.string).to match(/event sending failed/) + 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 - it "doesn't swallow errors caused by the user (like in before_send)" do + it "swallows and logs errors caused by the user (like in before_send)" do configuration.before_send = -> (_, _) { raise TypeError } - expect do - expect(subject.capture_event(event, scope)).to be_nil - end.to raise_error(TypeError) + expect(subject.capture_event(event, scope)).to be_nil + + expect(string_io.string).to match(/event sending failed: TypeError/) + end + end + + context "when sending events in background causes error" do + before do + Sentry.background_worker = Sentry::BackgroundWorker.new(configuration) + end + + it "swallows and logs Sentry::ExternalError (caused by transport's networking error)" do + expect(subject.capture_event(event, scope)).to be_a(Sentry::Event) + sleep(0.1) + + expect(string_io.string).to match(/event sending failed: Failed to open TCP connection/) + end + + it "swallows and logs errors caused by the user (like in before_send)" do + configuration.before_send = -> (_, _) { raise TypeError } + + expect(subject.capture_event(event, scope)).to be_a(Sentry::Event) + sleep(0.1) + + expect(string_io.string).to match(/event sending failed: TypeError/) end end context "when config.async causes error" do + before do + expect(subject).to receive(:send_event) + end + it "swallows Redis related error and send the event synchronizely" do class Redis class ConnectionError < StandardError; end @@ -240,19 +281,17 @@ class ConnectionError < StandardError; end configuration.async = -> (_, _) { raise Redis::ConnectionError } - expect(subject).to receive(:send_event) - subject.capture_event(event, scope) expect(string_io.string).to match(/async event sending failed: Redis::ConnectionError/) end - it "raises normal exception" do + it "swallows and logs the exception" do configuration.async = -> (_, _) { raise TypeError } - expect do - subject.capture_event(event, scope) - end.to raise_error(TypeError) + subject.capture_event(event, scope) + + expect(string_io.string).to match(/async event sending failed: TypeError/) end end end From 1675d405e63dd7a95da0f60cddba46f95e99d81d Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 26 Feb 2021 17:24:37 +0800 Subject: [PATCH 08/13] Refactor Client#capture_event --- sentry-ruby/lib/sentry/client.rb | 60 ++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index aeab478f2..3f1f30b26 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -2,10 +2,11 @@ module Sentry class Client - attr_reader :transport, :configuration + attr_reader :transport, :configuration, :logger def initialize(configuration) @configuration = configuration + @logger = configuration.logger if transport_class = configuration.transport.transport_class @transport = transport_class.new(configuration) @@ -26,34 +27,16 @@ def capture_event(event, scope, hint = {}) scope.apply_to_event(event, hint) if async_block = configuration.async - begin - # We have to convert to a JSON-like hash, because background job - # processors (esp ActiveJob) may not like weird types in the event hash - event_hash = event.to_json_compatible - - if async_block.arity == 2 - hint = JSON.parse(JSON.generate(hint)) - async_block.call(event_hash, hint) - else - async_block.call(event_hash) - end - rescue => e - configuration.logger.error(LOGGER_PROGNAME) { "async event sending failed: #{e.message}" } - send_event(event, hint) - end + dispatch_async_event(async_block, event, hint) + elsif hint.fetch(:background, true) + dispatch_background_event(event, hint) else - if hint.fetch(:background, true) - Sentry.background_worker.perform do - send_event(event, hint) - end - else - send_event(event, hint) - end + send_event(event, hint) end event rescue => e - configuration.logger.error(LOGGER_PROGNAME) { "event capturing failed: #{e.message}" } + logger.error(LOGGER_PROGNAME) { "event capturing failed: #{e.message}" } nil end @@ -91,7 +74,7 @@ def send_event(event, hint = nil) event = configuration.before_send.call(event, hint) if configuration.before_send && event_type == "event" if event.nil? - configuration.logger.info(LOGGER_PROGNAME) { "Discarded event because before_send returned nil" } + logger.info(LOGGER_PROGNAME) { "Discarded event because before_send returned nil" } return end @@ -99,8 +82,33 @@ def send_event(event, hint = nil) event rescue => e - configuration.logger.error(LOGGER_PROGNAME) { "event sending failed: #{e.message}" } + logger.error(LOGGER_PROGNAME) { "event sending failed: #{e.message}" } raise end + + private + + def dispatch_background_event(event, hint) + Sentry.background_worker.perform do + send_event(event, hint) + end + end + + def dispatch_async_event(async_block, event, hint) + # We have to convert to a JSON-like hash, because background job + # processors (esp ActiveJob) may not like weird types in the event hash + event_hash = event.to_json_compatible + + if async_block.arity == 2 + hint = JSON.parse(JSON.generate(hint)) + async_block.call(event_hash, hint) + else + async_block.call(event_hash) + end + rescue => e + logger.error(LOGGER_PROGNAME) { "async event sending failed: #{e.message}" } + send_event(event, hint) + end + end end From 385f8a0650b5919e1dde4a182522f610e380e707 Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 26 Feb 2021 17:56:01 +0800 Subject: [PATCH 09/13] Log event message when event sending failed --- sentry-ruby/lib/sentry/client.rb | 1 + sentry-ruby/lib/sentry/transport.rb | 9 --------- sentry-ruby/spec/sentry/client/event_sending_spec.rb | 10 ++++++++++ 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 3f1f30b26..9cf28e473 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -83,6 +83,7 @@ def send_event(event, hint = nil) event rescue => e logger.error(LOGGER_PROGNAME) { "event sending failed: #{e.message}" } + logger.error(LOGGER_PROGNAME) { "Unreported Event: #{Event.get_log_message(event.to_hash)}" } raise end diff --git a/sentry-ruby/lib/sentry/transport.rb b/sentry-ruby/lib/sentry/transport.rb index 31f3153db..5c2f627df 100644 --- a/sentry-ruby/lib/sentry/transport.rb +++ b/sentry-ruby/lib/sentry/transport.rb @@ -69,15 +69,6 @@ def prepare_encoded_event(event) configuration.logger.info(LOGGER_PROGNAME) { "Sending #{event_type} #{event_id} to Sentry" } encode(event_hash) end - - def failed_for_exception(e, event) - configuration.logger.warn(LOGGER_PROGNAME) { "Unable to record event with remote Sentry server (#{e.class} - #{e.message}):\n#{e.backtrace[0..10].join("\n")}" } - log_not_sending(event) - end - - def log_not_sending(event) - configuration.logger.warn(LOGGER_PROGNAME) { "Failed to submit event. Unreported Event: #{Event.get_log_message(event.to_hash)}" } - end end end diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb index 10b646874..030141da1 100644 --- a/sentry-ruby/spec/sentry/client/event_sending_spec.rb +++ b/sentry-ruby/spec/sentry/client/event_sending_spec.rb @@ -235,6 +235,7 @@ expect(subject.capture_event(event, scope)).to be_nil expect(string_io.string).to match(/event sending failed: Failed to open TCP connection/) + expect(string_io.string).to match(/Unreported Event: Test message/) expect(string_io.string).to match(/event capturing failed: Failed to open TCP connection/) end @@ -244,6 +245,7 @@ expect(subject.capture_event(event, scope)).to be_nil expect(string_io.string).to match(/event sending failed: TypeError/) + expect(string_io.string).to match(/Unreported Event: Test message/) end end @@ -257,6 +259,7 @@ sleep(0.1) expect(string_io.string).to match(/event sending failed: Failed to open TCP connection/) + expect(string_io.string).to match(/Unreported Event: Test message/) end it "swallows and logs errors caused by the user (like in before_send)" do @@ -266,6 +269,7 @@ sleep(0.1) expect(string_io.string).to match(/event sending failed: TypeError/) + expect(string_io.string).to match(/Unreported Event: Test message/) end end @@ -302,6 +306,9 @@ class ConnectionError < StandardError; end expect do subject.send_event(event) end.to raise_error(Sentry::ExternalError) + + expect(string_io.string).to match(/event sending failed: Failed to open TCP connection/) + expect(string_io.string).to match(/Unreported Event: Test message/) end end @@ -314,6 +321,9 @@ class ConnectionError < StandardError; end expect do subject.send_event(event) end.to raise_error(TypeError) + + expect(string_io.string).to match(/event sending failed: TypeError/) + expect(string_io.string).to match(/Unreported Event: Test message/) end end end From 8e7535959e8c6e7f72a339184c8a9b9cdb7d6916 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sat, 27 Feb 2021 18:10:47 +0800 Subject: [PATCH 10/13] Capitalize all event sending messages --- sentry-ruby/lib/sentry/client.rb | 6 +++--- .../spec/sentry/client/event_sending_spec.rb | 20 +++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 9cf28e473..7d3f62b68 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -36,7 +36,7 @@ def capture_event(event, scope, hint = {}) event rescue => e - logger.error(LOGGER_PROGNAME) { "event capturing failed: #{e.message}" } + logger.error(LOGGER_PROGNAME) { "Event capturing failed: #{e.message}" } nil end @@ -82,7 +82,7 @@ def send_event(event, hint = nil) event rescue => e - logger.error(LOGGER_PROGNAME) { "event sending failed: #{e.message}" } + logger.error(LOGGER_PROGNAME) { "Event sending failed: #{e.message}" } logger.error(LOGGER_PROGNAME) { "Unreported Event: #{Event.get_log_message(event.to_hash)}" } raise end @@ -107,7 +107,7 @@ def dispatch_async_event(async_block, event, hint) async_block.call(event_hash) end rescue => e - logger.error(LOGGER_PROGNAME) { "async event sending failed: #{e.message}" } + logger.error(LOGGER_PROGNAME) { "Async event sending failed: #{e.message}" } send_event(event, hint) end diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb index 030141da1..eacbf9c7e 100644 --- a/sentry-ruby/spec/sentry/client/event_sending_spec.rb +++ b/sentry-ruby/spec/sentry/client/event_sending_spec.rb @@ -221,7 +221,7 @@ it "swallows the event and logs the failure" do expect(subject.capture_event(event, scope)).to be_nil - expect(string_io.string).to match(/event capturing failed: TypeError/) + expect(string_io.string).to match(/Event capturing failed: TypeError/) end end @@ -234,9 +234,9 @@ it "swallows and logs Sentry::ExternalError (caused by transport's networking error)" do expect(subject.capture_event(event, scope)).to be_nil - expect(string_io.string).to match(/event sending failed: Failed to open TCP connection/) + expect(string_io.string).to match(/Event sending failed: Failed to open TCP connection/) expect(string_io.string).to match(/Unreported Event: Test message/) - expect(string_io.string).to match(/event capturing failed: Failed to open TCP connection/) + expect(string_io.string).to match(/Event capturing failed: Failed to open TCP connection/) end it "swallows and logs errors caused by the user (like in before_send)" do @@ -244,7 +244,7 @@ expect(subject.capture_event(event, scope)).to be_nil - expect(string_io.string).to match(/event sending failed: TypeError/) + expect(string_io.string).to match(/Event sending failed: TypeError/) expect(string_io.string).to match(/Unreported Event: Test message/) end end @@ -258,7 +258,7 @@ expect(subject.capture_event(event, scope)).to be_a(Sentry::Event) sleep(0.1) - expect(string_io.string).to match(/event sending failed: Failed to open TCP connection/) + expect(string_io.string).to match(/Event sending failed: Failed to open TCP connection/) expect(string_io.string).to match(/Unreported Event: Test message/) end @@ -268,7 +268,7 @@ expect(subject.capture_event(event, scope)).to be_a(Sentry::Event) sleep(0.1) - expect(string_io.string).to match(/event sending failed: TypeError/) + expect(string_io.string).to match(/Event sending failed: TypeError/) expect(string_io.string).to match(/Unreported Event: Test message/) end end @@ -287,7 +287,7 @@ class ConnectionError < StandardError; end subject.capture_event(event, scope) - expect(string_io.string).to match(/async event sending failed: Redis::ConnectionError/) + expect(string_io.string).to match(/Async event sending failed: Redis::ConnectionError/) end it "swallows and logs the exception" do @@ -295,7 +295,7 @@ class ConnectionError < StandardError; end subject.capture_event(event, scope) - expect(string_io.string).to match(/async event sending failed: TypeError/) + expect(string_io.string).to match(/Async event sending failed: TypeError/) end end end @@ -307,7 +307,7 @@ class ConnectionError < StandardError; end subject.send_event(event) end.to raise_error(Sentry::ExternalError) - expect(string_io.string).to match(/event sending failed: Failed to open TCP connection/) + expect(string_io.string).to match(/Event sending failed: Failed to open TCP connection/) expect(string_io.string).to match(/Unreported Event: Test message/) end end @@ -322,7 +322,7 @@ class ConnectionError < StandardError; end subject.send_event(event) end.to raise_error(TypeError) - expect(string_io.string).to match(/event sending failed: TypeError/) + expect(string_io.string).to match(/Event sending failed: TypeError/) expect(string_io.string).to match(/Unreported Event: Test message/) end end From 09c450849c7f5c2d16c70c7c32ab90276dced4a3 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sat, 27 Feb 2021 18:19:31 +0800 Subject: [PATCH 11/13] Change logging message base on event type --- sentry-ruby/lib/sentry/client.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 7d3f62b68..4c5ead7a5 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -71,19 +71,22 @@ def event_from_transaction(transaction) def send_event(event, hint = nil) event_type = event.is_a?(Event) ? event.type : event["type"] - event = configuration.before_send.call(event, hint) if configuration.before_send && event_type == "event" - if event.nil? - logger.info(LOGGER_PROGNAME) { "Discarded event because before_send returned nil" } - return + if event_type == "event" && configuration.before_send + event = configuration.before_send.call(event, hint) + + if event.nil? + logger.info(LOGGER_PROGNAME) { "Discarded event because before_send returned nil" } + return + end end transport.send_event(event) event rescue => e - logger.error(LOGGER_PROGNAME) { "Event sending failed: #{e.message}" } - logger.error(LOGGER_PROGNAME) { "Unreported Event: #{Event.get_log_message(event.to_hash)}" } + logger.error(LOGGER_PROGNAME) { "#{event_type.capitalize} sending failed: #{e.message}" } + logger.error(LOGGER_PROGNAME) { "Unreported #{event_type.capitalize}: #{Event.get_log_message(event.to_hash)}" } raise end @@ -107,7 +110,8 @@ def dispatch_async_event(async_block, event, hint) async_block.call(event_hash) end rescue => e - logger.error(LOGGER_PROGNAME) { "Async event sending failed: #{e.message}" } + event_type = event_hash["type"] + logger.error(LOGGER_PROGNAME) { "Async #{event_type} sending failed: #{e.message}" } send_event(event, hint) end From 8e7e7b0fe4fa35d588601ca8078d9bd2b17656ef Mon Sep 17 00:00:00 2001 From: st0012 Date: Sat, 27 Feb 2021 18:46:58 +0800 Subject: [PATCH 12/13] Fix Event.get_log_message --- sentry-ruby/lib/sentry/event.rb | 27 ++++++++------ sentry-ruby/spec/sentry/event_spec.rb | 51 +++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/sentry-ruby/lib/sentry/event.rb b/sentry-ruby/lib/sentry/event.rb index 20cf6a4a3..9838cc2ee 100644 --- a/sentry-ruby/lib/sentry/event.rb +++ b/sentry-ruby/lib/sentry/event.rb @@ -52,19 +52,26 @@ def initialize(configuration:, integration_meta: nil, message: nil) class << self def get_log_message(event_hash) message = event_hash[:message] || event_hash['message'] - message = get_message_from_exception(event_hash) if message.nil? || message.empty? - message = '' if message.nil? || message.empty? - message + + return message unless message.nil? || message.empty? + + message = get_message_from_exception(event_hash) + + return message unless message.nil? || message.empty? + + message = event_hash[:transaction] || event_hash["transaction"] + + return message unless message.nil? || message.empty? + + '' end def get_message_from_exception(event_hash) - ( - event_hash && - event_hash[:exception] && - event_hash[:exception][:values] && - event_hash[:exception][:values][0] && - "#{event_hash[:exception][:values][0][:type]}: #{event_hash[:exception][:values][0][:value]}" - ) + if exception = event_hash.dig(:exception, :values, 0) + "#{exception[:type]}: #{exception[:value]}" + elsif exception = event_hash.dig("exception", "values", 0) + "#{exception["type"]}: #{exception["value"]}" + end end end diff --git a/sentry-ruby/spec/sentry/event_spec.rb b/sentry-ruby/spec/sentry/event_spec.rb index 91736d250..effd80157 100644 --- a/sentry-ruby/spec/sentry/event_spec.rb +++ b/sentry-ruby/spec/sentry/event_spec.rb @@ -154,4 +154,55 @@ expect(json["extra"]['anonymous_module']).not_to be_a(Class) end end + + describe ".get_log_message" do + let(:client) do + Sentry::Client.new(configuration) + end + + context "with adnormal event" do + subject do + Sentry::Event.new(configuration: configuration) + end + + it "returns placeholder message" do + expect(described_class.get_log_message(subject.to_hash)).to eq('') + expect(described_class.get_log_message(subject.to_json_compatible)).to eq('') + end + end + context "with transaction event" do + let(:transaction) do + Sentry::Transaction.new(name: "test transaction", op: "sql.active_record", sampled: true) + end + + subject do + client.event_from_transaction(transaction) + end + + it "returns the transaction's name" do + expect(described_class.get_log_message(subject.to_hash)).to eq("test transaction") + expect(described_class.get_log_message(subject.to_json_compatible)).to eq("test transaction") + end + end + context "with exception event" do + subject do + client.event_from_exception(Exception.new("error!")) + end + + it "returns the exception's message" do + expect(described_class.get_log_message(subject.to_hash)).to eq("Exception: error!") + expect(described_class.get_log_message(subject.to_json_compatible)).to eq("Exception: error!") + end + end + context "with message event" do + subject do + client.event_from_message("test") + end + + it "returns the event's message" do + expect(described_class.get_log_message(subject.to_hash)).to eq("test") + expect(described_class.get_log_message(subject.to_json_compatible)).to eq("test") + end + end + end end From 0343f1be27088f027c6049e77a3cf5ddfb42637c Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 7 Mar 2021 11:04:38 +0800 Subject: [PATCH 13/13] Update changelog --- sentry-ruby/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sentry-ruby/CHANGELOG.md b/sentry-ruby/CHANGELOG.md index da744be6c..2367f1488 100644 --- a/sentry-ruby/CHANGELOG.md +++ b/sentry-ruby/CHANGELOG.md @@ -3,6 +3,9 @@ ## Unreleased - Refactor interface construction [#1296](https://github.com/getsentry/sentry-ruby/pull/1296) +- Improve SDK's error handling [#1298](https://github.com/getsentry/sentry-ruby/pull/1298) + - Fixes [#1246](https://github.com/getsentry/sentry-ruby/issues/1246) and [#1289](https://github.com/getsentry/sentry-ruby/issues/1289) + - Please read [#1290](https://github.com/getsentry/sentry-ruby/issues/1290) to see the full specification - Treat query string as pii too [#1302](https://github.com/getsentry/sentry-ruby/pull/1302) - Fixes [#1301](https://github.com/getsentry/sentry-ruby/issues/1301) - Ignore sentry-trace when tracing is not enabled [#1308](https://github.com/getsentry/sentry-ruby/pull/1308)