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>

12 files changed, 289 insertions, 154 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)
diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/catalog.rb
index d74b9a388..7f3049136 100755
--- a/spec/unit/resource/catalog.rb
+++ b/spec/unit/resource/catalog.rb
@@ -578,7 +578,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
             Puppet::Transaction.stubs(:new).returns(@transaction)
             @transaction.stubs(:evaluate)
             @transaction.stubs(:cleanup)
-            @transaction.stubs(:addtimes)
+            @transaction.stubs(:add_times)
         end
 
         it "should create and evaluate a transaction" do
@@ -588,13 +588,13 @@ describe Puppet::Resource::Catalog, "when compiling" do
 
         it "should provide the catalog retrieval time to the transaction" do
             @catalog.retrieval_duration = 5
-            @transaction.expects(:addtimes).with(:config_retrieval => 5)
+            @transaction.expects(:add_times).with(:config_retrieval => 5)
             @catalog.apply
         end
 
         it "should use a retrieval time of 0 if none is set in the catalog" do
             @catalog.retrieval_duration = nil
-            @transaction.expects(:addtimes).with(:config_retrieval => 0)
+            @transaction.expects(:add_times).with(:config_retrieval => 0)
             @catalog.apply
         end
 
@@ -634,7 +634,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
             Puppet::Transaction.stubs(:new).returns(@transaction)
             @transaction.stubs(:evaluate)
             @transaction.stubs(:cleanup)
-            @transaction.stubs(:addtimes)
+            @transaction.stubs(:add_times)
             Puppet::Type.type(:file).expects(:new).with(args).returns(resource)
             resource.expects :remove
             @catalog.apply do |trans|
@@ -649,7 +649,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
             Puppet::Transaction.stubs(:new).returns(@transaction)
             @transaction.stubs(:evaluate)
             @transaction.stubs(:cleanup)
-            @transaction.stubs(:addtimes)
+            @transaction.stubs(:add_times)
             file = Puppet::Type.type(:file).new(:name => "/yay", :ensure => :file)
             @catalog.apply do |trans|
                 @catalog.add_resource file
diff --git a/spec/unit/resource/status.rb b/spec/unit/resource/status.rb
index 47f29593b..b03f9d440 100755
--- a/spec/unit/resource/status.rb
+++ b/spec/unit/resource/status.rb
@@ -10,14 +10,14 @@ describe Puppet::Resource::Status do
         @status = Puppet::Resource::Status.new(@resource)
     end
 
-    [:node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time].each do |attr|
+    [:node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time, :change_count].each do |attr|
         it "should support #{attr}" do
             @status.send(attr.to_s + "=", "foo")
             @status.send(attr).should == "foo"
         end
     end
 
-    [:skipped, :failed, :changed, :out_of_sync, :scheduled].each do |attr|
+    [:skipped, :failed, :restarted, :failed_to_restart, :changed, :out_of_sync, :scheduled].each do |attr|
         it "should support #{attr}" do
             @status.send(attr.to_s + "=", "foo")
             @status.send(attr).should == "foo"
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index 3fcbccac4..f0c98db8e 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -15,6 +15,14 @@ describe Puppet::Transaction do
         @transaction.events.should == %w{my events}
     end
 
+    it "should delegate adding times to its report" do
+        @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
+        @transaction.report.expects(:add_times).with(:foo, 10)
+        @transaction.report.expects(:add_times).with(:bar, 20)
+
+        @transaction.add_times :foo => 10, :bar => 20
+    end
+
     it "should be able to accept resource status instances" do
         resource = Puppet::Type.type(:notify).new :title => "foobar"
         status = Puppet::Resource::Status.new(resource)
@@ -29,6 +37,12 @@ describe Puppet::Transaction do
         @transaction.resource_status(resource.to_s).should equal(status)
     end
 
+    # This will basically only ever be used during testing.
+    it "should automatically create resource statuses if asked for a non-existent status" do
+        resource = Puppet::Type.type(:notify).new :title => "foobar"
+        @transaction.resource_status(resource).should be_instance_of(Puppet::Resource::Status)
+    end
+
     it "should add provided resource statuses to its report" do
         resource = Puppet::Type.type(:notify).new :title => "foobar"
         status = Puppet::Resource::Status.new(resource)
@@ -36,8 +50,9 @@ describe Puppet::Transaction do
         @transaction.report.resource_statuses[resource.to_s].should equal(status)
     end
 
