Expand data_collector resource list to include all resources

Historically when a Chef run fails, the event handlers only report
on resources that have been processed during the run. This change
allows the Data Collector to report those resources in the resource
collection that have not yet been processed so users can better
ascertain how much of their run was completed, and specifically
what resources were not processed as a result of the Chef failure.

Instead of building up a list of resources as they are processed,
this change instead creates a resource report for each resource+action
in the resource collection and then modifies each of those reports
once the resource is processed.

5 files changed, 133 insertions, 59 deletions

diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index c8dd354f60..e7ef8d39ec 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -22,6 +22,7 @@ require "uri"
 require "chef/event_dispatch/base"
 require "chef/data_collector/messages"
 require "chef/data_collector/resource_report"
+require "ostruct"
 
 class Chef
 
@@ -51,12 +52,12 @@ class Chef
     # and exports its data through a webhook-like mechanism to a configured
     # endpoint.
     class Reporter < EventDispatch::Base
-      attr_reader :completed_resources, :status, :exception, :error_descriptions,
-                  :expanded_run_list, :run_status, :http,
+      attr_reader :all_resource_reports, :status, :exception, :error_descriptions,
+                  :expanded_run_list, :run_context, :run_status, :http,
                   :current_resource_report, :enabled
 
       def initialize
-        @completed_resources     = []
+        @all_resource_reports    = []
         @current_resource_loaded = nil
         @error_descriptions      = {}
         @expanded_run_list       = {}
@@ -93,6 +94,29 @@ class Chef
         send_run_completion(status: "failure")
       end
 
+      # see EventDispatch::Base#converge_start
+      # Upon receipt, we stash the run_context for use at the
+      # end of the run in order to determine what resource+action
+      # combinations have not yet fired so we can report on
+      # unprocessed resources.
+      def converge_start(run_context)
+        @run_context = run_context
+      end
+
+      # see EventDispatch::Base#converge_complete
+      # At the end of the converge, we add any unprocessed resources
+      # to our report list.
+      def converge_complete
+        detect_unprocessed_resources
+      end
+
+      # see EventDispatch::Base#converge_failed
+      # At the end of the converge, we add any unprocessed resources
+      # to our report list
+      def converge_failed(exception)
+        detect_unprocessed_resources
+      end
+
       # see EventDispatch::Base#resource_current_state_loaded
       # Create a new ResourceReport instance that we'll use to track
       # the state of this resource during the run. Nested resources are
@@ -100,13 +124,7 @@ class Chef
       # resource, and we only care about tracking top-level resources.
       def resource_current_state_loaded(new_resource, action, current_resource)
         return if nested_resource?(new_resource)
-        update_current_resource_report(
-          Chef::DataCollector::ResourceReport.new(
-            new_resource,
-            action,
-            current_resource
-          )
-        )
+        update_current_resource_report(create_resource_report(new_resource, action, current_resource))
       end
 
       # see EventDispatch::Base#resource_up_to_date
@@ -116,19 +134,15 @@ class Chef
       end
 
       # see EventDispatch::Base#resource_skipped
-      # If this is a top-level resource, we create a ResourceReport instance
-      # (because a skipped resource does not trigger the
+      # If this is a top-level resource, we create a ResourceReport
+      # instance (because a skipped resource does not trigger the
       # resource_current_state_loaded event), and flag it as skipped.
       def resource_skipped(new_resource, action, conditional)
         return if nested_resource?(new_resource)
 
-        update_current_resource_report(
-          Chef::DataCollector::ResourceReport.new(
-            new_resource,
-            action
-          )
-        )
-        current_resource_report.skipped(conditional)
+        resource_report = create_resource_report(new_resource, action)
+        resource_report.skipped(conditional)
+        update_current_resource_report(resource_report)
       end
 
       # see EventDispatch::Base#resource_updated
@@ -154,13 +168,12 @@ class Chef
       end
 
       # see EventDispatch::Base#resource_completed
