-
-
Notifications
You must be signed in to change notification settings - Fork 506
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Improve SDK's error handling (#1298)
* Add sentry/exceptions.rb * 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. * Transport#send_event shouldn't swallow any error * Move event sending specs to their own file * Implement error handling on Client#capture_event * Fix background worker's logger setup * 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`. * Refactor Client#capture_event * Log event message when event sending failed * Capitalize all event sending messages * Change logging message base on event type * Fix Event.get_log_message * Update changelog
- sentry-sidekiq-v4.4.0
- sentry-sidekiq-v4.4.0-beta.0
- sentry-sidekiq-v4.3.0
- sentry-ruby-v4.4.2
- sentry-ruby-v4.4.1
- sentry-ruby-v4.4.0
- sentry-ruby-v4.4.0-beta.0
- sentry-ruby-v4.3.1
- sentry-ruby-v4.3.0
- sentry-raven-v3.1.2
- sentry-rails-v4.4.0
- sentry-rails-v4.4.0-beta.0
- sentry-rails-v4.3.4
- sentry-rails-v4.3.3
- sentry-rails-v4.3.3-beta.0
- sentry-rails-v4.3.2
- sentry-rails-v4.3.1
- sentry-rails-v4.3.0
- sentry-delayed_job-v4.4.0
- sentry-delayed_job-v4.4.0-beta.0
- sentry-delayed_job-v4.3.1
- sentry-delayed_job-v4.3.0
- 5.22.4
- 5.22.3
- 5.22.2
- 5.22.1
- 5.22.0
- 5.21.0
- 5.20.1
- 5.20.0
- 5.19.0
- 5.18.2
- 5.18.1
- 5.18.0
- 5.17.3
- 5.17.2
- 5.17.1
- 5.17.0
- 5.16.1
- 5.16.0
- 5.15.2
- 5.15.1
- 5.15.0
- 5.14.0
- 5.13.0
- 5.12.0
- 5.11.0
- 5.10.0
- 5.9.0
- 5.8.0
- 5.7.0
- 5.6.0
- 5.5.0
- 5.4.2
- 5.4.1
- 5.4.0
- 5.3.1
- 5.3.0
- 5.2.1
- 5.2.0
- 5.1.1
- 5.1.0
- 5.0.2
- 5.0.1
- 5.0.0
- 4.9.2
- 4.9.1
- 4.9.0
- 4.8.3
- 4.8.2
- 4.8.1
- 4.8.0
- 4.7.3
- 4.7.2
- 4.7.1
- 4.7.0
- 4.6.5
- 4.6.4
- 4.6.3
- 4.6.2
- 4.6.1
- 4.6.0
- 4.6.0-beta.0
- 4.5.2
- 4.5.1
- 4.5.0
- 4.5.0-beta.1
Showing
13 changed files
with
479 additions
and
262 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
module Sentry | ||
class Error < StandardError | ||
end | ||
|
||
class ExternalError < StandardError | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,331 @@ | ||
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 | ||
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 | ||
|
||
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::ExternalError.new("networking error")) | ||
end | ||
|
||
it "raises the error" do | ||
expect do | ||
subject.send_event(event) | ||
end.to raise_error(Sentry::ExternalError, "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 | ||
|
||
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.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 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 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(/Unreported Event: Test message/) | ||
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 | ||
configuration.before_send = -> (_, _) { raise TypeError } | ||
|
||
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 | ||
|
||
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/) | ||
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 | ||
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/) | ||
expect(string_io.string).to match(/Unreported Event: Test message/) | ||
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 | ||
end | ||
|
||
configuration.async = -> (_, _) { raise Redis::ConnectionError } | ||
|
||
subject.capture_event(event, scope) | ||
|
||
expect(string_io.string).to match(/Async event sending failed: Redis::ConnectionError/) | ||
end | ||
|
||
it "swallows and logs the exception" do | ||
configuration.async = -> (_, _) { raise TypeError } | ||
|
||
subject.capture_event(event, scope) | ||
|
||
expect(string_io.string).to match(/Async event sending failed: 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) | ||
|
||
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 | ||
|
||
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) | ||
|
||
expect(string_io.string).to match(/Event sending failed: TypeError/) | ||
expect(string_io.string).to match(/Unreported Event: Test message/) | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters