Skip to content

Commit

Permalink
Alias set_error to add_error method
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@unflxw
unflxw committed Jul 26, 2024
1 parent f0b4dcb commit 17123e2
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 123 deletions.
114 changes: 57 additions & 57 deletions lib/appsignal/transaction.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
135 changes: 72 additions & 63 deletions spec/lib/appsignal/transaction_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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
Expand Down Expand Up @@ -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") }
Expand All @@ -1334,47 +1366,24 @@ 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

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",
Expand All @@ -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
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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."
)
Expand All @@ -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",
Expand Down
Loading

0 comments on commit 17123e2

Please sign in to comment.