-      # Mark the ResourceReport instance as finished (for timing details)
-      # and add it to the list of resources encountered during this run.
+      # Mark the ResourceReport instance as finished (for timing details).
       # This marks the end of this resource during this run.
       def resource_completed(new_resource)
         if current_resource_report && !nested_resource?(new_resource)
           current_resource_report.finish
-          add_completed_resource(current_resource_report)
+          add_resource_report(current_resource_report)
           update_current_resource_report(nil)
         end
       end
@@ -271,7 +284,7 @@ class Chef
           Chef::DataCollector::Messages.run_end_message(
             run_status: run_status,
             expanded_run_list: expanded_run_list,
-            completed_resources: completed_resources,
+            resources: all_resource_reports,
             status: opts[:status],
             error_descriptions: error_descriptions
           ).to_json
@@ -297,8 +310,12 @@ class Chef
         Chef::Config[:data_collector][:token]
       end
 
-      def add_completed_resource(resource_report)
-        @completed_resources << resource_report
+      def add_resource_report(resource_report)
+        @all_resource_reports << OpenStruct.new(
+          resource: resource_report.new_resource,
+          action: resource_report.action,
+          report_data: resource_report.to_hash
+        )
       end
 
       def disable_data_collector_reporter
@@ -321,6 +338,38 @@ class Chef
         @error_descriptions = discription_hash
       end
 
+      def create_resource_report(new_resource, action, current_resource = nil)
+        Chef::DataCollector::ResourceReport.new(
+          new_resource,
+          action,
+          current_resource
+        )
+      end
+
+      def detect_unprocessed_resources
+        # create a Set containing all resource+action combinations from
+        # the Resource Collection
+        collection_resources = Set.new
+        run_context.resource_collection.all_resources.each do |resource|
+          Array(resource.action).each do |action|
+            collection_resources.add([resource, action])
+          end
+        end
+
+        # Delete from the Set any resource+action combination we have
+        # already processed.
+        all_resource_reports.each do |report|
+          collection_resources.delete([report.resource, report.action])
+        end
+
+        # The items remaining in the Set are unprocessed resource+actions,
+        # so we'll create new resource reports for them which default to
+        # a state of "unprocessed".
+        collection_resources.each do |resource, action|
+          add_resource_report(create_resource_report(resource, action))
+        end
+      end
+
       # If we are getting messages about a resource while we are in the middle of
       # another resource's update, we assume that the nested resource is just the
       # implementation of a provider, and we want to hide it from the reporting
diff --git a/lib/chef/data_collector/messages.rb b/lib/chef/data_collector/messages.rb
index e23262c9e4..89bad9f9f9 100644
--- a/lib/chef/data_collector/messages.rb
+++ b/lib/chef/data_collector/messages.rb
@@ -70,15 +70,15 @@ class Chef
           "message_type"           => "run_converge",
           "node_name"              => run_status.node.name,
           "organization_name"      => organization,
-          "resources"              => reporter_data[:completed_resources].map(&:for_json),
+          "resources"              => reporter_data[:resources].map(&:report_data),
           "run_id"                 => run_status.run_id,
           "run_list"               => run_status.node.run_list.for_json,
           "start_time"             => run_status.start_time.utc.iso8601,
           "end_time"               => run_status.end_time.utc.iso8601,
           "source"                 => collector_source,
           "status"                 => reporter_data[:status],
