Merge pull request #8776 from chef/lcg/data-collector-exception-handling

Tweak data collector exception handling

2 files changed, 97 insertions, 8 deletions

diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index aac864f485..931671bf50 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -177,14 +177,17 @@ class Chef
 
         @http ||= setup_http_client(Chef::Config[:data_collector][:server_url])
         @http.post(nil, message, headers)
-      rescue Timeout::Error, Errno::EINVAL, Errno::ECONNRESET,
-        Errno::ECONNREFUSED, EOFError, Net::HTTPBadResponse,
-        Net::HTTPHeaderSyntaxError, Net::ProtocolError, OpenSSL::SSL::SSLError,
-        Errno::EHOSTDOWN => e
+      rescue => e
         # Do not disable data collector reporter if additional output_locations have been specified
         events.unregister(self) unless Chef::Config[:data_collector][:output_locations]
 
-        code = e&.response&.code&.to_s || "Exception Code Empty"
+        begin
+          code = e&.response&.code&.to_s
+        rescue
+          # i really dont care
+        end
+
+        code ||= "No HTTP Code"
 
         msg = "Error while reporting run start to Data Collector. URL: #{Chef::Config[:data_collector][:server_url]} Exception: #{code} -- #{e.message} "
 
@@ -192,9 +195,13 @@ class Chef
           Chef::Log.error(msg)
           raise
         else
-          # Make the message non-scary for folks who don't have automate:
-          msg << " (This is normal if you do not have #{Chef::Dist::AUTOMATE})"
-          Chef::Log.info(msg)
+          if code == "404"
+            # Make the message non-scary for folks who don't have automate:
+            msg << " (This is normal if you do not have #{Chef::Dist::AUTOMATE})"
+            Chef::Log.debug(msg)
+          else
+            Chef::Log.warn(msg)
+          end
         end
       end
 
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index d7b18b3c28..2f33a429fc 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -879,4 +879,86 @@ describe Chef::DataCollector do
       data_collector.send(:send_to_file_location, tempfile, { invalid: shift_jis })
     end
   end
+
+  describe "#send_to_datacollector" do
+    def stub_http_client(exception = nil)
+      if exception.nil?
+        expect(http_client).to receive(:post).with(nil, message, data_collector.send(:headers))
+      else
+        expect(http_client).to receive(:post).with(nil, message, data_collector.send(:headers)).and_raise(exception)
+      end
+    end
+
+    let(:message) { "message" }
+    let(:http_client) { instance_double(Chef::ServerAPI) }
+
+    before do
+      expect(data_collector).to receive(:setup_http_client).and_return(http_client)
+    end
+
+    it "does not disable the data_collector when no exception is raised" do
+      stub_http_client
+      expect(data_collector.events).not_to receive(:unregister)
+      data_collector.send(:send_to_data_collector, message)
+    end
+
+    errors = [ Timeout::Error, Errno::EINVAL, Errno::ECONNRESET,
+               Errno::ECONNREFUSED, EOFError, Net::HTTPBadResponse,
+               Net::HTTPHeaderSyntaxError, Net::ProtocolError, OpenSSL::SSL::SSLError,
+               Errno::EHOSTDOWN ]
+
+    errors.each do |exception_class|
+      context "when the client raises a #{exception_class} exception" do
+        before do
+          stub_http_client(exception_class)
+        end
+
+        it "disables the reporter" do
+          expect(data_collector.events).to receive(:unregister).with(data_collector)
+          data_collector.send(:send_to_data_collector, message)
+        end
+
+        it "logs an error and raises when raise_on_failure is enabled" do
+          Chef::Config[:data_collector][:raise_on_failure] = true
+          expect(Chef::Log).to receive(:error)
+          expect { data_collector.send(:send_to_data_collector, message) }.to raise_error(exception_class)
+        end
+
+        it "logs a warn message and does not raise an exception when raise_on_failure is disabled" do
+          Chef::Config[:data_collector][:raise_on_failure] = false
+          expect(Chef::Log).to receive(:warn)
+          data_collector.send(:send_to_data_collector, message)
+        end
+      end
+    end
+
+    context "when the client raises a 404 exception" do
+      let(:err) do
+        response = double("Net::HTTP response", code: "404")
+        Net::HTTPClientException.new("Not Found", response)
+      end
+
+      before do
+        stub_http_client(err)
+      end
+
+      it "disables the reporter" do
+        expect(data_collector.events).to receive(:unregister).with(data_collector)
+        data_collector.send(:send_to_data_collector, message)
+      end
+
+      it "logs an error and raises when raise_on_failure is enabled" do
+        Chef::Config[:data_collector][:raise_on_failure] = true
+        expect(Chef::Log).to receive(:error)
+        expect { data_collector.send(:send_to_data_collector, message) }.to raise_error(err)
+      end
+
+      # this is different specifically for 404s
+      it "logs an info message and does not raise an exception when raise_on_failure is disabled" do
+        Chef::Config[:data_collector][:raise_on_failure] = false
+        expect(Chef::Log).to receive(:debug).with(/This is normal if you do not have Chef Automate/)
+        data_collector.send(:send_to_data_collector, message)
+      end
+    end
+  end
 end