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>
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() {
diff --git a/payload_state.h b/payload_state.h
index d0211c5..9ad4377 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -111,6 +111,11 @@
}
private:
+ enum class AttemptType {
+ kUpdate,
+ kRollback,
+ };
+
friend class PayloadStateTest;
FRIEND_TEST(PayloadStateTest, RebootAfterUpdateFailedMetric);
FRIEND_TEST(PayloadStateTest, RebootAfterUpdateSucceed);
@@ -119,8 +124,8 @@
FRIEND_TEST(PayloadStateTest, UpdateSuccessWithWipedPrefs);
// Helper called when an attempt has begun, is called by
- // UpdateResumed() and UpdateRestarted().
- void AttemptStarted();
+ // UpdateResumed(), UpdateRestarted() and Rollback().
+ void AttemptStarted(AttemptType attempt_type);
// Increments the payload attempt number used for metrics.
void IncrementPayloadAttemptNumber();
@@ -506,6 +511,9 @@
// The connection type when the attempt started.
metrics::ConnectionType attempt_connection_type_;
+ // Whether we're currently rolling back.
+ AttemptType attempt_type_;
+
DISALLOW_COPY_AND_ASSIGN(PayloadState);
};
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index 20d321c..11eb4af 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -1080,6 +1080,13 @@
rollback_version));
payload_state.LoadRollbackVersion();
EXPECT_EQ(rollback_version, payload_state.GetRollbackVersion());
+
+ // Check that we don't report any metrics in UpdateSucceeded().
+ EXPECT_CALL(*fake_system_state.mock_metrics_lib(), SendToUMA(_, _, _, _, _))
+ .Times(0);
+ EXPECT_CALL(*fake_system_state.mock_metrics_lib(), SendEnumToUMA(_, _, _))
+ .Times(0);
+ payload_state.UpdateSucceeded();
}
TEST(PayloadStateTest, DurationsAreCorrect) {