Run error blocks eagerly in collector mode

In collector mode each error is recorded when it is added, but its block
was still held until completion. Deferring only makes sense in agent
mode, where blocks run against the duplicate transactions fabricated at
completion. In collector mode there is no benefit to holding them back.

Run the block when the error is added instead. Side effects that target
the current span, such as breadcrumbs, nested errors or custom
instrumentation, then land where the error was reported rather than on
the root span at completion. None of those belong in an error block, but
this puts them in the right place if they are used.

With blocks no longer deferred in collector mode, the error tracking
that supported deferral was doing two jobs at once. `@error_blocks` was
both the set of distinct errors, which drives `last_errors`, the dedup
check and the error limit, and the per-error blocks that only agent mode
runs. Split them. `@errors` is now the distinct-error set, used in both
modes, and `@error_blocks` holds only blocks and is populated only in
agent mode. `@errors` is a Set, so membership is by object identity,
matching how the errors were deduplicated as hash keys before.

Finally, rename `records_errors_eagerly?` to `supports_multiple_errors?`.
The old name described the mechanism. The real question at the call site
is whether the backend can hold more than one error on a transaction.

Author: unflxw

lib/appsignal/transaction.rb

@@ -174,6 +174,16 @@ def initialize(namespace, id: SecureRandom.uuid, backend: nil, opentelemetry_con
       @completed = false
       @tags = {}
       @store = Hash.new { |hash, key| hash[key] = {} }
+      # The distinct errors added to this transaction, in add order. Drives
+      # `last_errors`, the dedup check and the `ERRORS_LIMIT`, in both modes.
+      # A Set, so membership is by object identity (`eql?`/`hash`), matching how
+      # these errors used to be deduplicated as Hash keys. `Exception#==` would
+      # instead collapse distinct errors with the same class, message and
+      # backtrace, which we don't want.
+      @errors = Set.new
+      # Blocks per error, only populated in agent mode, where they run against
+      # the duplicate transactions at completion. Collector mode runs blocks
+      # when the error is added and never touches this.
       @error_blocks = Hash.new { |hash, key| hash[key] = [] }
       @is_duplicate = false
       @error_set = nil
@@ -236,7 +246,7 @@ def complete
       should_sample = true
 
       unless duplicate?
-        self.class.last_errors = @error_blocks.keys
+        self.class.last_errors = @errors.to_a
         should_sample = @backend.finish
       end
 
@@ -627,7 +637,7 @@ def add_error(error, &block)
       return unless error
       return unless Appsignal.active?
 
-      if error.instance_variable_get(:@__appsignal_error_reported) && !@error_blocks.include?(error)
+      if error.instance_variable_get(:@__appsignal_error_reported) && !@errors.include?(error)
         return
       end
 
@@ -715,26 +725,37 @@ def to_h
 
     # @!visibility private
     def internal_set_error(error, &block)
-      is_new_error = !@error_blocks.include?(error)
+      is_new_error = !@errors.include?(error)
 
-      if is_new_error && @error_blocks.length >= ERRORS_LIMIT
+      if is_new_error && @errors.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
 
-      if @error_blocks.empty?
+      if @errors.empty?
         _set_error(error)
-      elsif is_new_error && @backend.records_errors_eagerly?
+      elsif is_new_error && @backend.supports_multiple_errors?
         # Record additional errors immediately so each exception event lands on
         # the span current now, not the root span at completion. The agent
         # backend instead reports extras as duplicate transactions.
         _send_error_to_backend(error)
       end
 
-      @error_blocks[error] << block
-      @error_blocks[error].compact!
+      @errors.add(error)
+
+      if @backend.supports_multiple_errors?
+        # Collector mode: the error is already recorded, so run its block now
+        # rather than at completion. Anything a block attaches to the current
+        # span -- breadcrumbs, nested errors, custom instrumentation -- then
+        # lands where the error was reported, not on the root span at
+        # completion.
+        self.class.with_transaction(self) { block.call(self) } if block
+      else
+        @error_blocks[error] << block
+        @error_blocks[error].compact!
+      end
     end
 
     private
@@ -751,40 +772,25 @@ def run_before_complete_hooks
       end
     end
 
-    # Reports the errors stored on the transaction at completion, in one of two
-    # ways depending on the backend:
+    # Reports the errors stored on the transaction at completion.
     #
-    #   - eager (collector): each error was already recorded as its own exception
-    #     event when added, on the span current at that moment; here we only run
-    #     the error blocks.
-    #   - deferred (agent): the extension holds a single error, so the primary
-    #     error's blocks run on this transaction and every additional error is
-    #     reported as a duplicate transaction.
+    # In eager (collector) mode nothing is left to do: each error was recorded
+    # and its block run when the error was added. In deferred (agent) mode the
+    # extension holds a single error, so the primary error's blocks run on this
+    # transaction and every additional error is reported as a duplicate
+    # transaction.
     def report_errors
-      if @backend.records_errors_eagerly?
-        run_error_blocks
-      else
-        report_errors_as_duplicates
-      end
-    end
+      return if @backend.supports_multiple_errors?
 
-    # Eager mode: the errors are already recorded, so just run their blocks.
-    # Blocks run in add-order, so a later error's block wins on a shared key, and
-    # all block-set metadata merges onto the root span. (Per-error metadata
-    # isolation is deferred -- the processor/UI does not read per-event
-    # attributes yet.)
-    def run_error_blocks
-      @error_blocks.each_value do |blocks|
-        self.class.with_transaction(self) do
-          blocks.each { |block| block.call(self) }
-        end
-      end
+      report_errors_as_duplicates
     end
 
-    # Agent mode: the extension transaction holds a single error, so report each
-    # additional error as a duplicate transaction.
+    # Agent-only legacy path. The extension transaction holds a single error, so
+    # extra errors are reported as duplicate transactions. This whole method
+    # disappears once agent mode is dropped: collector mode records every error
+    # eagerly and leaves nothing to do at completion.
     def report_errors_as_duplicates
-      @error_blocks.each do |error, blocks|
+      @errors.each do |error|
         # Ignore the error that is already set in this transaction.
         next if error == @error_set
 
@@ -793,7 +799,7 @@ def report_errors_as_duplicates
           # with a block that calls all the blocks set for that error
           # in the original transaction.
           transaction.internal_set_error(error) do
-            blocks.each { |block| block.call(transaction) }
+            @error_blocks[error].each { |block| block.call(transaction) }
           end
 
           transaction.complete

lib/appsignal/transaction/base_backend.rb

@@ -52,9 +52,10 @@ def set_error(_class_name, _message, _backtrace, _causes, _root_cause_missing)
         raise NotImplementedError
       end
 
-      # Whether the backend records each error eagerly onto one trace, or relies
-      # on the Transaction duplicating itself per error.
-      def records_errors_eagerly?
+      # Whether the backend can hold more than one error on a single
+      # transaction. When it can't (agent mode), the Transaction reports the
+      # extra errors as duplicate transactions instead.
+      def supports_multiple_errors?
         raise NotImplementedError
       end
 
@@ -71,8 +72,9 @@ def discard
         raise NotImplementedError
       end
 
-      # Only used when `records_errors_eagerly?` is false (agent mode). Backends
-      # that record eagerly never duplicate and leave this unimplemented.
+      # Only used when `supports_multiple_errors?` is false (agent mode).
+      # Backends that support multiple errors never duplicate and leave this
+      # unimplemented.
       def duplicate(_new_transaction_id)
         raise NotImplementedError
       end

lib/appsignal/transaction/extension_backend.rb

@@ -109,7 +109,7 @@ def discard
 
       # The extension transaction holds a single error, so the Transaction
       # reports additional errors as duplicate transactions instead.
-      def records_errors_eagerly?
+      def supports_multiple_errors?
         false
       end
 

lib/appsignal/transaction/opentelemetry_backend.rb

@@ -268,9 +268,10 @@ def discard
       end
 
       # Each error is recorded eagerly as its own `exception` event on the span
-      # current when it was added, so the Transaction never duplicates itself --
-      # which is why `duplicate` is left unimplemented (see BaseBackend).
-      def records_errors_eagerly?
+      # current when it was added, so a trace holds many errors and the
+      # Transaction never duplicates itself -- which is why `duplicate` is left
+      # unimplemented (see BaseBackend).
+      def supports_multiple_errors?
         true
       end
 

spec/lib/appsignal/transaction/extension_backend_spec.rb

@@ -218,9 +218,9 @@
     end
   end
 
-  describe "#records_errors_eagerly?" do
+  describe "#supports_multiple_errors?" do
     it "returns false (extra errors are reported as duplicate transactions)" do
-      expect(backend.records_errors_eagerly?).to eq(false)
+      expect(backend.supports_multiple_errors?).to eq(false)
     end
   end
 end

spec/lib/appsignal/transaction/opentelemetry_backend_spec.rb

@@ -475,9 +475,9 @@ def exception_event(backend)
     end
   end
 
-  describe "#records_errors_eagerly?" do
+  describe "#supports_multiple_errors?" do
     it "returns true (multiple exception events on one span)" do
-      expect(create_backend.records_errors_eagerly?).to eq(true)
+      expect(create_backend.supports_multiple_errors?).to eq(true)
     end
   end
 

spec/lib/appsignal/transaction_spec.rb

@@ -203,6 +203,26 @@
         expect(event_span.events.map(&:name)).to include("exception", "appsignal.breadcrumb")
         expect(Array(foreign_span.events).map(&:name)).to be_empty
       end
+
+      it "runs an error block on the span current when the error was added", :collector_mode do
+        start_collector_agent
+        transaction = create_transaction(Appsignal::Transaction::HTTP_REQUEST)
+
+        # The block runs when the error is added, while the event span is still
+        # current -- not deferred to completion, when the root span would be
+        # current. A breadcrumb added from the block lands on the event span.
+        transaction.start_event
+        transaction.add_error(ExampleStandardError.new("boom")) do |t|
+          t.add_breadcrumb("network", "GET /", "ok", { "code" => "200" })
+        end
+        transaction.finish_event("sql.query", "Query", "SELECT 1",
+          Appsignal::EventFormatter::DEFAULT)
+        Appsignal::Transaction.complete_current!
+
+        event_span = event_spans.find { |s| s.attributes["appsignal.category"] == "sql.query" }
+        expect(event_span.events.map(&:name)).to include("exception", "appsignal.breadcrumb")
+        expect(Array(root_span.events).map(&:name)).to_not include("appsignal.breadcrumb")
+      end
     end
   end