Add Installer.UpdatesAbandonedCount metric
This patch adds a new metric Installer.UpdatesAbandonedCount to track
the number of update attempts that didn't complete because a newer
update was detected during the download. This is implemented by
counting the number of different responses seen since the last
successful update.
Updates are typically only abandoned if a device is suspended or
powered off while an update is happening. This can happen either
because the device was not turned on for a very long time or because
it had little or no connectivity to Omaha and/or the servers serving
the payload.
This metric will help show how many users run into this problem.
BUG=chromium:248800
TEST=New units tests + Unit tests pass + Manually tested
Change-Id: I524a380a931c2fb30916d033b7e5b0c700f57103
Reviewed-on: https://gerrit.chromium.org/gerrit/59098
Reviewed-by: Chris Sosa <sosa@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
diff --git a/constants.cc b/constants.cc
index 3609a02..0f425d7 100644
--- a/constants.cc
+++ b/constants.cc
@@ -30,6 +30,7 @@
const char kPrefsLastRollCallPingDay[] = "last-roll-call-ping-day";
const char kPrefsManifestMetadataSize[] = "manifest-metadata-size";
const char kPrefsNumReboots[] = "num-reboots";
+const char kPrefsNumResponsesSeen[] = "num-responses-seen";
const char kPrefsPayloadAttemptNumber[] = "payload-attempt-number";
const char kPrefsPreviousVersion[] = "previous-version";
const char kPrefsResumedUpdateFailures[] = "resumed-update-failures";
diff --git a/constants.h b/constants.h
index e97ec90..4bd9685 100644
--- a/constants.h
+++ b/constants.h
@@ -33,6 +33,7 @@
extern const char kPrefsLastRollCallPingDay[];
extern const char kPrefsManifestMetadataSize[];
extern const char kPrefsNumReboots[];
+extern const char kPrefsNumResponsesSeen[];
extern const char kPrefsPayloadAttemptNumber[];
extern const char kPrefsPreviousVersion[];
extern const char kPrefsResumedUpdateFailures[];
diff --git a/mock_payload_state.h b/mock_payload_state.h
index 6513a83..8d1edf5 100644
--- a/mock_payload_state.h
+++ b/mock_payload_state.h
@@ -33,6 +33,7 @@
MOCK_METHOD0(GetCurrentUrl, std::string());
MOCK_METHOD0(GetUrlFailureCount, uint32_t());
MOCK_METHOD0(GetUrlSwitchCount, uint32_t());
+ MOCK_METHOD0(GetNumResponsesSeen, int());
MOCK_METHOD0(GetBackoffExpiryTime, base::Time());
MOCK_METHOD0(GetUpdateDuration, base::TimeDelta());
MOCK_METHOD0(GetUpdateDurationUptime, base::TimeDelta());
diff --git a/payload_state.cc b/payload_state.cc
index 0636f56..b804822 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -60,6 +60,7 @@
LoadTotalBytesDownloaded(source);
}
LoadNumReboots();
+ LoadNumResponsesSeen();
return true;
}
@@ -82,6 +83,7 @@
// clear away all the existing state.
if (has_response_changed) {
LOG(INFO) << "Resetting all persisted state as this is a new response";
+ SetNumResponsesSeen(num_responses_seen_ + 1);
SetResponseSignature(new_response_signature);
ResetPersistedState();
return;
@@ -151,6 +153,11 @@
ReportUpdateUrlSwitchesMetric();
ReportRebootMetrics();
ReportDurationMetrics();
+ ReportUpdatesAbandonedCountMetric();
+
+ // Reset the number of responses seen since it counts from the last
+ // successful update, e.g. now.
+ SetNumResponsesSeen(0);
}
void PayloadState::UpdateFailed(ErrorCode error) {
@@ -883,6 +890,30 @@
<< GetTotalBytesDownloaded(source);
}
+void PayloadState::LoadNumResponsesSeen() {
+ SetNumResponsesSeen(GetPersistedValue(kPrefsNumResponsesSeen));
+}
+
+void PayloadState::SetNumResponsesSeen(int num_responses_seen) {
+ CHECK(prefs_);
+ num_responses_seen_ = num_responses_seen;
+ LOG(INFO) << "Num Responses Seen = " << num_responses_seen_;
+ prefs_->SetInt64(kPrefsNumResponsesSeen, num_responses_seen_);
+}
+
+void PayloadState::ReportUpdatesAbandonedCountMetric() {
+ string metric = "Installer.UpdatesAbandonedCount";
+ int value = num_responses_seen_ - 1;
+
+ LOG(INFO) << "Uploading " << value << " (count) for metric " << metric;
+ system_state_->metrics_lib()->SendToUMA(
+ metric,
+ value,
+ 0, // min value
+ 100, // max value
+ kNumDefaultUmaBuckets);
+}
+
void PayloadState::ComputeCandidateUrls() {
bool http_url_ok = false;
diff --git a/payload_state.h b/payload_state.h
index e28d5e8..f8a2d0c 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -61,6 +61,10 @@
return url_switch_count_;
}
+ virtual inline int GetNumResponsesSeen() {
+ return num_responses_seen_;
+ }
+
virtual inline base::Time GetBackoffExpiryTime() {
return backoff_expiry_time_;
}
@@ -251,6 +255,16 @@
// the Omaha response.
void ComputeCandidateUrls();
+ // Sets |num_responses_seen_| and persist it to disk.
+ void SetNumResponsesSeen(int num_responses_seen);
+
+ // Initializes |num_responses_seen_| from persisted state.
+ void LoadNumResponsesSeen();
+
+ // Reports metric conveying how many times updates were abandoned
+ // before an update was applied.
+ void ReportUpdatesAbandonedCountMetric();
+
// The global state of the system.
SystemState* system_state_;
@@ -307,6 +321,11 @@
// data we read from the socket.
DownloadSource current_download_source_;
+ // The number of different Omaha responses seen. Increases every time
+ // a new response is seen. Resets to 0 only when the system has been
+ // successfully updated.
+ int num_responses_seen_;
+
// The number of system reboots during an update attempt. Technically since
// we don't go out of our way to not update it when not attempting an update,
// also records the number of reboots before the next update attempt starts.
diff --git a/payload_state_interface.h b/payload_state_interface.h
index cd49abf..0cfb969 100644
--- a/payload_state_interface.h
+++ b/payload_state_interface.h
@@ -79,6 +79,10 @@
// for the current response.
virtual uint32_t GetUrlSwitchCount() = 0;
+ // Returns the total number of different responses seen since the
+ // last successful update.
+ virtual int GetNumResponsesSeen() = 0;
+
// Returns the expiry time for the current backoff period.
virtual base::Time GetBackoffExpiryTime() = 0;
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index 77a8d02..13052ef 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -130,6 +130,7 @@
EXPECT_EQ("", payload_state.GetCurrentUrl());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
+ EXPECT_EQ(1, payload_state.GetNumResponsesSeen());
}
TEST(PayloadStateTest, SetResponseWorksWithSingleUrl) {
@@ -177,6 +178,7 @@
EXPECT_EQ("https://single.url.test", payload_state.GetCurrentUrl());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
+ EXPECT_EQ(1, payload_state.GetNumResponsesSeen());
}
TEST(PayloadStateTest, SetResponseWorksWithMultipleUrls) {
@@ -223,6 +225,7 @@
EXPECT_EQ("http://multiple.url.test", payload_state.GetCurrentUrl());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
+ EXPECT_EQ(1, payload_state.GetNumResponsesSeen());
}
TEST(PayloadStateTest, CanAdvanceUrlIndexCorrectly) {
@@ -284,6 +287,7 @@
// Set the first response.
SetupPayloadStateWith2Urls("Hash5823", true, &payload_state, &response);
+ EXPECT_EQ(1, payload_state.GetNumResponsesSeen());
// Advance the URL index to 1 by faking an error.
ErrorCode error = kErrorCodeDownloadMetadataSignatureMismatch;
@@ -293,6 +297,7 @@
// Now, slightly change the response and set it again.
SetupPayloadStateWith2Urls("Hash8225", true, &payload_state, &response);
+ EXPECT_EQ(2, payload_state.GetNumResponsesSeen());
// Make sure the url index was reset to 0 because of the new response.
EXPECT_EQ("http://test", payload_state.GetCurrentUrl());
@@ -354,6 +359,7 @@
EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
SetupPayloadStateWith2Urls("Hash5873", true, &payload_state, &response);
+ EXPECT_EQ(1, payload_state.GetNumResponsesSeen());
// This should advance the URL index.
payload_state.UpdateFailed(kErrorCodeDownloadMetadataSignatureMismatch);
@@ -424,6 +430,7 @@
// Now, slightly change the response and set it again.
SetupPayloadStateWith2Urls("Hash8532", true, &payload_state, &response);
+ EXPECT_EQ(2, payload_state.GetNumResponsesSeen());
// Make sure the url index was reset to 0 because of the new response.
EXPECT_EQ(0, payload_state.GetPayloadAttemptNumber());
@@ -612,6 +619,7 @@
EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
SetupPayloadStateWith2Urls("Hash3286", true, &payload_state, &response);
+ EXPECT_EQ(1, payload_state.GetNumResponsesSeen());
// Simulate a previous attempt with in order to set an initial non-zero value
// for the total bytes downloaded for HTTP.
@@ -628,6 +636,7 @@
// Change the response hash so as to simulate a new response which will
// reset the current bytes downloaded, but not the total bytes downloaded.
SetupPayloadStateWith2Urls("Hash9904", true, &payload_state, &response);
+ EXPECT_EQ(2, payload_state.GetNumResponsesSeen());
// First, simulate successful download of a few bytes over HTTP.
int first_chunk = 5000000;
@@ -717,6 +726,7 @@
kDownloadSourceHttpsServer));
EXPECT_EQ(0,
payload_state.GetTotalBytesDownloaded(kDownloadSourceHttpsServer));
+ EXPECT_EQ(0, payload_state.GetNumResponsesSeen());
}
TEST(PayloadStateTest, RestartingUpdateResetsMetrics) {