Skip to content

Commit

Permalink
Do not report the same error more than once
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
unflxw committed Jul 26, 2024
1 parent 1cd3043 commit 06925a3
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 31 deletions.
21 changes: 10 additions & 11 deletions lib/appsignal/transaction.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
#
Expand All @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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 = []
Expand Down
44 changes: 24 additions & 20 deletions spec/lib/appsignal/transaction_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Expand All @@ -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)

Expand All @@ -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)

Expand Down Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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"])
Expand Down

0 comments on commit 06925a3

Please sign in to comment.