-    it "should return nil when asked for a status that has not been created" do
-        @transaction.resource_status("File[/foo]").should be_nil
+    it "should calculate metrics on and report the report when asked to generate a report" do
+        @transaction.report.expects(:calculate_metrics)
+        @transaction.generate_report.should equal(@transaction.report)
     end
 
     it "should consider a resource to be failed if a status instance exists for that resource and indicates it is failed" do
@@ -48,7 +63,29 @@ describe Puppet::Transaction do
         @transaction.should be_failed(resource)
     end
 
-   it "should consider a resource to have failed dependencies if any of its dependencies are failed"
+    it "should not consider a resource to be failed if a status instance exists for that resource but indicates it is not failed" do
+        resource = Puppet::Type.type(:notify).new :name => "yayness"
+        status = Puppet::Resource::Status.new(resource)
+        @transaction.add_resource_status(status)
+        @transaction.should_not be_failed(resource)
+    end
+
+    it "should consider there to be failed resources if any statuses are marked failed" do
+        resource = Puppet::Type.type(:notify).new :name => "yayness"
+        status = Puppet::Resource::Status.new(resource)
+        status.failed = "some message"
+        @transaction.add_resource_status(status)
+        @transaction.should be_any_failed
+    end
+    
+    it "should not consider there to be failed resources if no statuses are marked failed" do
+        resource = Puppet::Type.type(:notify).new :name => "yayness"
+        status = Puppet::Resource::Status.new(resource)
+        @transaction.add_resource_status(status)
+        @transaction.should_not be_any_failed
+    end
+
+    it "should consider a resource to have failed dependencies if any of its dependencies are failed"
 
     describe "when initializing" do
         it "should create an event manager" do
@@ -70,7 +107,7 @@ describe Puppet::Transaction do
             @transaction.stubs(:eval_children_and_apply_resource)
             @transaction.stubs(:skip?).returns false
 
-            @resource = stub("resource")
+            @resource = Puppet::Type.type(:file).new :path => "/my/file"
         end
 
         it "should check whether the resource should be skipped" do
@@ -96,9 +133,9 @@ describe Puppet::Transaction do
                 @transaction.expects(:skip?).with(@resource).returns true
             end
 
-            it "should increment the 'skipped' count" do
+            it "should mark the resource's status as skipped" do
                 @transaction.eval_resource(@resource)
-                @transaction.resourcemetrics[:skipped].should == 1
+                @transaction.resource_status(@resource).should be_skipped
             end
         end
     end
@@ -134,7 +171,7 @@ describe Puppet::Transaction do
             @transaction.resource_harness.expects(:evaluate).raises ArgumentError
             @resource.expects(:err)
             @transaction.apply(@resource)
-            @transaction.resource_status(@resource).should be_nil
+            @transaction.report.resource_statuses[@resource.to_s].should be_nil
         end
     end
 
@@ -226,22 +263,6 @@ describe Puppet::Transaction do
         end
     end
 
-    describe "when adding metrics to a report" do
-        before do
-            @catalog = Puppet::Resource::Catalog.new
-            @transaction = Puppet::Transaction.new(@catalog)
-
-            @report = stub 'report', :newmetric => nil, :time= => nil
-        end
-
-        [:resources, :time, :changes].each do |metric|
-            it "should add times for '#{metric}'" do
-                @report.expects(:newmetric).with { |m, v| m == metric }
-                @transaction.add_metrics_to_report(@report)
-            end
-        end
-    end
-
     describe "when prefetching" do
         it "should match resources by name, not title" do
             @catalog = Puppet::Resource::Catalog.new
@@ -257,26 +278,21 @@ describe Puppet::Transaction do
         end
     end
 
-    describe "when determining changed resources" do
-        before :each do
-            @catalog = Puppet::Resource::Catalog.new
-            @transaction = Puppet::Transaction.new(@catalog)
+    it "should return all resources for which the resource status indicates the resource has changed when determinig changed resources" do
+        @catalog = Puppet::Resource::Catalog.new
+        @transaction = Puppet::Transaction.new(@catalog)
+        names = []
+        2.times do |i|
+            name = "/my/file#{i}"
+            resource = Puppet::Type.type(:file).new :path => name
+            names << resource.to_s
+            @catalog.add_resource resource
+            @transaction.add_resource_status Puppet::Resource::Status.new(resource)
         end
 
