commit afed4a1a355c8416d496901234932e99864d6215
author David Zeuthen <zeuthen@chromium.org> Wed Apr 09 15:28:44 2014 -0700
committer chrome-internal-fetch <chrome-internal-fetch@google.com> Tue Apr 15 21:45:28 2014 +0000
tree 9d2841a9c8bcf43d6d5618aff862038d0bf27073
parent 5bb4c90b8bdf931426d1926b21b0316a86b4b4e4

Don't report metrics when rolling back.

While reviewing histograms for the new metrics, some of the values for
the UpdateEngine.Attempt.* metrics had bogus values. For example, the
DownloadSource and ConnectionType metrics had values in their overflow
buckets. This is weird as the code carefully tries to ensure that
values outside the respective enum classes are never used.

Here's how this can happen: If we're rolling back, the UpdateAttempter
class calls PayloadState::Rollback() instead of PayloadState::UpdateResumed()
or PayloadState::UpdateRestarted().

Crucially, PayloadState::Rollback() never calls the AttemptStarted()
method so the attempt_*_ member variables are left uninitialized.

Then later on UpdateAttempter::ProcessingDone() calls
PayloadState::UpdateSucceeded() or PayloadState::UpdateFailed() which
ends up calling PayloadState::CollectAndReportAttemptMetrics() and we
report the uninitialized attempt_*_ members.

This CL fixes this problem by making PayloadState::Rollback() call
AttemptStarted() and then keep track of whether it's a rollback or
not.  In the affirmative we don't report metrics. There's also a unit
test to verify this.

Additionally, this CL also fixes the oversight that the attempt_*_
members were not initialized in the constructor, per policy.

It would probably be better if rollback was implemented in another way
(so it didn't trigger codepaths like this) but that's not how it was
done. We should probably also think about reporting metrics specific
to rollback.

BUG=chromium:355745
TEST=New unit test + Unit tests pass.

Change-Id: Id2e606f02797714520290c4cbe34a056ccdae053
Reviewed-on: https://chromium-review.googlesource.com/193950
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>

payload_state.cc

diff --git a/payload_state.cc b/payload_state.cc
index 51041ef..4c8efa0 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -45,7 +45,11 @@
       url_index_(0),
       url_failure_count_(0),
       url_switch_count_(0),
-      p2p_num_attempts_(0) {
+      p2p_num_attempts_(0),
+      attempt_num_bytes_downloaded_(0),
+      attempt_connection_type_(metrics::ConnectionType::kUnknown),
+      attempt_type_(AttemptType::kUpdate)
+{
  for (int i = 0; i <= kNumDownloadSources; i++)
   total_bytes_downloaded_[i] = current_bytes_downloaded_[i] = 0;
 }
@@ -157,7 +161,9 @@
   SetUrlFailureCount(0);
 }
 
-void PayloadState::AttemptStarted() {
+void PayloadState::AttemptStarted(AttemptType attempt_type) {
+  attempt_type_ = attempt_type;
+
   ClockInterface *clock = system_state_->clock();
   attempt_start_time_boot_ = clock->GetBootTime();
   attempt_start_time_monotonic_ = clock->GetMonotonicTime();
@@ -182,14 +188,14 @@
 void PayloadState::UpdateResumed() {
   LOG(INFO) << "Resuming an update that was previously started.";
   UpdateNumReboots();
-  AttemptStarted();
+  AttemptStarted(AttemptType::kUpdate);
 }
 
 void PayloadState::UpdateRestarted() {
   LOG(INFO) << "Starting a new update";
   ResetDownloadSourcesOnNewUpdate();
   SetNumReboots(0);
-  AttemptStarted();
+  AttemptStarted(AttemptType::kUpdate);
 }
 
 void PayloadState::UpdateSucceeded() {
@@ -197,8 +203,11 @@
   CalculateUpdateDurationUptime();
   SetUpdateTimestampEnd(system_state_->clock()->GetWallclockTime());
 
-  CollectAndReportAttemptMetrics(kErrorCodeSuccess);
-  CollectAndReportSuccessfulUpdateMetrics();
+  // Only report metrics on an update, not on a rollback.
+  if (attempt_type_ == AttemptType::kUpdate) {
+    CollectAndReportAttemptMetrics(kErrorCodeSuccess);
+    CollectAndReportSuccessfulUpdateMetrics();
+  }
 
   // Reset the number of responses seen since it counts from the last
   // successful update, e.g. now.
@@ -220,7 +229,9 @@
     return;
   }
 
-  CollectAndReportAttemptMetrics(base_error);
+  // Only report metrics on an update, not on a rollback.
+  if (attempt_type_ == AttemptType::kUpdate)
+    CollectAndReportAttemptMetrics(base_error);
 
   switch (base_error) {
     // Errors which are good indicators of a problem with a particular URL or
@@ -371,6 +382,7 @@
 
 void PayloadState::Rollback() {
   SetRollbackVersion(system_state_->request_params()->app_version());
+  AttemptStarted(AttemptType::kRollback);
 }
 
 void PayloadState::IncrementPayloadAttemptNumber() {