Moving Metric management to the reports

This is one less bit that the transaction does.
The resource status objects had nearly enough information
to do everything, so I just added that last bit, and moved
everything over.  It's all much cleaner now.

I had to change some existing, internal APIs, but mostly
this should be hidden from outside users.

Signed-off-by: Luke Kanies <luke@reductivelabs.com>

6 files changed, 94 insertions, 93 deletions

diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index cb6128517..c5ae8f52d 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -135,8 +135,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
         transaction.tags = options[:tags] if options[:tags]
         transaction.ignoreschedules = true if options[:ignoreschedules]
 
-        transaction.addtimes :config_retrieval => self.retrieval_duration || 0
-
+        transaction.add_times :config_retrieval => self.retrieval_duration || 0
 
         begin
             transaction.evaluate
diff --git a/lib/puppet/resource/status.rb b/lib/puppet/resource/status.rb
index 0ca295d4d..ab088fb19 100644
--- a/lib/puppet/resource/status.rb
+++ b/lib/puppet/resource/status.rb
@@ -2,10 +2,10 @@ class Puppet::Resource::Status
     include Puppet::Util::Tagging
     include Puppet::Util::Logging
 
-    ATTRIBUTES = [:resource, :node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time]
+    ATTRIBUTES = [:resource, :node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time, :change_count]
     attr_accessor *ATTRIBUTES
 
-    STATES = [:skipped, :failed, :changed, :out_of_sync, :scheduled]
+    STATES = [:skipped, :failed, :failed_to_restart, :restarted, :changed, :out_of_sync, :scheduled]
     attr_accessor *STATES
 
     attr_reader :source_description, :default_log_level, :time, :resource
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index f8c7a859a..1b0c470e1 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -17,9 +17,6 @@ class Puppet::Transaction
     # The report, once generated.
     attr_reader :report
 
-    # Mostly only used for tests
-    attr_reader :resourcemetrics, :changes
-
     # Routes and stores any events and subscriptions.
     attr_reader :event_manager
 
@@ -30,37 +27,15 @@ class Puppet::Transaction
     include Puppet::Util::Tagging
 
     # Add some additional times for reporting
-    def addtimes(hash)
+    def add_times(hash)
         hash.each do |name, num|
-            @timemetrics[name] = num
+            report.add_times(name, num)
         end
     end
 
-    # Check to see if we should actually allow processing, but this really only
-    # matters when a resource is getting deleted.
-    def allow_processing?(resource, changes)
-        # If a resource is going to be deleted but it still has
-        # dependencies, then don't delete it unless it's implicit or the
-        # dependency is itself being deleted.
-        if resource.purging? and resource.deleting?
-            if deps = relationship_graph.dependents(resource) and ! deps.empty? and deps.detect { |d| ! d.deleting? }
-                resource.warning "%s still depend%s on me -- not purging" %
-                    [deps.collect { |r| r.ref }.join(","), deps.length > 1 ? "":"s"]
-                return false
-            end
-        end
-
-        return true
-    end
-
     # Are there any failed resources in this transaction?
     def any_failed?
-        failures = @failures.inject(0) { |failures, array| failures += array[1]; failures }
-        if failures > 0
-            failures
-        else
-            false
-        end
+        report.resource_statuses.values.detect { |status| status.failed? }
     end
 
     # Apply all changes for a resource
@@ -116,7 +91,7 @@ class Puppet::Transaction
     # Evaluate a single resource.
     def eval_resource(resource)
         if skip?(resource)
-            @resourcemetrics[:skipped] += 1
+            resource_status(resource).skipped = true
             return
         end
 
@@ -127,7 +102,7 @@ class Puppet::Transaction
     end
 
     def eval_children_and_apply_resource(resource)
-        @resourcemetrics[:scheduled] += 1
+        resource_status(resource).scheduled = true
 
         # We need to generate first regardless, because the recursive
         # actions sometimes change how the top resource is applied.
@@ -141,18 +116,13 @@ class Puppet::Transaction
         end
 
         # Perform the actual changes
-        seconds = thinmark do
-            apply(resource)
-        end
+        apply(resource)
 
         if ! children.empty? and ! resource.depthfirst?
             children.each do |child|
                 eval_resource(child)
             end
         end
-
-        # Keep track of how long we spend in each type of resource
-        @timemetrics[resource.class.name] += seconds
     end
 
     # This method does all the actual work of running a transaction.  It
@@ -255,26 +225,10 @@ class Puppet::Transaction
         end
     end
 
-    def add_metrics_to_report(report)
-        @resourcemetrics[:failed] = @failures.find_all do |name, num|
-            num > 0
-        end.length
-
-        # Get the total time spent
-        @timemetrics[:total] = @timemetrics.inject(0) do |total, vals|
-            total += vals[1]
-            total
-        end
-
-        # Add all of the metrics related to resource count and status
-        report.newmetric(:resources, @resourcemetrics)
-
-        # Record the relative time spent in each resource.
-        report.newmetric(:time, @timemetrics)
-
-        # Then all of the change-related metrics
-        report.newmetric(:changes, :total => @changes.length)
-        return report
+    # Generate a transaction report.
+    def generate_report
+        @report.calculate_metrics
+        return @report
     end
 
     # Should we ignore tags?
@@ -287,25 +241,6 @@ class Puppet::Transaction
     def initialize(catalog)
         @catalog = catalog
 