-        it "should return all resources for which the resource status indicates the resource has changed" do
-            names = []
-            2.times do |i|
-                name = "/my/file#{i}"
-                resource = Puppet::Type.type(:file).new :path => name
-                names << resource.to_s
-                @catalog.add_resource resource
-                @transaction.add_resource_status Puppet::Resource::Status.new(resource)
-            end
+        @transaction.resource_status(names[0]).changed = true
 
-            @transaction.resource_status(names[0]).changed = true
-
-            @transaction.changed?.should == [@catalog.resource(names[0])]
-        end
+        @transaction.changed?.should == [@catalog.resource(names[0])]
     end
 end
 
diff --git a/spec/unit/transaction/event_manager.rb b/spec/unit/transaction/event_manager.rb
index 7d8fb8afd..12407c3cc 100755
--- a/spec/unit/transaction/event_manager.rb
+++ b/spec/unit/transaction/event_manager.rb
@@ -24,7 +24,7 @@ describe Puppet::Transaction::EventManager do
         before do
             @manager = Puppet::Transaction::EventManager.new(@transaction)
 
-            @resource = stub("resource", :self_refresh? => false, :deleting => false)
+            @resource = Puppet::Type.type(:file).new :path => "/my/file"
 
             @graph = stub 'graph', :matching_edges => [], :resource => @resource
             @manager.stubs(:relationship_graph).returns @graph
@@ -138,7 +138,7 @@ describe Puppet::Transaction::EventManager do
             @manager = Puppet::Transaction::EventManager.new(@transaction)
             @manager.stubs(:queue_event)
 
-            @resource = stub 'resource', :notice => nil, :event => @event
+            @resource = Puppet::Type.type(:file).new :path => "/my/file"
             @event = Puppet::Transaction::Event.new(:name => :event, :resource => @resource)
         end
 
@@ -151,14 +151,14 @@ describe Puppet::Transaction::EventManager do
             @manager.process_events(@resource)
         end
 
-        it "should update the 'restarted' metric" do
+        it "should set the 'restarted' state on the resource status" do
             @manager.expects(:queued_events).with(@resource).yields(:callback1, [@event])
 
             @resource.stubs(:callback1)
 
             @manager.process_events(@resource)
 
-            @transaction.resourcemetrics[:restarted].should == 1
+            @transaction.resource_status(@resource).should be_restarted
         end
 
         it "should queue a 'restarted' event generated by the resource" do
@@ -239,9 +239,9 @@ describe Puppet::Transaction::EventManager do
                 lambda { @manager.process_events(@resource) }.should_not raise_error
             end
 
-            it "should update the 'failed_restarts' metric" do
+            it "should set the 'failed_restarts' state on the resource status" do
                 @manager.process_events(@resource)
-                @transaction.resourcemetrics[:failed_restarts].should == 1
+                @transaction.resource_status(@resource).should be_failed_to_restart
             end
 
             it "should not queue a 'restarted' event" do
@@ -249,9 +249,9 @@ describe Puppet::Transaction::EventManager do
                 @manager.process_events(@resource)
             end
 
-            it "should not increase the restarted resource count" do
+            it "should set the 'restarted' state on the resource status" do
                 @manager.process_events(@resource)
-                @transaction.resourcemetrics[:restarted].should == 0
+                @transaction.resource_status(@resource).should_not be_restarted
             end
         end
     end
diff --git a/spec/unit/transaction/report.rb b/spec/unit/transaction/report.rb
index 8476444fd..10960de60 100755
--- a/spec/unit/transaction/report.rb
+++ b/spec/unit/transaction/report.rb
@@ -83,24 +83,131 @@ describe Puppet::Transaction::Report do
     describe "when computing exit status" do
         it "should produce 2 if changes are present" do
             report = Puppet::Transaction::Report.new
-            report.newmetric("changes", {:total => 1})
-            report.newmetric("resources", {:failed => 0})
+            report.add_metric("changes", {:total => 1})
+            report.add_metric("resources", {:failed => 0})
             report.exit_status.should == 2
         end
 
         it "should produce 4 if failures are present" do
             report = Puppet::Transaction::Report.new
-            report.newmetric("changes", {:total => 0})
-            report.newmetric("resources", {:failed => 1})
+            report.add_metric("changes", {:total => 0})
+            report.add_metric("resources", {:failed => 1})
             report.exit_status.should == 4
         end
 
         it "should produce 6 if both changes and failures are present" do
             report = Puppet::Transaction::Report.new
