From 2a2e3dd7af78adfbb5c54fb44c1e1646493017bd Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Thu, 25 Jul 2024 20:21:06 +0200 Subject: [PATCH] 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 https://github.com/appsignal/support/issues/315 --- ...rror-when-instrumenting-response-bodies.md | 6 ++ lib/appsignal/rack/body_wrapper.rb | 22 ++++- spec/lib/appsignal/rack/body_wrapper_spec.rb | 91 +++++++++++++++++++ 3 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 .changesets/do-not-report-puma--connectionerror-when-instrumenting-response-bodies.md diff --git a/.changesets/do-not-report-puma--connectionerror-when-instrumenting-response-bodies.md b/.changesets/do-not-report-puma--connectionerror-when-instrumenting-response-bodies.md new file mode 100644 index 000000000..686e0deae --- /dev/null +++ b/.changesets/do-not-report-puma--connectionerror-when-instrumenting-response-bodies.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: change +--- + +Do not report `Puma::ConnectionError` 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 index 0fc36e09c..2b82af999 100644 --- 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 index 08467543b..aefbf46da 100644 --- 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