Merge pull request #5058 from chef/adamleff/IPO-196/add-unprocessed-resources-to-data-collector

Expand data_collector resource list to include all resources

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)