From 06925a328954600adbaa51d6e488c61d0aa85072 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Fri, 26 Jul 2024 15:12:41 +0200 Subject: [PATCH] Do not report the same error more than once Keep all errors in an array, including the first error. Do not add an error to the errors array if it is already present in the array. --- lib/appsignal/transaction.rb | 21 ++++++------ spec/lib/appsignal/transaction_spec.rb | 44 ++++++++++++++------------ 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/lib/appsignal/transaction.rb b/lib/appsignal/transaction.rb index 94c988cff..0619bbaab 100644 --- a/lib/appsignal/transaction.rb +++ b/lib/appsignal/transaction.rb @@ -89,7 +89,7 @@ def clear_current_transaction! # @api private attr_reader :ext, :transaction_id, :action, :namespace, :request, :paused, :tags, :options, - :breadcrumbs, :custom_data, :is_duplicate, :has_error, :errors + :breadcrumbs, :custom_data, :is_duplicate, :errors # Use {.create} to create new transactions. # @@ -110,7 +110,6 @@ def initialize(transaction_id, namespace, ext: nil) @params = nil @session_data = nil @headers = nil - @has_error = false @errors = [] @is_duplicate = false @@ -141,7 +140,9 @@ def complete # is set on finish, will be duplicated from the original transaction. sample_data if !is_duplicate && ext.finish(0) - errors.each do |error| + # Ignore the first error as it is already set in the original + # transaction. + errors.drop(1).each do |error| duplicate.tap do |transaction| transaction.set_error(error) transaction.complete @@ -467,14 +468,14 @@ def add_error(error) return unless error return unless Appsignal.active? - return _set_error(error) unless @has_error + _set_error(error) if @errors.empty? + + return if @errors.include?(error) - # Subtract one from the limit to account for the first error, - # which is not stored in the errors array. - if @errors.length >= (ERRORS_LIMIT - 1) + if @errors.length >= ERRORS_LIMIT Appsignal.internal_logger.warn "Appsignal::Transaction#add_error: Transaction has more " \ - "than #{ERRORS_LIMIT} errors. Only the first " \ - "#{ERRORS_LIMIT} errors will be reported." + "than #{ERRORS_LIMIT} distinct errors. Only the first " \ + "#{ERRORS_LIMIT} distinct errors will be reported." return end @@ -550,8 +551,6 @@ def _set_error(error) backtrace ? Appsignal::Utils::Data.generate(backtrace) : Appsignal::Extension.data_array_new ) - @has_error = true - root_cause_missing = false causes = [] diff --git a/spec/lib/appsignal/transaction_spec.rb b/spec/lib/appsignal/transaction_spec.rb index 55c5609d8..140eee4c8 100644 --- a/spec/lib/appsignal/transaction_spec.rb +++ b/spec/lib/appsignal/transaction_spec.rb @@ -209,7 +209,7 @@ end end - context "when a transaction has additional errors" do + context "when a transaction has errors" do let(:error) do e = ExampleStandardError.new("test message") allow(e).to receive(:backtrace).and_return(["line 1"]) @@ -223,7 +223,7 @@ end context "when an error is already set on the transaction" do - it "reports additional errors as duplicate transactions" do + it "reports errors as duplicate transactions" do transaction.set_error(error) transaction.add_error(other_error) @@ -250,7 +250,7 @@ end context "when no error is set on the transaction" do - it "reports the first additional error in the original transaction" do + it "reports the first error in the original transaction" do transaction.add_error(error) transaction.add_error(other_error) @@ -1323,10 +1323,10 @@ def to_s ) end - it "does not store the error in the additional errors array" do + it "does stores the error in the errors array" do transaction.add_error(error) - expect(transaction.errors).to be_empty + expect(transaction.errors).to eq([error]) end end @@ -1339,10 +1339,10 @@ def to_s before { transaction.set_error(other_error) } - it "stores an error in the additional errors array" do + it "stores an error in the errors array" do transaction.add_error(error) - expect(transaction.errors).to eq([error]) + expect(transaction.errors).to eq([other_error, error]) end it "does not set the error on the extension" do @@ -1356,16 +1356,28 @@ def to_s end end - context "when the additional errors array is at the limit" do + context "when the error has already been added" do + before { transaction.add_error(error) } + + it "does not add the error to the errors array" do + expect(transaction.errors).to eq([error]) + + transaction.add_error(error) + + expect(transaction.errors).to eq([error]) + end + end + + context "when the errors array is at the limit" do before do 10.times do |i| transaction.add_error(ExampleStandardError.new("error #{i}")) end end - it "does not add the error to the additional errors array" do + it "does not add the error to the errors array" do expect(transaction).to have_error("ExampleStandardError", "error 0", []) - expect(transaction.errors.length).to eq(9) + expect(transaction.errors.length).to eq(10) expected_errors = transaction.errors.dup transaction.add_error(error) @@ -1379,8 +1391,8 @@ def to_s expect(logs).to contains_log( :warn, - "Appsignal::Transaction#add_error: Transaction has more than 10 errors. " \ - "Only the first 10 errors will be reported." + "Appsignal::Transaction#add_error: Transaction has more than 10 distinct errors. " \ + "Only the first 10 distinct errors will be reported." ) end end @@ -1401,14 +1413,6 @@ def to_s expect(transaction.errors).to be_empty end - it "sets the has_error attribute to true" do - expect(transaction.has_error).to be_falsy - - transaction.send(:_set_error, error) - - expect(transaction.has_error).to be_truthy - end - context "for a http request" do it "sets an error on the transaction" do allow(error).to receive(:backtrace).and_return(["line 1"])