p2p: Add HttpPeer to DownloadSource enumeration
This way the Installer.DownloadSourcesUsed metric conveys how often
p2p is used and new metrics Installer.SuccessfulMBsDownloadedFromHttpPeer
and Installer.TotalMBsDownloadedFromHttpPeer gives additional detail.
BUG=chromium:284714
TEST=modify unit tests to cover this case + unit tests pass
Change-Id: Ia4dff090091a282e1a184c2b18f8290749f5fdeb
Reviewed-on: https://chromium-review.googlesource.com/167913
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
diff --git a/constants.h b/constants.h
index 5b0ba8c..b221331 100644
--- a/constants.h
+++ b/constants.h
@@ -78,6 +78,7 @@
typedef enum {
kDownloadSourceHttpsServer, // UMA Binary representation: 0001
kDownloadSourceHttpServer, // UMA Binary representation: 0010
+ kDownloadSourceHttpPeer, // UMA Binary representation: 0100
// Note: Add new sources only above this line.
kNumDownloadSources
diff --git a/mock_payload_state.h b/mock_payload_state.h
index 2e8536a..4e21a0f 100644
--- a/mock_payload_state.h
+++ b/mock_payload_state.h
@@ -31,8 +31,9 @@
MOCK_METHOD0(Rollback, void());
MOCK_METHOD1(ExpectRebootInNewVersion,
void(const std::string& target_version_uid));
- MOCK_METHOD0(GetP2PNumAttempts, int());
- MOCK_METHOD0(GetP2PFirstAttemptTimestamp, base::Time());
+ MOCK_METHOD0(P2PNewAttempt, void());
+ MOCK_METHOD0(P2PAttemptAllowed, bool());
+ MOCK_METHOD1(SetUsingP2PForDownloading, void(bool));
// Getters.
MOCK_METHOD0(GetResponseSignature, std::string());
@@ -49,8 +50,9 @@
MOCK_METHOD1(GetTotalBytesDownloaded, uint64_t(DownloadSource source));
MOCK_METHOD0(GetNumReboots, uint32_t());
MOCK_METHOD0(GetRollbackVersion, std::string());
- MOCK_METHOD0(P2PNewAttempt, void());
- MOCK_METHOD0(P2PAttemptAllowed, bool());
+ MOCK_METHOD0(GetP2PNumAttempts, int());
+ MOCK_METHOD0(GetP2PFirstAttemptTimestamp, base::Time());
+ MOCK_METHOD0(GetUsingP2PForDownloading, bool());
};
} // namespace chromeos_update_engine
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 7df4c8d..8e77afa 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -77,6 +77,7 @@
<< " with local URL " << params->p2p_url()
<< " since p2p is enabled.";
install_plan_.download_url = params->p2p_url();
+ system_state_->payload_state()->SetUsingP2PForDownloading(true);
}
// Fill up the other properties based on the response.
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index 9624b41..6fe48da 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -375,6 +375,9 @@
OmahaRequestParams params(&mock_system_state);
mock_system_state.set_request_params(¶ms);
+ EXPECT_CALL(*mock_system_state.mock_payload_state(),
+ SetUsingP2PForDownloading(true));
+
string p2p_url = "http://9.8.7.6/p2p";
params.set_p2p_url(p2p_url);
params.set_use_p2p_for_downloading(true);
diff --git a/payload_state.cc b/payload_state.cc
index e27e8d7..1463bc0 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -35,6 +35,7 @@
PayloadState::PayloadState()
: prefs_(NULL),
+ using_p2p_for_downloading_(false),
payload_attempt_number_(0),
full_payload_attempt_number_(0),
url_index_(0),
@@ -115,6 +116,13 @@
UpdateCurrentDownloadSource();
}
+void PayloadState::SetUsingP2PForDownloading(bool value) {
+ using_p2p_for_downloading_ = value;
+ // Update the current download source which depends on whether we are
+ // using p2p or not.
+ UpdateCurrentDownloadSource();
+}
+
void PayloadState::DownloadComplete() {
LOG(INFO) << "Payload downloaded successfully";
IncrementPayloadAttemptNumber();
@@ -443,7 +451,9 @@
void PayloadState::UpdateCurrentDownloadSource() {
current_download_source_ = kNumDownloadSources;
- if (GetUrlIndex() < candidate_urls_.size()) {
+ if (using_p2p_for_downloading_) {
+ current_download_source_ = kDownloadSourceHttpPeer;
+ } else if (GetUrlIndex() < candidate_urls_.size()) {
string current_url = candidate_urls_[GetUrlIndex()];
if (StartsWithASCII(current_url, "https://", false))
current_download_source_ = kDownloadSourceHttpsServer;
diff --git a/payload_state.h b/payload_state.h
index f0b74e5..f358bda 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -44,6 +44,7 @@
virtual bool ShouldBackoffDownload();
virtual void Rollback();
virtual void ExpectRebootInNewVersion(const std::string& target_version_uid);
+ virtual void SetUsingP2PForDownloading(bool value);
virtual inline std::string GetResponseSignature() {
return response_signature_;
@@ -104,6 +105,10 @@
virtual void P2PNewAttempt();
virtual bool P2PAttemptAllowed();
+ virtual bool GetUsingP2PForDownloading() {
+ return using_p2p_for_downloading_;
+ }
+
private:
friend class PayloadStateTest;
FRIEND_TEST(PayloadStateTest, RebootAfterUpdateFailedMetric);
@@ -385,6 +390,10 @@
// This is the current response object from Omaha.
OmahaResponse response_;
+ // Whether p2p is being used for downloading as set with the
+ // SetUsingP2PForDownloading() method.
+ bool using_p2p_for_downloading_;
+
// This stores a "signature" of the current response. The signature here
// refers to a subset of the current response from Omaha. Each update to
// this value is persisted so we resume from the same value in case of a
diff --git a/payload_state_interface.h b/payload_state_interface.h
index 83fe3f5..5c67ba6 100644
--- a/payload_state_interface.h
+++ b/payload_state_interface.h
@@ -74,6 +74,11 @@
virtual void ExpectRebootInNewVersion(
const std::string& target_version_uid) = 0;
+ // Sets whether p2p is being used to download the update payload. This
+ // is used to keep track download sources being used and should be called
+ // before the transfer begins.
+ virtual void SetUsingP2PForDownloading(bool value) = 0;
+
// Returns true if we should backoff the current download attempt.
// False otherwise.
virtual bool ShouldBackoffDownload() = 0;
@@ -155,6 +160,9 @@
// and examining how many attempts have been done already as well
// as when the first attempt was.
virtual bool P2PAttemptAllowed() = 0;
+
+ // Gets the value previously set with SetUsingP2PForDownloading().
+ virtual bool GetUsingP2PForDownloading() = 0;
};
} // namespace chromeos_update_engine
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index fc6b4e9..613940e 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -42,6 +42,10 @@
"current-bytes-downloaded-from-HttpServer";
const char* kTotalBytesDownloadedFromHttp =
"total-bytes-downloaded-from-HttpServer";
+const char* kCurrentBytesDownloadedFromHttpPeer =
+ "current-bytes-downloaded-from-HttpPeer";
+const char* kTotalBytesDownloadedFromHttpPeer =
+ "total-bytes-downloaded-from-HttpPeer";
static void SetupPayloadStateWith2Urls(string hash,
bool http_enabled,
@@ -117,6 +121,8 @@
.Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttp, 0))
.Times(AtLeast(1));
+ EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttpPeer, 0))
+ .Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kPrefsNumReboots, 0)).Times(AtLeast(1));
PayloadState payload_state;
EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
@@ -165,6 +171,8 @@
.Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttp, 0))
.Times(AtLeast(1));
+ EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttpPeer, 0))
+ .Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kPrefsNumReboots, 0))
.Times(AtLeast(1));
PayloadState payload_state;
@@ -212,6 +220,8 @@
.Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttp, 0))
.Times(AtLeast(1));
+ EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttpPeer, 0))
+ .Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kPrefsNumReboots, 0))
.Times(AtLeast(1));
@@ -387,6 +397,8 @@
.Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttp, 0))
.Times(AtLeast(1));
+ EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttpPeer, 0))
+ .Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kCurrentBytesDownloadedFromHttp, progress_bytes))
.Times(AtLeast(1));
EXPECT_CALL(*prefs, SetInt64(kTotalBytesDownloadedFromHttp, progress_bytes))
@@ -803,6 +815,16 @@
EXPECT_EQ(https_total,
payload_state.GetTotalBytesDownloaded(kDownloadSourceHttpsServer));
+ // Simulate error (will cause URL switch), set p2p is to be used and
+ // then do 42MB worth of progress
+ payload_state.UpdateFailed(error);
+ payload_state.SetUsingP2PForDownloading(true);
+ int p2p_total = 42 * 1000 * 1000;
+ payload_state.DownloadProgress(p2p_total);
+
+ EXPECT_EQ(p2p_total,
+ payload_state.GetTotalBytesDownloaded(kDownloadSourceHttpPeer));
+
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(_, _, _, _, _))
.Times(AnyNumber());
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
@@ -818,8 +840,14 @@
"Installer.TotalMBsDownloadedFromHttpsServer",
https_total / kNumBytesInOneMiB, _, _, _));
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.SuccessfulMBsDownloadedFromHttpPeer",
+ p2p_total / kNumBytesInOneMiB, _, _, _));
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.TotalMBsDownloadedFromHttpPeer",
+ p2p_total / kNumBytesInOneMiB, _, _, _));
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
"Installer.UpdateURLSwitches",
- 2, _, _, _));
+ 3, _, _, _));
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
"Installer.UpdateDurationMinutes",
_, _, _, _));
@@ -827,9 +855,12 @@
"Installer.UpdateDurationUptimeMinutes",
_, _, _, _));
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
- "Installer.DownloadSourcesUsed", 3, _, _, _));
+ "Installer.DownloadSourcesUsed",
+ (1 << kDownloadSourceHttpsServer) | (1 << kDownloadSourceHttpServer) |
+ (1 << kDownloadSourceHttpPeer),
+ _, _, _));
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
- "Installer.DownloadOverheadPercentage", 542, _, _, _));
+ "Installer.DownloadOverheadPercentage", 318, _, _, _));
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendEnumToUMA(
"Installer.PayloadFormat", kPayloadTypeFull, kNumPayloadTypes));
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
@@ -849,6 +880,35 @@
EXPECT_EQ(0, payload_state.GetNumResponsesSeen());
}
+TEST(PayloadStateTest, DownloadSourcesUsedIsCorrect) {
+ OmahaResponse response;
+ PayloadState payload_state;
+ MockSystemState mock_system_state;
+
+ EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
+ SetupPayloadStateWith2Urls("Hash3286", true, &payload_state, &response);
+
+ // Simulate progress in order to mark HTTP as one of the sources used.
+ int num_bytes = 42 * 1000 * 1000;
+ payload_state.DownloadProgress(num_bytes);
+
+ // Check that this was done via HTTP.
+ EXPECT_EQ(num_bytes,
+ payload_state.GetCurrentBytesDownloaded(kDownloadSourceHttpServer));
+ EXPECT_EQ(num_bytes,
+ payload_state.GetTotalBytesDownloaded(kDownloadSourceHttpServer));
+
+ // Check that only HTTP is reported as a download source.
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(_, _, _, _, _))
+ .Times(AnyNumber());
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.DownloadSourcesUsed",
+ (1 << kDownloadSourceHttpServer),
+ _, _, _));
+
+ payload_state.UpdateSucceeded();
+}
+
TEST(PayloadStateTest, RestartingUpdateResetsMetrics) {
OmahaResponse response;
MockSystemState mock_system_state;
diff --git a/utils.cc b/utils.cc
index 8f04134..007d154 100644
--- a/utils.cc
+++ b/utils.cc
@@ -773,6 +773,7 @@
switch (source) {
case kDownloadSourceHttpsServer: return "HttpsServer";
case kDownloadSourceHttpServer: return "HttpServer";
+ case kDownloadSourceHttpPeer: return "HttpPeer";
case kNumDownloadSources: return "Unknown";
// Don't add a default case to let the compiler warn about newly added
// download sources which should be added here.