-            report.newmetric("changes", {:total => 1})
-            report.newmetric("resources", {:failed => 1})
+            report.add_metric("changes", {:total => 1})
+            report.add_metric("resources", {:failed => 1})
             report.exit_status.should == 6
         end
     end
+
+    describe "when calculating metrics" do
+        before do
+            @report = Puppet::Transaction::Report.new
+        end
+
+        def metric(name, value)
+            if metric = @report.metrics[name.to_s]
+                metric[value]
+            else
+                nil
+            end
+        end
+
+        def add_statuses(count, type = :file)
+            3.times do |i|
+                status = Puppet::Resource::Status.new(Puppet::Type.type(type).new(:title => "/my/path#{i}"))
+                yield status if block_given?
+                @report.add_resource_status status
+            end
+        end
+
+
+        [:time, :resources, :changes, :events].each do |type|
+            it "should add #{type} metrics" do
+                @report.calculate_metrics
+                @report.metrics[type.to_s].should be_instance_of(Puppet::Transaction::Metric)
+            end
+        end
+
+        describe "for resources" do
+            it "should provide the total number of resources" do
+                add_statuses(3)
+
+                @report.calculate_metrics
+                metric(:resources, :total).should == 3
+            end
+
+            Puppet::Resource::Status::STATES.each do |state|
+                it "should provide the number of #{state} resources as determined by the status objects" do
+                    add_statuses(3) { |status| status.send(state.to_s + "=", true) }
+
+                    @report.calculate_metrics
+                    metric(:resources, state).should == 3
+                end
+            end
+        end
+
+        describe "for changes" do
+            it "should provide the number of changes from the resource statuses" do
+                add_statuses(3) { |status| status.change_count = 3 }
+                @report.calculate_metrics
+                metric(:changes, :total).should == 9
+            end
+        end
+
+        describe "for times" do
+            it "should provide the total amount of time for each resource type" do
+                add_statuses(3, :file) do |status|
+                    status.evaluation_time = 1 
+                end
+                add_statuses(3, :exec) do |status|
+                    status.evaluation_time = 2 
+                end
+                add_statuses(3, :mount) do |status|
+                    status.evaluation_time = 3 
+                end
+
+                @report.calculate_metrics
+
+                metric(:time, "file").should == 3
+                metric(:time, "exec").should == 6
+                metric(:time, "mount").should == 9
+            end
+
+            it "should add any provided times from external sources" do
+                @report.add_times :foobar, 50
+                @report.calculate_metrics
+                metric(:time, "foobar").should == 50
+            end
+        end
+
+        describe "for events" do
+            it "should provide the total number of events" do
+                add_statuses(3) do |status|
+                    3.times { |i| status.add_event(Puppet::Transaction::Event.new) }
+                end
+                @report.calculate_metrics
+                metric(:events, :total).should == 9
+            end
+
+            Puppet::Transaction::Event::EVENT_STATUSES.each do |status_name|
+                it "should provide the number of #{status_name} events" do
+                    add_statuses(3) do |status|
+                        3.times do |i|
+                            event = Puppet::Transaction::Event.new
+                            event.status = status_name
+                            status.add_event(event)
+                        end
+                    end
+
+                    @report.calculate_metrics
+                    metric(:events, status_name).should == 9
+                end
+            end
+        end
+    end
 end
 
diff --git a/spec/unit/transaction/resource_harness.rb b/spec/unit/transaction/resource_harness.rb
index 2051b2c79..f69b560f8 100755
--- a/spec/unit/transaction/resource_harness.rb
+++ b/spec/unit/transaction/resource_harness.rb
@@ -84,6 +84,17 @@ describe Puppet::Transaction::ResourceHarness do
             @harness.expects(:apply_changes).never
             @harness.evaluate(@resource).should_not be_out_of_sync
         end
+
+        it "should store the resource's evaluation time in the resource status" do
+            @harness.evaluate(@resource).evaluation_time.should be_instance_of(Float)
+        end
+
+        it "should set the change count to the total number of changes" do
+            changes = %w{a b c d}
+            @harness.expects(:changes_to_perform).returns changes
+            @harness.expects(:apply_changes).with(@status, changes)
+            @harness.evaluate(@resource).change_count.should == 4
+        end
     end
 
     describe "when creating changes" do