Limit additional errors

Report at most ten additional errors per transaction. If more errors
are provided, report only the last ten additional errors.

Pass the extension as an option when initialising the transaction
object.

lib/appsignal/transaction.rb

@@ -20,6 +20,7 @@ class Transaction
     BREADCRUMB_LIMIT = 20
     # @api private
     ERROR_CAUSES_LIMIT = 10
+    ADDITIONAL_ERRORS_LIMIT = 10
 
     class << self
       # Create a new transaction and set it as the currently active
@@ -96,7 +97,7 @@ def clear_current_transaction!
     # @param namespace [String] Namespace of the to be created transaction.
     # @see create
     # @api private
-    def initialize(transaction_id, namespace)
+    def initialize(transaction_id, namespace, ext: nil)
       @transaction_id = transaction_id
       @action = nil
       @namespace = namespace
@@ -113,7 +114,7 @@ def initialize(transaction_id, namespace)
       @errors = []
       @is_duplicate = false
 
-      @ext = Appsignal::Extension.start_transaction(
+      @ext = ext || Appsignal::Extension.start_transaction(
         @transaction_id,
         @namespace,
         0
@@ -131,9 +132,20 @@ 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?
 
-      sample_data if ext.finish(0) && !is_duplicate
+      # 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
+      # data, because it should keep the sample data it duplicated from
+      # the original transaction.
+      # On duplicate transactions, the value of the sample flag, which
+      # is set on finish, will be duplicated from the original transaction.
+      sample_data if !is_duplicate && ext.finish(0)
 
       errors.each do |error|
         duplicate.tap do |transaction|
@@ -452,6 +464,12 @@ def set_metadata(key, value)
 
     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
@@ -606,16 +624,13 @@ def sample_data
     end
 
     def duplicate
+      new_transaction_id = SecureRandom.uuid
+
       self.class.new(
-        SecureRandom.uuid,
+        new_transaction_id,
         namespace,
-        request,
-        options
+        :ext => ext.duplicate(new_transaction_id)
       ).tap do |transaction|
-        transaction.ext = ext.duplicate(
-          transaction.transaction_id
-        )
-
         transaction.is_duplicate = true
       end
     end

spec/lib/appsignal/transaction_spec.rb

@@ -35,6 +35,14 @@
       end
     end
 
+    context "when an explicit extension transaction is passed in the initialiser" do
+      let(:ext) { "some_ext" }
+
+      it "assigns the extension transaction to the transaction" do
+        expect(new_transaction(:ext => ext).ext).to be(ext)
+      end
+    end
+
     context "when a transaction is already running" do
       before do
         allow(SecureRandom).to receive(:uuid)
@@ -1195,50 +1203,52 @@ def to_s
     end
   end
 
-    describe "#add_error" do
-      let(:error) do
-        e = ExampleStandardError.new("test message")
-        allow(e).to receive(:backtrace).and_return(["line 1"])
-        e
-      end
+  describe "#add_error" do
+    let(:transaction) { create_transaction }
 
-      let(:other_error) do
-        e = ExampleStandardError.new("other test message")
-        allow(e).to receive(:backtrace).and_return(["line 2"])
-        e
-      end
+    let(:error) do
+      e = ExampleStandardError.new("test message")
+      allow(e).to receive(:backtrace).and_return(["line 1"])
+      e
+    end
 
-      it "stores an error in the errors array" do
-        transaction.add_error(error)
+    let(:other_error) do
+      e = ExampleStandardError.new("other test message")
+      allow(e).to receive(:backtrace).and_return(["line 2"])
+      e
+    end
 
-        expect(transaction.errors).to eq([error])
-      end
+    it "stores an error in the errors array" do
+      transaction.add_error(error)
 
-      it "does not set the error on the extension" do
-        transaction.add_error(error)
+      expect(transaction.errors).to eq([error])
+    end
 
-        expect(transaction.to_h["error"]).to be_nil
-      end
+    it "does not set the error on the extension" do
+      transaction.add_error(error)
 
-      it "does not set the has_error attribute to true" do
-        expect(transaction.has_error).to be_falsy
+      expect(transaction.to_h["error"]).to be_nil
+    end
 
-        transaction.add_error(error)
+    it "does not set the has_error attribute to true" do
+      expect(transaction.has_error).to be_falsy
 
-        expect(transaction.has_error).to be_falsy
-      end
+      transaction.add_error(error)
 
-      it "does not override an error set with set_error" do
-        transaction.set_error(error)
-        transaction.add_error(other_error)
+      expect(transaction.has_error).to be_falsy
+    end
 
-        expect(transaction.to_h["error"]).to eq(
-          "name" => "ExampleStandardError",
-          "message" => "test message",
-          "backtrace" => ["line 1"].to_json
-        )
-      end
+    it "does not override an error set with set_error" do
+      transaction.set_error(error)
+      transaction.add_error(other_error)
+
+      expect(transaction.to_h["error"]).to eq(
+        "name" => "ExampleStandardError",
+        "message" => "test message",
+        "backtrace" => ["line 1"].to_json
+      )
     end
+  end
 
   describe "#set_error" do
     let(:transaction) { new_transaction }
@@ -1257,19 +1267,19 @@ def to_s
       expect(transaction).to_not have_error
     end
 
-      it "does not add the error to the errors array" do
-        transaction.set_error(error)
+    it "does not add the error to the errors array" do
+      transaction.set_error(error)
 
-        expect(transaction.errors).to be_empty
-      end
+      expect(transaction.errors).to be_empty
+    end
 
-      it "sets the has_error attribute to true" do
-        expect(transaction.has_error).to be_falsy
+    it "sets the has_error attribute to true" do
+      expect(transaction.has_error).to be_falsy
 
-        transaction.set_error(error)
+      transaction.set_error(error)
 
-        expect(transaction.has_error).to be_truthy
-      end
+      expect(transaction.has_error).to be_truthy
+    end
 
     context "when error argument is not an error" do
       let(:error) { Object.new }
@@ -1299,13 +1309,13 @@ def to_s
       end
     end
 
-      context "when the error has no causes" do
-        it "should set an empty causes array as sample data" do
-          transaction.set_error(error)
+    context "when the error has no causes" do
+      it "should set an empty causes array as sample data" do
+        transaction.set_error(error)
 
-          expect(transaction).to include_error_causes([])
-        end
+        expect(transaction).to include_error_causes([])
       end
+    end
 
     context "when the error has multiple causes" do
       let(:error) do
@@ -1317,20 +1327,10 @@ def to_s
         allow(e2).to receive(:cause).and_return(e3)
         e
       end
-      context "when the error has multiple causes" do
-        let(:error) do
-          e = ExampleStandardError.new("test message")
-          e2 = RuntimeError.new("cause message")
-          e3 = StandardError.new("cause message 2")
-          allow(e).to receive(:backtrace).and_return(["line 1"])
-          allow(e).to receive(:cause).and_return(e2)
-          allow(e2).to receive(:cause).and_return(e3)
-          e
-        end
 
-        let(:error_without_cause) do
-          ExampleStandardError.new("error without cause")
-        end
+      let(:error_without_cause) do
+        ExampleStandardError.new("error without cause")
+      end
 
       it "sends the causes information as sample data" do
         transaction.set_error(error)
@@ -1567,49 +1567,50 @@ def to_s
 
   # private
 
-    describe "#duplicate" do
-      let(:options) do
-        { :some => :option }
-      end
+  describe "#duplicate" do
+    let(:transaction) { new_transaction }
 
-      subject { transaction.send(:duplicate) }
+    subject { transaction.send(:duplicate) }
 
-      it "returns a new transaction" do
-        is_expected.to be_a(Appsignal::Transaction)
-        is_expected.to_not be(transaction)
-      end
+    it "returns a new transaction" do
+      is_expected.to be_a(Appsignal::Transaction)
+      is_expected.to_not be(transaction)
+    end
 
-      it "has a different transaction id" do
-        expect(subject.transaction_id).to_not eq(transaction.transaction_id)
-      end
+    it "has a different transaction id" do
+      expect(subject.transaction_id).to_not eq(transaction.transaction_id)
+    end
 
-      it "copies the namespace, request and options" do
-        is_expected.to have_attributes(
-          :namespace => namespace,
-          :request => request,
-          :options => options
-        )
-      end
+    it "copies the namespace and request" do
+      expect(subject.namespace).to eq(transaction.namespace)
+      expect(subject.request).to eq(transaction.request)
+    end
 
-      it "has is_duplicate set to true" do
-        expect(transaction).to have_attributes(:is_duplicate => false)
-        expect(subject).to have_attributes(:is_duplicate => true)
-      end
+    it "has a different extension transaction than the original" do
+      expect(subject.ext).to be_a(Appsignal::Extension::Transaction)
+      expect(subject.ext).to_not be(transaction.ext)
+    end
 
-      it "duplicates extension data" do
-        transaction.set_error(StandardError.new("oops"))
-        transaction.set_sample_data("key", "value")
-        transaction.set_metadata("key", "value")
-        transaction.start_event
-        transaction.finish_event("name", "title", "body", 1)
+    it "has is_duplicate set to true" do
+      expect(transaction).to have_attributes(:is_duplicate => false)
+      expect(subject).to have_attributes(:is_duplicate => true)
+    end
+
+    it "duplicates extension data" do
+      transaction.set_error(StandardError.new("oops"))
+      transaction.set_custom_data({ :key => "value" })
+      transaction.set_metadata("key", "value")
+      transaction.start_event
+      transaction.finish_event("name", "title", "body", 1)
+      # Simulate sample data being written to the extension on complete
+      transaction.send(:sample_data)
 
-        subject_hash = subject.to_h.tap { |h| h.delete("id") }
-        transaction_hash = transaction.to_h.tap { |h| h.delete("id") }
+      subject_hash = subject.to_h.tap { |h| h.delete("id") }
+      transaction_hash = transaction.to_h.tap { |h| h.delete("id") }
 
-        expect(subject_hash).to eq(transaction_hash)
-      end
+      expect(subject_hash).to eq(transaction_hash)
     end
-
+  end
   describe "#cleaned_backtrace" do
     let(:transaction) { new_transaction }
     subject { transaction.send(:cleaned_backtrace, ["line 1", "line 2"]) }

spec/support/helpers/transaction_helpers.rb

@@ -15,8 +15,8 @@ def create_transaction(namespace = default_namespace)
     Appsignal::Transaction.create(namespace)
   end
 
-  def new_transaction(namespace = default_namespace)
-    Appsignal::Transaction.new(SecureRandom.uuid, namespace)
+  def new_transaction(namespace = default_namespace, ext: nil)
+    Appsignal::Transaction.new(SecureRandom.uuid, namespace, :ext => ext)
   end
 
   def rack_request(env)