From 17123e245359d155e4029739b13719a65151c2a1 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Thu, 25 Jul 2024 12:21:43 +0200 Subject: [PATCH] Alias `set_error` to `add_error` method Instead of having `add_error` always add the errors to the additional errors array, and then "promoting" the last of those errors to an error in the current transaction later, when the transaction is completed, do it when an error is first added using `add_error`. To avoid calls to `set_error` overriding this error instead of adding a new one, make `set_error` into an alias to `add_error`. Move the logic to set an error in the transaction into a private `_set_error` method. --- lib/appsignal/transaction.rb | 114 ++++++++++----------- spec/lib/appsignal/transaction_spec.rb | 135 +++++++++++++------------ spec/lib/appsignal_spec.rb | 28 ++++- 3 files changed, 154 insertions(+), 123 deletions(-) diff --git a/lib/appsignal/transaction.rb b/lib/appsignal/transaction.rb index 877fbace4..615a86b99 100644 --- a/lib/appsignal/transaction.rb +++ b/lib/appsignal/transaction.rb @@ -132,12 +132,6 @@ def complete return end - # If the transaction does not have a set error, take the last of - # the additional errors, if one exists, and set it as the error - # for this transaction. This ensures that we do not report both - # a performance sample and a duplicate error sample. - set_error(errors.pop) if !has_error && !errors.empty? - # If the transaction is a duplicate, we don't want to finish it, # because we want its finish time to be the finish time of the # original transaction, and we do not want to set its sample @@ -462,69 +456,31 @@ def set_metadata(key, value) @ext.set_metadata(key, value) end + # @see Appsignal::Helpers::Instrumentation#add_error def add_error(error) - @errors << error - return unless @errors.length > ADDITIONAL_ERRORS_LIMIT - - Appsignal.internal_logger.debug "Appsignal::Transaction#add_error: Transaction has more " \ - "than #{ADDITIONAL_ERRORS_LIMIT} additional errors. Only the last " \ - "#{ADDITIONAL_ERRORS_LIMIT} will be reported." - @errors = @errors.last(ADDITIONAL_ERRORS_LIMIT) - end - - # @see Appsignal::Helpers::Instrumentation#set_error - def set_error(error) unless error.is_a?(Exception) - Appsignal.internal_logger.error "Appsignal::Transaction#set_error: Cannot set error. " \ + Appsignal.internal_logger.error "Appsignal::Transaction#add_error: Cannot add error. " \ "The given value is not an exception: #{error.inspect}" return end + return unless error return unless Appsignal.active? - backtrace = cleaned_backtrace(error.backtrace) - @ext.set_error( - error.class.name, - cleaned_error_message(error), - backtrace ? Appsignal::Utils::Data.generate(backtrace) : Appsignal::Extension.data_array_new - ) - - @has_error = true - - root_cause_missing = false - - causes = [] - while error - error = error.cause + return _set_error(error) unless @has_error - break unless error - - if causes.length >= ERROR_CAUSES_LIMIT - Appsignal.internal_logger.debug "Appsignal::Transaction#set_error: Error has more " \ - "than #{ERROR_CAUSES_LIMIT} error causes. Only the first #{ERROR_CAUSES_LIMIT} " \ - "will be reported." - root_cause_missing = true - break - end + @errors << error + return unless @errors.length > ADDITIONAL_ERRORS_LIMIT - causes << error - end + Appsignal.internal_logger.debug "Appsignal::Transaction#add_error: Transaction has more " \ + "than #{ADDITIONAL_ERRORS_LIMIT} additional errors. Only the last " \ + "#{ADDITIONAL_ERRORS_LIMIT} will be reported." + @errors = @errors.last(ADDITIONAL_ERRORS_LIMIT) + end - causes_sample_data = causes.map do |e| - { - :name => e.class.name, - :message => cleaned_error_message(e) - } - end + alias :set_error :add_error - causes_sample_data.last[:is_root_cause] = false if root_cause_missing - - set_sample_data( - "error_causes", - causes_sample_data - ) - end - alias_method :add_exception, :set_error + alias_method :add_exception, :add_error # @see Helpers::Instrumentation#instrument # @api private @@ -583,6 +539,50 @@ def to_h private + def _set_error(error) + backtrace = cleaned_backtrace(error.backtrace) + @ext.set_error( + error.class.name, + cleaned_error_message(error), + backtrace ? Appsignal::Utils::Data.generate(backtrace) : Appsignal::Extension.data_array_new + ) + + @has_error = true + + root_cause_missing = false + + causes = [] + while error + error = error.cause + + break unless error + + if causes.length >= ERROR_CAUSES_LIMIT + Appsignal.internal_logger.debug "Appsignal::Transaction#add_error: Error has more " \ + "than #{ERROR_CAUSES_LIMIT} error causes. Only the first #{ERROR_CAUSES_LIMIT} " \ + "will be reported." + root_cause_missing = true + break + end + + causes << error + end + + causes_sample_data = causes.map do |e| + { + :name => e.class.name, + :message => cleaned_error_message(e) + } + end + + causes_sample_data.last[:is_root_cause] = false if root_cause_missing + + set_sample_data( + "error_causes", + causes_sample_data + ) + end + def set_sample_data(key, data) return unless key && data diff --git a/spec/lib/appsignal/transaction_spec.rb b/spec/lib/appsignal/transaction_spec.rb index 703976cc8..dab345146 100644 --- a/spec/lib/appsignal/transaction_spec.rb +++ b/spec/lib/appsignal/transaction_spec.rb @@ -250,7 +250,7 @@ end context "when no error is set on the transaction" do - it "reports the last additional error in the original transaction" do + it "reports the first additional error in the original transaction" do transaction.add_error(error) transaction.add_error(other_error) @@ -262,15 +262,15 @@ expect(original_transaction).to have_error( "ExampleStandardError", - "other test message", - ["line 2"] + "test message", + ["line 1"] ) expect(original_transaction).to be_completed expect(duplicate_transaction).to have_error( "ExampleStandardError", - "test message", - ["line 1"] + "other test message", + ["line 2"] ) expect(duplicate_transaction).to be_completed end @@ -1287,45 +1287,77 @@ def to_s e end - let(:other_error) do - e = ExampleStandardError.new("other test message") - allow(e).to receive(:backtrace).and_return(["line 2"]) - e - end + context "when error argument is not an error" do + let(:error) { Object.new } - it "stores an error in the errors array" do - transaction.add_error(error) + it "does not add the error" do + logs = capture_logs { transaction.add_error(error) } - expect(transaction.errors).to eq([error]) + expect(transaction).to_not have_error + expect(logs).to contains_log( + :error, + "Appsignal::Transaction#add_error: Cannot add error. " \ + "The given value is not an exception: #{error.inspect}" + ) + end end - it "does not set the error on the extension" do - transaction.add_error(error) + context "when AppSignal is not active" do + it "does not add the error" do + allow(Appsignal).to receive(:active?).and_return(false) + + transaction.add_error(error) - expect(transaction.to_h["error"]).to be_nil + expect(transaction).to_not have_error + end end - it "does not set the has_error attribute to true" do - expect(transaction.has_error).to be_falsy + context "when no error is set in the transaction" do + it "sets the error on the transaction" do + transaction.add_error(error) + + expect(transaction).to have_error( + "ExampleStandardError", + "test message", + ["line 1"] + ) + end - transaction.add_error(error) + it "does not store the error in the errors array" do + transaction.add_error(error) - expect(transaction.has_error).to be_falsy + expect(transaction.errors).to be_empty + end end - it "does not override an error set with set_error" do - transaction.set_error(error) - transaction.add_error(other_error) + context "when an error is already set in the transaction" do + let(:other_error) do + e = ExampleStandardError.new("other test message") + allow(e).to receive(:backtrace).and_return(["line 2"]) + e + end - expect(transaction.to_h["error"]).to eq( - "name" => "ExampleStandardError", - "message" => "test message", - "backtrace" => ["line 1"].to_json - ) + before { transaction.set_error(other_error) } + + it "stores an error in the errors array" do + transaction.add_error(error) + + expect(transaction.errors).to eq([error]) + end + + it "does not set the error on the extension" do + transaction.add_error(error) + + expect(transaction).to have_error( + "ExampleStandardError", + "other test message", + ["line 2"] + ) + end end end - describe "#set_error" do + describe "#_set_error" do let(:transaction) { new_transaction } let(:env) { http_request_env_with_data } let(:error) { ExampleStandardError.new("test message") } @@ -1334,16 +1366,8 @@ def to_s expect(transaction).to respond_to(:add_exception) end - it "does not add the error if not active" do - allow(Appsignal).to receive(:active?).and_return(false) - - transaction.set_error(error) - - expect(transaction).to_not have_error - end - it "does not add the error to the errors array" do - transaction.set_error(error) + transaction.send(:_set_error, error) expect(transaction.errors).to be_empty end @@ -1351,30 +1375,15 @@ def to_s it "sets the has_error attribute to true" do expect(transaction.has_error).to be_falsy - transaction.set_error(error) + transaction.send(:_set_error, error) expect(transaction.has_error).to be_truthy end - context "when error argument is not an error" do - let(:error) { Object.new } - - it "does not add the error" do - logs = capture_logs { transaction.set_error(error) } - - expect(transaction).to_not have_error - expect(logs).to contains_log( - :error, - "Appsignal::Transaction#set_error: Cannot set error. " \ - "The given value is not an exception: #{error.inspect}" - ) - end - end - context "for a http request" do it "sets an error on the transaction" do allow(error).to receive(:backtrace).and_return(["line 1"]) - transaction.set_error(error) + transaction.send(:_set_error, error) expect(transaction).to have_error( "ExampleStandardError", @@ -1386,7 +1395,7 @@ def to_s context "when the error has no causes" do it "should set an empty causes array as sample data" do - transaction.set_error(error) + transaction.send(:_set_error, error) expect(transaction).to include_error_causes([]) end @@ -1408,7 +1417,7 @@ def to_s end it "sends the causes information as sample data" do - transaction.set_error(error) + transaction.send(:_set_error, error) expect(transaction).to have_error( "ExampleStandardError", @@ -1430,8 +1439,8 @@ def to_s end it "does not keep error causes from previously set errors" do - transaction.set_error(error) - transaction.set_error(error_without_cause) + transaction.send(:_set_error, error) + transaction.send(:_set_error, error_without_cause) expect(transaction).to have_error( "ExampleStandardError", @@ -1467,7 +1476,7 @@ def to_s end expected_error_causes.last["is_root_cause"] = false - logs = capture_logs { transaction.set_error(error) } + logs = capture_logs { transaction.send(:_set_error, error) } expect(transaction).to have_error( "ExampleStandardError", @@ -1477,7 +1486,7 @@ def to_s expect(transaction).to include_error_causes(expected_error_causes) expect(logs).to contains_log( :debug, - "Appsignal::Transaction#set_error: Error has more " \ + "Appsignal::Transaction#add_error: Error has more " \ "than 10 error causes. Only the first 10 " \ "will be reported." ) @@ -1493,11 +1502,11 @@ def to_s end it "does not raise an error" do - transaction.set_error(error) + transaction.send(:_set_error, error) end it "sets an error on the transaction without an error message" do - transaction.set_error(error) + transaction.send(:_set_error, error) expect(transaction).to have_error( "ExampleStandardError", diff --git a/spec/lib/appsignal_spec.rb b/spec/lib/appsignal_spec.rb index ce2e7f3f9..c8bf4112d 100644 --- a/spec/lib/appsignal_spec.rb +++ b/spec/lib/appsignal_spec.rb @@ -1386,13 +1386,32 @@ def on_start let(:transaction) { http_request_transaction } before { set_current_transaction(transaction) } - it "adds the error to the active transaction" do + it "sets the error in the active transaction" do Appsignal.report_error(error) expect(last_transaction).to eq(transaction) transaction._sample expect(transaction).to have_namespace(Appsignal::Transaction::HTTP_REQUEST) - expect(transaction.errors).to eq([error]) + expect(transaction).to have_error("ExampleException", "error message") + end + + context "when the active transaction already has an error" do + let(:other_error) { ExampleException.new("previous error message") } + + before { transaction.set_error(other_error) } + + it "does not overwrite the existing set error" do + Appsignal.report_error(error) + + transaction._sample + expect(transaction).to have_error("ExampleException", "previous error message") + end + + it "adds the error to the additional errors array" do + Appsignal.report_error(error) + + expect(transaction.errors).to eq([error]) + end end it "does not complete the transaction" do @@ -1412,7 +1431,10 @@ def on_start transaction._sample expect(transaction).to have_namespace("my_namespace") expect(transaction).to have_action("my_action") - expect(transaction.errors).to eq([error]) + expect(transaction).to have_error( + "ExampleException", + "error message" + ) expect(transaction).to include_tags("tag1" => "value1") expect(transaction).to_not be_completed end