From 1fdccba4ceeb8f9bb13ae077019b2c1f7d9d4fe4 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Thu, 25 Jul 2024 20:21:06 +0200 Subject: [PATCH 1/3] 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 00000000..02e5d82d --- /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 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 index 0fc36e09..2b82af99 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 08467543..aefbf46d 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 From a2775db26cebe82f9fb88823b6a8bc66d1cb90af Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Thu, 12 Sep 2024 16:48:01 +0200 Subject: [PATCH 2/3] Add error specs for BodyWrapper close method I noticed I broke something earlier while writing the previous commit in the close method, but it didn't fail any specs. That's because there weren't any for the error reporting. Add these errors so we can be sure it works. --- spec/lib/appsignal/rack/body_wrapper_spec.rb | 37 ++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/spec/lib/appsignal/rack/body_wrapper_spec.rb b/spec/lib/appsignal/rack/body_wrapper_spec.rb index aefbf46d..69450b01 100644 --- a/spec/lib/appsignal/rack/body_wrapper_spec.rb +++ b/spec/lib/appsignal/rack/body_wrapper_spec.rb @@ -113,6 +113,43 @@ expect(transaction).to include_event("name" => "close_response_body.rack") end + + it "reports an error if an error occurs on close" do + fake_body = double + expect(fake_body).to receive(:close).and_raise(ExampleException, "error message") + + wrapped = described_class.wrap(fake_body, transaction) + expect do + wrapped.close + end.to raise_error(ExampleException, "error message") + + expect(transaction).to have_error("ExampleException", "error message") + end + + it "doesn't report EPIPE error on close" do + fake_body = double + expect(fake_body).to receive(:close).and_raise(Errno::EPIPE) + + wrapped = described_class.wrap(fake_body, transaction) + expect do + wrapped.close + end.to raise_error(Errno::EPIPE) + + expect(transaction).to_not have_error + end + + it "does not report EPIPE error when it's the error cause on close" do + error = error_with_cause(StandardError, "error message", Errno::EPIPE) + fake_body = double + expect(fake_body).to receive(:close).and_raise(error) + + wrapped = described_class.wrap(fake_body, transaction) + expect do + wrapped.close + end.to raise_error(StandardError, "error message") + + expect(transaction).to_not have_error + end end describe "with a body supporting both each() and call" do From 46cd9acd25c18620e5437cd12b66b7fb663f897d Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Mon, 16 Sep 2024 12:26:21 +0200 Subject: [PATCH 3/3] Ignore error if nested cause is ignored Traverse the error causes tree to see if any nested error cause is ignored. --- lib/appsignal/rack/body_wrapper.rb | 3 ++- spec/lib/appsignal/rack/body_wrapper_spec.rb | 27 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lib/appsignal/rack/body_wrapper.rb b/lib/appsignal/rack/body_wrapper.rb index 2b82af99..21211f24 100644 --- a/lib/appsignal/rack/body_wrapper.rb +++ b/lib/appsignal/rack/body_wrapper.rb @@ -81,8 +81,9 @@ def appsignal_report_error(error) def appsignal_accepted_error?(error) return true unless error.cause + return false if IGNORED_ERRORS.include?(error.cause.class) - !IGNORED_ERRORS.include?(error.cause.class) + appsignal_accepted_error?(error.cause) end end diff --git a/spec/lib/appsignal/rack/body_wrapper_spec.rb b/spec/lib/appsignal/rack/body_wrapper_spec.rb index 69450b01..7ed5a307 100644 --- a/spec/lib/appsignal/rack/body_wrapper_spec.rb +++ b/spec/lib/appsignal/rack/body_wrapper_spec.rb @@ -103,6 +103,19 @@ expect(transaction).to_not have_error end + it "does not report EPIPE error when it's the nested error cause" do + error = error_with_nested_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") @@ -444,4 +457,18 @@ def error_with_cause(klass, message, cause) rescue => error error end + + def error_with_nested_cause(klass, message, cause) + begin + begin + raise cause + rescue + raise klass, message + end + rescue + raise klass, message + end + rescue => error + error + end end