-          "total_resource_count"   => reporter_data[:completed_resources].count,
-          "updated_resource_count" => reporter_data[:completed_resources].select { |r| r.status == "updated" }.count,
+          "total_resource_count"   => reporter_data[:resources].count,
+          "updated_resource_count" => reporter_data[:resources].select { |r| r.report_data["status"] == "updated" }.count,
         }
 
         message["error"] = {
diff --git a/lib/chef/data_collector/resource_report.rb b/lib/chef/data_collector/resource_report.rb
index 1793fe2c9d..89b6d26706 100644
--- a/lib/chef/data_collector/resource_report.rb
+++ b/lib/chef/data_collector/resource_report.rb
@@ -22,13 +22,14 @@ class Chef
   class DataCollector
     class ResourceReport
 
-      attr_reader :action, :current_resource, :elapsed_time, :new_resource, :status
-      attr_accessor :conditional, :exception
+      attr_reader :action, :elapsed_time, :new_resource, :status
+      attr_accessor :conditional, :current_resource, :exception
 
       def initialize(new_resource, action, current_resource = nil)
         @new_resource     = new_resource
         @action           = action
         @current_resource = current_resource
+        @status           = "unprocessed"
       end
 
       def skipped(conditional)
@@ -54,6 +55,14 @@ class Chef
         @elapsed_time = new_resource.elapsed_time
       end
 
+      def elapsed_time_in_milliseconds
+        elapsed_time.nil? ? nil : (elapsed_time * 1000).to_i
+      end
+
+      def potentially_changed?
+        %w{updated failed}.include?(status)
+      end
+
       def to_hash
         hash = {
           "type"     => new_resource.resource_name.to_sym,
@@ -61,8 +70,8 @@ class Chef
           "id"       => new_resource.identity.to_s,
           "after"    => new_resource.state_for_resource_reporter,
           "before"   => current_resource ? current_resource.state_for_resource_reporter : {},
-          "duration" => (elapsed_time * 1000).to_i.to_s,
-          "delta"    => new_resource.respond_to?(:diff) ? new_resource.diff : "",
+          "duration" => elapsed_time_in_milliseconds.to_s,
+          "delta"    => new_resource.respond_to?(:diff) && potentially_changed? ? new_resource.diff : "",
           "result"   => action.to_s,
           "status"   => status,
         }
diff --git a/spec/unit/data_collector/messages_spec.rb b/spec/unit/data_collector/messages_spec.rb
index dee86a3907..686e500507 100644
--- a/spec/unit/data_collector/messages_spec.rb
+++ b/spec/unit/data_collector/messages_spec.rb
@@ -62,12 +62,12 @@ describe Chef::DataCollector::Messages do
 
   describe '#run_end_message' do
     let(:run_status) { Chef::RunStatus.new(Chef::Node.new, Chef::EventDispatch::Dispatcher.new) }
-    let(:resource1)  { double("resource1", for_json: "resource_data", status: "updated") }
-    let(:resource2)  { double("resource2", for_json: "resource_data", status: "skipped") }
+    let(:report1)  { double("report1", report_data: { "status" => "updated" }) }
+    let(:report2)  { double("report2", report_data: { "status" => "skipped" }) }
     let(:reporter_data) do
       {
         run_status: run_status,
-        completed_resources: [resource1, resource2],
+        resources: [report1, report2],
       }
     end
 
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index 78b0aaf97d..831643347b 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -20,6 +20,7 @@
 
 require "spec_helper"
 require "chef/data_collector"
+require "chef/resource_builder"
 
 describe Chef::DataCollector do
   describe ".register_reporter?" do
@@ -193,10 +194,32 @@ describe Chef::DataCollector::Reporter do
     end
   end
 
+  describe '#converge_start' do
+    it "stashes the run_context for later use" do
+      reporter.converge_start("test_context")
+      expect(reporter.run_context).to eq("test_context")
+    end
+  end
+
+  describe '#converge_complete' do
+    it "detects and processes any unprocessed resources" do
+      expect(reporter).to receive(:detect_unprocessed_resources)
+      reporter.converge_complete
+    end
+  end
+
+  describe '#converge_failed' do
+    it "detects and processes any unprocessed resources" do
+      expect(reporter).to receive(:detect_unprocessed_resources)
+      reporter.converge_failed("exception")
+    end
+  end
+
   describe '#resource_current_state_loaded' do
     let(:new_resource)     { double("new_resource") }
     let(:action)           { double("action") }
     let(:current_resource) { double("current_resource") }
+    let(:resource_report)  { double("resource_report") }
 
     context "when resource is a nested resource" do
       it "does not update the resource report" do
@@ -207,14 +230,12 @@ describe Chef::DataCollector::Reporter do
     end
 
     context "when resource is not a nested resource" do
-      it "updates the resource report" do
+      it "creates the resource report and stores it as the current one" do
         allow(reporter).to receive(:nested_resource?).and_return(false)
-        expect(Chef::DataCollector::ResourceReport).to receive(:new).with(
-          new_resource,
-          action,
-          current_resource)
-        .and_return("resource_report")
-        expect(reporter).to receive(:update_current_resource_report).with("resource_report")
+        expect(reporter).to receive(:create_resource_report)
+          .with(new_resource, action, current_resource)
+          .and_return(resource_report)
+        expect(reporter).to receive(:update_current_resource_report).with(resource_report)
         reporter.resource_current_state_loaded(new_resource, action, current_resource)
       end
     end
@@ -256,7 +277,7 @@ describe Chef::DataCollector::Reporter do
 
     before do
       allow(reporter).to receive(:nested_resource?)
-      allow(reporter).to receive(:current_resource_report).and_return(resource_report)
+      allow(reporter).to receive(:create_resource_report).and_return(resource_report)
       allow(resource_report).to receive(:skipped)
     end
 
@@ -269,13 +290,12 @@ describe Chef::DataCollector::Reporter do
     end
 
     context "when the resource is not a nested resource" do
-      it "updates the resource report" do
+      it "creates the resource report and stores it as the current one" do
         allow(reporter).to receive(:nested_resource?).and_return(false)
-        expect(Chef::DataCollector::ResourceReport).to receive(:new).with(
-          new_resource,
-          action)
-        .and_return("resource_report")
-        expect(reporter).to receive(:update_current_resource_report).with("resource_report")
+        expect(reporter).to receive(:create_resource_report)
+          .with(new_resource, action)
+          .and_return(resource_report)
+        expect(reporter).to receive(:update_current_resource_report).with(resource_report)
         reporter.resource_skipped(new_resource, action, conditional)
       end
 
@@ -349,15 +369,16 @@ describe Chef::DataCollector::Reporter do
     let(:resource_report) { double("resource_report") }
 
     before do
-      allow(reporter).to receive(:add_completed_resource)
       allow(reporter).to receive(:update_current_resource_report)
+      allow(reporter).to receive(:add_resource_report)
+      allow(reporter).to receive(:current_resource_report)
       allow(resource_report).to receive(:finish)
     end
 
     context "when there is no current resource report" do
-      it "does not add the updated resource" do
+      it "does not touch the current resource report" do
         allow(reporter).to receive(:current_resource_report).and_return(nil)
-        expect(reporter).not_to receive(:add_completed_resource)
+        expect(reporter).not_to receive(:update_current_resource_report)
         reporter.resource_completed(new_resource)
       end
     end
@@ -368,9 +389,9 @@ describe Chef::DataCollector::Reporter do
       end
 
       context "when the resource is a nested resource" do
-        it "does not add the updated resource" do
+        it "does not mark the resource as finished" do
           allow(reporter).to receive(:nested_resource?).with(new_resource).and_return(true)
-          expect(reporter).not_to receive(:add_completed_resource)
+          expect(resource_report).not_to receive(:finish)
           reporter.resource_completed(new_resource)
         end
       end
@@ -385,11 +406,6 @@ describe Chef::DataCollector::Reporter do
           reporter.resource_completed(new_resource)
         end
 
-        it "adds the resource to the updated resource list" do
-          expect(reporter).to receive(:add_completed_resource).with(resource_report)
-          reporter.resource_completed(new_resource)
-        end
-
         it "nils out the current resource report" do
           expect(reporter).to receive(:update_current_resource_report).with(nil)
           reporter.resource_completed(new_resource)