-        @resourcemetrics = {
-            :total => @catalog.vertices.length,
-            :out_of_sync => 0,    # The number of resources that had changes
-            :applied => 0,        # The number of resources fixed
-            :skipped => 0,      # The number of resources skipped
-            :restarted => 0,    # The number of resources triggered
-            :failed_restarts => 0, # The number of resources that fail a trigger
-            :scheduled => 0     # The number of resources scheduled
-        }
-
-        # Metrics for distributing times across the different types.
-        @timemetrics = Hash.new(0)
-
-        # The resources that have failed and the number of failures each.  This
-        # is used for skipping resources because of failed dependencies.
-        @failures = Hash.new do |h, key|
-            h[key] = 0
-        end
-
         @report = Report.new
 
         @event_manager = Puppet::Transaction::EventManager.new(self)
@@ -382,7 +317,7 @@ class Puppet::Transaction
     end
 
     def resource_status(resource)
-        report.resource_statuses[resource.to_s]
+        report.resource_statuses[resource.to_s] || add_resource_status(Puppet::Resource::Status.new(resource))
     end
 
     # Is the resource currently scheduled?
diff --git a/lib/puppet/transaction/event_manager.rb b/lib/puppet/transaction/event_manager.rb
index a6ca8a229..aacd17a95 100644
--- a/lib/puppet/transaction/event_manager.rb
+++ b/lib/puppet/transaction/event_manager.rb
@@ -24,7 +24,7 @@ class Puppet::Transaction::EventManager
         if restarted
             queue_event(resource, resource.event(:name => :restarted, :status => "success"))
 
-            transaction.resourcemetrics[:restarted] += 1
+            transaction.resource_status(resource).restarted = true
         end
     end
 
@@ -74,7 +74,7 @@ class Puppet::Transaction::EventManager
     rescue => detail
         resource.err "Failed to call #{callback}: #{detail}"
 
-        transaction.resourcemetrics[:failed_restarts] += 1
+        transaction.resource_status(resource).failed_to_restart = true
         puts detail.backtrace if Puppet[:trace]
         return false
     end
diff --git a/lib/puppet/transaction/report.rb b/lib/puppet/transaction/report.rb
index 787ef19ad..3227730eb 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -23,14 +23,37 @@ class Puppet::Transaction::Report
         return self
     end
 
+    def add_times(name, value)
+        @external_times[name] = value
+    end
+
+    def add_metric(name, hash)
+        metric = Puppet::Util::Metric.new(name)
+
+        hash.each do |name, value|
+            metric.newvalue(name, value)
+        end
+
+        @metrics[metric.name] = metric
+        metric
+    end
+
     def add_resource_status(status)
         @resource_statuses[status.resource] = status
     end
 
+    def calculate_metrics
+        calculate_resource_metrics
+        calculate_time_metrics
+        calculate_change_metrics
+        calculate_event_metrics
+    end
+
     def initialize
         @metrics = {}
         @logs = []
         @resource_statuses = {}
+        @external_times ||= {}
         @host = Puppet[:certname]
         @time = Time.now
     end
@@ -39,17 +62,6 @@ class Puppet::Transaction::Report
         host
     end
 
-    # Create a new metric.
-    def newmetric(name, hash)
-        metric = Puppet::Util::Metric.new(name)
-
-        hash.each do |name, value|
-            metric.newvalue(name, value)
-        end
-
-        @metrics[metric.name] = metric
-    end
-
     # Provide a summary of this report.
     def summary
         ret = ""
@@ -85,4 +97,55 @@ class Puppet::Transaction::Report
         status |= 4 if @metrics["resources"][:failed] > 0
         return status
     end
+
+    private
+
+    def calculate_change_metrics
+        metrics = Hash.new(0)
+        resource_statuses.each do |name, status|
+            metrics[:total] += status.change_count if status.change_count
+        end
+
+        add_metric(:changes, metrics)
+    end
+
+    def calculate_event_metrics
+        metrics = Hash.new(0)
+        resource_statuses.each do |name, status|
+            metrics[:total] += status.events.length
+            status.events.each do |event|
+                metrics[event.status] += 1
+            end
+        end
+
+        add_metric(:events, metrics)
+    end
+
+    def calculate_resource_metrics
+        metrics = Hash.new(0)
+        metrics[:total] = resource_statuses.length
+
+        resource_statuses.each do |name, status|
+
+            Puppet::Resource::Status::STATES.each do |state|
+                metrics[state] += 1 if status.send(state)
+            end
+        end
+
+        add_metric(:resources, metrics)
+    end
+
+    def calculate_time_metrics
+        metrics = Hash.new(0)
+        resource_statuses.each do |name, status|
+            type = Puppet::Resource::Reference.new(name).type
+            metrics[type.to_s.downcase] += status.evaluation_time if status.evaluation_time
+        end
+
+        @external_times.each do |name, value|
+            metrics[name.to_s.downcase] = value
+        end
+
+        add_metric(:time, metrics)
+    end
 end
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index a7784f344..081e3b10e 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -44,10 +44,12 @@ class Puppet::Transaction::ResourceHarness
     end
 
     def evaluate(resource)
+        start = Time.now
         status = Puppet::Resource::Status.new(resource)
 
         if changes = changes_to_perform(status, resource) and ! changes.empty?
             status.out_of_sync = true
+            status.change_count = changes.length
             apply_changes(status, changes)
             resource.cache(:synced, Time.now)
             resource.flush if resource.respond_to?(:flush)
@@ -59,6 +61,8 @@ class Puppet::Transaction::ResourceHarness
         resource.err "Could not evaluate: #{detail}"
         status.failed = true
         return status
+    ensure
+        (status.evaluation_time = Time.now - start) if status
     end
 
     def initialize(transaction)