Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduplicate error wrappers and error causes #1366

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .changesets/do-not-report-error-causes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
---
bump: minor
type: change
---

Do not report error causes if the wrapper error has already been reported. This deduplicates errors and prevents the error wrapper and error cause to be reported separately, as long as the error wrapper is reported first.
unflxw marked this conversation as resolved.
Show resolved Hide resolved

```ruby
error_wrapper = nil
error_cause = nil
begin
begin
raise StandardError, "error cause"
rescue => e
error_cause = e
raise Exception, "error wrapper"
end
rescue Exception => e
error_wrapper = e
end

Appsignal.report_error(error_wrapper) # Reports error
Appsignal.report_error(error_cause) # Doesn't report error
```
32 changes: 23 additions & 9 deletions lib/appsignal/transaction.rb
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ def complete
# In the duplicate transaction for each error, set an error
# with a block that calls all the blocks set for that error
# in the original transaction.
transaction.set_error(error) do
transaction.internal_set_error(error) do
blocks.each { |block| block.call(transaction) }
end

Expand Down Expand Up @@ -520,17 +520,18 @@ def add_error(error, &block)
return unless error
return unless Appsignal.active?

_set_error(error) if @error_blocks.empty?

if !@error_blocks.include?(error) && @error_blocks.length >= ERRORS_LIMIT
Appsignal.internal_logger.warn "Appsignal::Transaction#add_error: Transaction has more " \
"than #{ERRORS_LIMIT} distinct errors. Only the first " \
"#{ERRORS_LIMIT} distinct errors will be reported."
if error.instance_variable_get(:@__appsignal_error_reported) && !@error_blocks.include?(error)
unflxw marked this conversation as resolved.
Show resolved Hide resolved
return
end

@error_blocks[error] << block
@error_blocks[error].compact!
internal_set_error(error, &block)

# Mark errors and their causes as tracked so we don't report duplicates,
# but also not error causes if the wrapper error is already reported.
while error
error.instance_variable_set(:@__appsignal_error_reported, true) unless error.frozen?
error = error.cause
end
end
alias :set_error :add_error
alias_method :add_exception, :add_error
Expand Down Expand Up @@ -592,6 +593,19 @@ def to_h
attr_writer :is_duplicate, :tags, :custom_data, :breadcrumbs, :params,
:session_data, :headers

def internal_set_error(error, &block)
_set_error(error) if @error_blocks.empty?

if !@error_blocks.include?(error) && @error_blocks.length >= ERRORS_LIMIT
Appsignal.internal_logger.warn "Appsignal::Transaction#add_error: Transaction has more " \
"than #{ERRORS_LIMIT} distinct errors. Only the first " \
"#{ERRORS_LIMIT} distinct errors will be reported."
return
end
@error_blocks[error] << block
@error_blocks[error].compact!
end

private

attr_reader :breadcrumbs
Expand Down
37 changes: 37 additions & 0 deletions spec/lib/appsignal/transaction_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1568,6 +1568,43 @@ def to_s
end
end

context "when the error is already reported" do
it "does not add the error" do
# Report it on another transaction first
transaction1 = new_transaction
transaction1.add_error(error)
expect(transaction1).to have_error

transaction2 = new_transaction
transaction2.add_error(error)
expect(transaction2).to_not have_error
end
end
unflxw marked this conversation as resolved.
Show resolved Hide resolved

context "when the error cause is already reported" do
it "does not add the error" do
error =
begin
begin
raise ExampleStandardError, "error cause"
rescue
raise ExampleException, "error wrapper"
end
rescue ExampleException => e
e
end

# Report the wrapper on another transaction first
transaction1 = new_transaction
transaction1.add_error(error)
expect(transaction1).to have_error

transaction2 = new_transaction
transaction2.add_error(error.cause)
expect(transaction).to_not have_error
end
end

context "when a block is given" do
it "stores the block in the error blocks" do
block = proc { "block" }
Expand Down
Loading