Ignore Errno::EPIPE error causes when reading body

We got another report of the `Puma::ConnectionError` error being reported when upgrading our Ruby gem. The error cause is always the already ignored `Errno::EPIPE` error. It is reported by our response body wrapper that's called when instrumenting response bodies. In this case the error was: Puma::ConnectionError: Socket timeout writing data The cause of this error was this error: > Errno::EPIPE: Broken pipe This doesn't look like an error people can act on to fix it, let's ignore it instead. Related issue appsignal/support#315
appsignal · Sep 16, 2024 · 1fdccba · 1fdccba
1 parent 1aba950
commit 1fdccba
Show file tree

Hide file tree

Showing 3 changed files with 114 additions and 5 deletions.
diff --git a/...esets/do-not-report-puma--connectionerror-when-instrumenting-response-bodies.md b/...esets/do-not-report-puma--connectionerror-when-instrumenting-response-bodies.md
@@ -0,0 +1,6 @@
+---
+bump: patch
+type: change
+---
+
+Do not report errors caused by `Errno::EPIPE` (broken pipe errors) when instrumenting response bodies, to avoid reporting errors that cannot be fixed by the application.
diff --git a/lib/appsignal/rack/body_wrapper.rb b/lib/appsignal/rack/body_wrapper.rb
@@ -54,7 +54,7 @@ def close
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
 
@@ -72,6 +72,18 @@ def method_missing(method_name, *args, &block)
         @body.__send__(method_name, *args, &block)
       end
       ruby2_keywords(:method_missing) if respond_to?(:ruby2_keywords, true)
+
+      private
+
+      def appsignal_report_error(error)
+        @transaction.set_error(error) if appsignal_accepted_error?(error)
+      end
+
+      def appsignal_accepted_error?(error)
+        return true unless error.cause
+
+        !IGNORED_ERRORS.include?(error.cause.class)
+      end
     end
 
     # The standard Rack body wrapper which exposes "each" for iterating
@@ -97,7 +109,7 @@ def each(&blk)
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
     end
@@ -118,7 +130,7 @@ def call(stream)
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
     end
@@ -144,7 +156,7 @@ def to_ary
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
     end
@@ -162,7 +174,7 @@ def to_path
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
     end

diff --git a/spec/lib/appsignal/rack/body_wrapper_spec.rb b/spec/lib/appsignal/rack/body_wrapper_spec.rb
@@ -90,6 +90,19 @@
       expect(transaction).to_not have_error
     end
 
+    it "does not report EPIPE error when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_body = double
+      expect(fake_body).to receive(:each).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        expect { |b| wrapped.each(&b) }.to yield_control
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
     it "closes the body and tracks an instrumentation event when it gets closed" do
       fake_body = double(:close => nil)
       expect(fake_body).to receive(:each).once.and_yield("a").and_yield("b").and_yield("c")
@@ -165,6 +178,19 @@
       expect(transaction).to_not have_error
     end
 
+    it "does not report EPIPE error when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_body = double
+      expect(fake_body).to receive(:each).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        expect { |b| wrapped.each(&b) }.to yield_control
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
     it "reads out the body in full using to_ary" do
       expect(fake_body).to receive(:to_ary).and_return(["one", "two", "three"])
 
@@ -190,6 +216,21 @@
 
       expect(transaction).to have_error("ExampleException", "error message")
     end
+
+    it "does not report EPIPE error when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_body = double
+      allow(fake_body).to receive(:each)
+      expect(fake_body).to receive(:to_ary).once.and_raise(error)
+      expect(fake_body).to_not receive(:close) # Per spec we expect the body has closed itself
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        wrapped.to_ary
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
   end
 
   describe "with a body supporting both to_path and each" do
@@ -249,6 +290,30 @@
       expect(transaction).to_not have_error
     end
 
+    it "does not report EPIPE error from #each when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      expect(fake_body).to receive(:each).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        expect { |b| wrapped.each(&b) }.to yield_control
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
+    it "does not report EPIPE error from #to_path when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      allow(fake_body).to receive(:to_path).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        wrapped.to_path
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
     it "exposes to_path to the sender" do
       allow(fake_body).to receive(:to_path).and_return("/tmp/file.bin")
 
@@ -315,5 +380,31 @@
 
       expect(transaction).to_not have_error
     end
+
+    it "does not report EPIPE error from #call when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_rack_stream = double
+      allow(fake_body).to receive(:call)
+        .with(fake_rack_stream)
+        .and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+
+      expect do
+        wrapped.call(fake_rack_stream)
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+  end
+
+  def error_with_cause(klass, message, cause)
+    begin
+      raise cause
+    rescue
+      raise klass, message
+    end
+  rescue => error
+    error
   end
 end