Add metric to report number of URL switches per update attempt
BUG=chromium:226765
TEST=Existing unit tests modified to test feature, Unit tests pass
Change-Id: I5ee0816cac3fa4fa641949ed4872aef8074e3fcc
Reviewed-on: https://gerrit.chromium.org/gerrit/48537
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
diff --git a/constants.cc b/constants.cc
index 3064c40..0bc95f7 100644
--- a/constants.cc
+++ b/constants.cc
@@ -31,6 +31,7 @@
const char kPrefsTotalBytesDownloaded[] = "total-bytes-downloaded";
const char kPrefsUpdateCheckCount[] = "update-check-count";
const char kPrefsUpdateCheckResponseHash[] = "update-check-response-hash";
+const char kPrefsUpdateDurationUptime[] = "update-duration-uptime";
const char kPrefsUpdateFirstSeenAt[] = "update-first-seen-at";
const char kPrefsUpdateServerCertificate[] = "update-server-cert";
const char kPrefsUpdateStateNextDataOffset[] = "update-state-next-data-offset";
@@ -39,8 +40,8 @@
const char kPrefsUpdateStateSignatureBlob[] = "update-state-signature-blob";
const char kPrefsUpdateStateSignedSHA256Context[] =
"update-state-signed-sha-256-context";
-const char kPrefsWallClockWaitPeriod[] = "wall-clock-wait-period";
const char kPrefsUpdateTimestampStart[] = "update-timestamp-start";
-const char kPrefsUpdateDurationUptime[] = "update-duration-uptime";
+const char kPrefsUrlSwitchCount[] = "url-switch-count";
+const char kPrefsWallClockWaitPeriod[] = "wall-clock-wait-period";
}
diff --git a/constants.h b/constants.h
index b2c1de2..3b8a918 100644
--- a/constants.h
+++ b/constants.h
@@ -32,6 +32,7 @@
extern const char kPrefsTotalBytesDownloaded[];
extern const char kPrefsUpdateCheckCount[];
extern const char kPrefsUpdateCheckResponseHash[];
+extern const char kPrefsUpdateDurationUptime[];
extern const char kPrefsUpdateFirstSeenAt[];
extern const char kPrefsUpdateServerCertificate[];
extern const char kPrefsUpdateStateNextDataOffset[];
@@ -39,9 +40,9 @@
extern const char kPrefsUpdateStateSHA256Context[];
extern const char kPrefsUpdateStateSignatureBlob[];
extern const char kPrefsUpdateStateSignedSHA256Context[];
-extern const char kPrefsWallClockWaitPeriod[];
extern const char kPrefsUpdateTimestampStart[];
-extern const char kPrefsUpdateDurationUptime[];
+extern const char kPrefsUrlSwitchCount[];
+extern const char kPrefsWallClockWaitPeriod[];
// A download source is any combination of protocol and server (that's of
// interest to us when looking at UMA metrics) using which we may download
diff --git a/mock_payload_state.h b/mock_payload_state.h
index c32ff78..fe744de 100644
--- a/mock_payload_state.h
+++ b/mock_payload_state.h
@@ -31,6 +31,7 @@
MOCK_METHOD0(GetPayloadAttemptNumber, uint32_t());
MOCK_METHOD0(GetUrlIndex, uint32_t());
MOCK_METHOD0(GetUrlFailureCount, uint32_t());
+ MOCK_METHOD0(GetUrlSwitchCount, uint32_t());
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 bdc9bb4..2a26722 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -34,7 +34,8 @@
: prefs_(NULL),
payload_attempt_number_(0),
url_index_(0),
- url_failure_count_(0) {
+ url_failure_count_(0),
+ url_switch_count_(0) {
for (int i = 0; i <= kNumDownloadSources; i++)
total_bytes_downloaded_[i] = current_bytes_downloaded_[i] = 0;
}
@@ -46,6 +47,7 @@
LoadPayloadAttemptNumber();
LoadUrlIndex();
LoadUrlFailureCount();
+ LoadUrlSwitchCount();
LoadBackoffExpiryTime();
LoadUpdateTimestampStart();
// The LoadUpdateDurationUptime() method relies on LoadUpdateTimestampStart()
@@ -132,6 +134,7 @@
CalculateUpdateDurationUptime();
SetUpdateTimestampEnd(Time::Now());
ReportBytesDownloadedMetrics();
+ ReportUpdateUrlSwitchesMetric();
}
void PayloadState::UpdateFailed(ActionExitCode error) {
@@ -304,6 +307,10 @@
IncrementPayloadAttemptNumber();
}
+ // If we have multiple URLs, record that we just switched to another one
+ if (GetNumUrls() > 1)
+ SetUrlSwitchCount(url_switch_count_ + 1);
+
// Whenever we update the URL index, we should also clear the URL failure
// count so we can start over fresh for the new URL.
SetUrlFailureCount(0);
@@ -415,10 +422,24 @@
}
}
+void PayloadState::ReportUpdateUrlSwitchesMetric() {
+ string metric = "Installer.UpdateURLSwitches";
+ int value = static_cast<int>(url_switch_count_);
+
+ LOG(INFO) << "Uploading " << value << " (count) for metric " << metric;
+ system_state_->metrics_lib()->SendToUMA(
+ metric,
+ value,
+ 0, // min value
+ 100, // max value
+ kNumDefaultUmaBuckets);
+}
+
void PayloadState::ResetPersistedState() {
SetPayloadAttemptNumber(0);
SetUrlIndex(0);
SetUrlFailureCount(0);
+ SetUrlSwitchCount(0);
UpdateBackoffExpiryTime(); // This will reset the backoff expiry time.
SetUpdateTimestampStart(Time::Now());
SetUpdateTimestampEnd(Time()); // Set to null time
@@ -521,6 +542,17 @@
UpdateCurrentDownloadSource();
}
+void PayloadState::LoadUrlSwitchCount() {
+ SetUrlSwitchCount(GetPersistedValue(kPrefsUrlSwitchCount));
+}
+
+void PayloadState::SetUrlSwitchCount(uint32_t url_switch_count) {
+ CHECK(prefs_);
+ url_switch_count_ = url_switch_count;
+ LOG(INFO) << "URL Switch Count = " << url_switch_count_;
+ prefs_->SetInt64(kPrefsUrlSwitchCount, url_switch_count_);
+}
+
void PayloadState::LoadUrlFailureCount() {
SetUrlFailureCount(GetPersistedValue(kPrefsCurrentUrlFailureCount));
}
diff --git a/payload_state.h b/payload_state.h
index b34422f..5427c74 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -56,6 +56,10 @@
return url_failure_count_;
}
+ virtual inline uint32_t GetUrlSwitchCount() {
+ return url_switch_count_;
+ }
+
virtual inline base::Time GetBackoffExpiryTime() {
return backoff_expiry_time_;
}
@@ -80,7 +84,8 @@
// Advances the current URL index to the next available one. If all URLs have
// been exhausted during the current payload download attempt (as indicated
// by the payload attempt number), then it will increment the payload attempt
- // number and wrap around again with the first URL in the list.
+ // number and wrap around again with the first URL in the list. This also
+ // updates the URL switch count, if needed.
void IncrementUrlIndex();
// Increments the failure count of the current URL. If the configured max
@@ -104,6 +109,9 @@
// Reports the various metrics related to the number of bytes downloaded.
void ReportBytesDownloadedMetrics();
+ // Reports the metric related to number of URL switches.
+ void ReportUpdateUrlSwitchesMetric();
+
// Resets all the persisted state values which are maintained relative to the
// current response signature. The response signature itself is not reset.
void ResetPersistedState();
@@ -153,6 +161,12 @@
// restart.
void SetUrlFailureCount(uint32_t url_failure_count);
+ // Sets |url_switch_count_| to the given value and persists the value.
+ void SetUrlSwitchCount(uint32_t url_switch_count);
+
+ // Initializes |url_switch_count_| from the persisted stae.
+ void LoadUrlSwitchCount();
+
// Initializes the backoff expiry time from the persisted state.
void LoadBackoffExpiryTime();
@@ -253,6 +267,9 @@
// persisted so we resume from the same value in case of a process restart.
int64_t url_failure_count_;
+ // The number of times we've switched URLs.
+ int32_t url_switch_count_;
+
// The current download source based on the current URL. This value is
// not persisted as it can be recomputed everytime we update the URL.
// We're storing this so as not to recompute this on every few bytes of
diff --git a/payload_state_interface.h b/payload_state_interface.h
index 133124d..76b847a 100644
--- a/payload_state_interface.h
+++ b/payload_state_interface.h
@@ -72,6 +72,10 @@
// Returns the current URL's failure count.
virtual uint32_t GetUrlFailureCount() = 0;
+ // Returns the total number of times a new URL has been switched to
+ // for the current response.
+ virtual uint32_t GetUrlSwitchCount() = 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 ffd1b27..a0f5a42 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -114,6 +114,7 @@
EXPECT_EQ(expected_response_sign, stored_response_sign);
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
}
TEST(PayloadStateTest, SetResponseWorksWithSingleUrl) {
@@ -158,6 +159,7 @@
EXPECT_EQ(expected_response_sign, stored_response_sign);
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
}
TEST(PayloadStateTest, SetResponseWorksWithMultipleUrls) {
@@ -200,6 +202,7 @@
EXPECT_EQ(expected_response_sign, stored_response_sign);
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
}
TEST(PayloadStateTest, CanAdvanceUrlIndexCorrectly) {
@@ -244,6 +247,9 @@
// Verify that on the next error, it again advances to 1.
payload_state.UpdateFailed(error);
EXPECT_EQ(1, payload_state.GetUrlIndex());
+
+ // Verify that we switched URLs three times
+ EXPECT_EQ(3, payload_state.GetUrlSwitchCount());
}
TEST(PayloadStateTest, NewResponseResetsPayloadState) {
@@ -260,6 +266,7 @@
ActionExitCode error = kActionCodeDownloadMetadataSignatureMismatch;
payload_state.UpdateFailed(error);
EXPECT_EQ(1, payload_state.GetUrlIndex());
+ EXPECT_EQ(1, payload_state.GetUrlSwitchCount());
// Now, slightly change the response and set it again.
SetupPayloadStateWith2Urls("Hash8225", &payload_state, &response);
@@ -267,6 +274,7 @@
// Make sure the url index was reset to 0 because of the new response.
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
EXPECT_EQ(0,
payload_state.GetCurrentBytesDownloaded(kDownloadSourceHttpServer));
EXPECT_EQ(0,
@@ -327,18 +335,21 @@
EXPECT_EQ(0, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(1, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(1, payload_state.GetUrlSwitchCount());
// This should advance the failure count only.
payload_state.UpdateFailed(kActionCodeDownloadTransferError);
EXPECT_EQ(0, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(1, payload_state.GetUrlIndex());
EXPECT_EQ(1, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(1, payload_state.GetUrlSwitchCount());
// This should advance the failure count only.
payload_state.UpdateFailed(kActionCodeDownloadTransferError);
EXPECT_EQ(0, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(1, payload_state.GetUrlIndex());
EXPECT_EQ(2, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(1, payload_state.GetUrlSwitchCount());
// This should advance the URL index as we've reached the
// max failure count and reset the failure count for the new URL index.
@@ -348,6 +359,7 @@
EXPECT_EQ(1, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(2, payload_state.GetUrlSwitchCount());
EXPECT_TRUE(payload_state.ShouldBackoffDownload());
// This should advance the URL index.
@@ -355,6 +367,7 @@
EXPECT_EQ(1, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(1, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(3, payload_state.GetUrlSwitchCount());
EXPECT_TRUE(payload_state.ShouldBackoffDownload());
// This should advance the URL index and payload attempt number due to
@@ -363,6 +376,7 @@
EXPECT_EQ(2, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(4, payload_state.GetUrlSwitchCount());
EXPECT_TRUE(payload_state.ShouldBackoffDownload());
// This HTTP error code should only increase the failure count.
@@ -371,6 +385,7 @@
EXPECT_EQ(2, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(1, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(4, payload_state.GetUrlSwitchCount());
EXPECT_TRUE(payload_state.ShouldBackoffDownload());
// And that failure count should be reset when we download some bytes
@@ -379,6 +394,7 @@
EXPECT_EQ(2, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(4, payload_state.GetUrlSwitchCount());
EXPECT_TRUE(payload_state.ShouldBackoffDownload());
// Now, slightly change the response and set it again.
@@ -388,6 +404,7 @@
EXPECT_EQ(0, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
EXPECT_FALSE(payload_state.ShouldBackoffDownload());
}
@@ -421,6 +438,7 @@
EXPECT_EQ(1, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
}
TEST(PayloadStateTest, SetResponseResetsInvalidUrlIndex) {
@@ -439,6 +457,7 @@
EXPECT_EQ(1, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(1, payload_state.GetUrlIndex());
EXPECT_EQ(1, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(1, payload_state.GetUrlSwitchCount());
// Now, simulate a corrupted url index on persisted store which gets
// loaded when update_engine restarts. Using a different prefs object
@@ -453,6 +472,8 @@
.WillRepeatedly(DoAll(SetArgumentPointee<1>(2), Return(true)));
EXPECT_CALL(*prefs2, GetInt64(kPrefsCurrentUrlFailureCount, _))
.Times(AtLeast(1));
+ EXPECT_CALL(*prefs2, GetInt64(kPrefsUrlSwitchCount, _))
+ .Times(AtLeast(1));
// Note: This will be a different payload object, but the response should
// have the same hash as before so as to not trivially reset because the
@@ -466,6 +487,7 @@
EXPECT_EQ(0, payload_state.GetPayloadAttemptNumber());
EXPECT_EQ(0, payload_state.GetUrlIndex());
EXPECT_EQ(0, payload_state.GetUrlFailureCount());
+ EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
}
TEST(PayloadStateTest, NoBackoffForDeltaPayloads) {
@@ -628,6 +650,9 @@
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
"Installer.TotalMBsDownloadedFromHttpsServer",
https_total / kNumBytesInOneMiB, _, _, _));
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.UpdateURLSwitches",
+ 2, _, _, _));
payload_state.UpdateSucceeded();