Split the ReportUpdateAttemptMetrics into two functions
Move out the report download metrics into a new function so that
ReportUpdateAttemptMetrics has a reasonable number of parameters.
Create mocks for these two functions also.
Test: unittest pass
Change-Id: Ib9fc33d282a448c7e2d19bb7e7c06619efc2a278
diff --git a/fake_system_state.h b/fake_system_state.h
index 990e227..67ad3aa 100644
--- a/fake_system_state.h
+++ b/fake_system_state.h
@@ -190,7 +190,7 @@
return &fake_hardware_;
}
- inline testing::NiceMock<MockMetrics>* mock_metrics_reporter() {
+ inline testing::NiceMock<MockMetricsReporter>* mock_metrics_reporter() {
CHECK(metrics_reporter_ == &mock_metrics_reporter_);
return &mock_metrics_reporter_;
}
@@ -236,7 +236,7 @@
FakeClock fake_clock_;
testing::NiceMock<MockConnectionManager> mock_connection_manager_;
FakeHardware fake_hardware_;
- testing::NiceMock<MockMetrics> mock_metrics_reporter_;
+ testing::NiceMock<MockMetricsReporter> mock_metrics_reporter_;
testing::NiceMock<MockPrefs> mock_prefs_;
testing::NiceMock<MockPrefs> mock_powerwash_safe_prefs_;
testing::NiceMock<MockPayloadState> mock_payload_state_;
diff --git a/metrics_reporter_android.cc b/metrics_reporter_android.cc
index ebfad0d..4a57918 100644
--- a/metrics_reporter_android.cc
+++ b/metrics_reporter_android.cc
@@ -36,13 +36,8 @@
base::TimeDelta /* duration */,
base::TimeDelta /* duration_uptime */,
int64_t /* payload_size */,
- int64_t /* payload_bytes_downloaded */,
- int64_t /* payload_download_speed_bps */,
- DownloadSource /* download_source */,
metrics::AttemptResult /* attempt_result */,
- ErrorCode error_code,
- metrics::DownloadErrorCode /* payload_download_error_code */,
- metrics::ConnectionType /* connection_type */) {
+ ErrorCode error_code) {
// No need to log histogram under sideload mode.
#ifndef _UE_SIDELOAD
android::metricslogger::LogHistogram(metrics::kMetricsUpdateEngineErrorCode,
diff --git a/metrics_reporter_android.h b/metrics_reporter_android.h
index 9ee8bf5..b83e651 100644
--- a/metrics_reporter_android.h
+++ b/metrics_reporter_android.h
@@ -41,20 +41,21 @@
metrics::CheckReaction reaction,
metrics::DownloadErrorCode download_error_code) override {}
- void ReportUpdateAttemptMetrics(
- SystemState* system_state,
- int attempt_number,
- PayloadType payload_type,
- base::TimeDelta duration,
- base::TimeDelta duration_uptime,
- int64_t payload_size,
+ void ReportUpdateAttemptMetrics(SystemState* system_state,
+ int attempt_number,
+ PayloadType payload_type,
+ base::TimeDelta duration,
+ base::TimeDelta duration_uptime,
+ int64_t payload_size,
+ metrics::AttemptResult attempt_result,
+ ErrorCode internal_error_code) override;
+
+ void ReportUpdateAttemptDownloadMetrics(
int64_t payload_bytes_downloaded,
int64_t payload_download_speed_bps,
DownloadSource download_source,
- metrics::AttemptResult attempt_result,
- ErrorCode internal_error_code,
metrics::DownloadErrorCode payload_download_error_code,
- metrics::ConnectionType connection_type) override;
+ metrics::ConnectionType connection_type) override {}
void ReportAbnormallyTerminatedUpdateAttemptMetrics() override {}
diff --git a/metrics_reporter_interface.h b/metrics_reporter_interface.h
index 0c8a025..bc9d387 100644
--- a/metrics_reporter_interface.h
+++ b/metrics_reporter_interface.h
@@ -86,12 +86,8 @@
// |kMetricAttemptDurationUptimeMinutes|
// |kMetricAttemptTimeSinceLastAttemptMinutes|
// |kMetricAttemptTimeSinceLastAttemptUptimeMinutes|
- // |kMetricAttemptPayloadBytesDownloadedMiB|
- // |kMetricAttemptPayloadDownloadSpeedKBps|
- // |kMetricAttemptDownloadSource|
// |kMetricAttemptResult|
// |kMetricAttemptInternalErrorCode|
- // |kMetricAttemptDownloadErrorCode|
//
// The |kMetricAttemptInternalErrorCode| metric will only be reported
// if |internal_error_code| is not |kErrorSuccess|.
@@ -103,18 +99,27 @@
// |kMetricAttemptTimeSinceLastAttemptUptimeMinutes| metrics are
// automatically calculated and reported by maintaining persistent and
// process-local state variables.
- virtual void ReportUpdateAttemptMetrics(
- SystemState* system_state,
- int attempt_number,
- PayloadType payload_type,
- base::TimeDelta duration,
- base::TimeDelta duration_uptime,
- int64_t payload_size,
+ virtual void ReportUpdateAttemptMetrics(SystemState* system_state,
+ int attempt_number,
+ PayloadType payload_type,
+ base::TimeDelta duration,
+ base::TimeDelta duration_uptime,
+ int64_t payload_size,
+ metrics::AttemptResult attempt_result,
+ ErrorCode internal_error_code) = 0;
+
+ // Helper function to report download metrics after the completion of each
+ // update attempt. The following metrics are reported:
+ //
+ // |kMetricAttemptPayloadBytesDownloadedMiB|
+ // |kMetricAttemptPayloadDownloadSpeedKBps|
+ // |kMetricAttemptDownloadSource|
+ // |kMetricAttemptDownloadErrorCode|
+ // |kMetricAttemptConnectionType|
+ virtual void ReportUpdateAttemptDownloadMetrics(
int64_t payload_bytes_downloaded,
int64_t payload_download_speed_bps,
DownloadSource download_source,
- metrics::AttemptResult attempt_result,
- ErrorCode internal_error_code,
metrics::DownloadErrorCode payload_download_error_code,
metrics::ConnectionType connection_type) = 0;
diff --git a/metrics_reporter_omaha.cc b/metrics_reporter_omaha.cc
index 3aaf1c6..5f24cbf 100644
--- a/metrics_reporter_omaha.cc
+++ b/metrics_reporter_omaha.cc
@@ -212,13 +212,8 @@
base::TimeDelta duration,
base::TimeDelta duration_uptime,
int64_t payload_size,
- int64_t payload_bytes_downloaded,
- int64_t payload_download_speed_bps,
- DownloadSource download_source,
metrics::AttemptResult attempt_result,
- ErrorCode internal_error_code,
- metrics::DownloadErrorCode payload_download_error_code,
- metrics::ConnectionType connection_type) {
+ ErrorCode internal_error_code) {
string metric = metrics::kMetricAttemptNumber;
LOG(INFO) << "Uploading " << attempt_number << " for metric " << metric;
metrics_lib_->SendToUMA(metric,
@@ -259,30 +254,7 @@
1024, // max: 1024 MiB = 1 GiB
50); // num_buckets
- metric = metrics::kMetricAttemptPayloadBytesDownloadedMiB;
- int64_t payload_bytes_downloaded_mib =
- payload_bytes_downloaded / kNumBytesInOneMiB;
- LOG(INFO) << "Uploading " << payload_bytes_downloaded_mib << " for metric "
- << metric;
- metrics_lib_->SendToUMA(metric,
- payload_bytes_downloaded_mib,
- 0, // min: 0 MiB
- 1024, // max: 1024 MiB = 1 GiB
- 50); // num_buckets
- metric = metrics::kMetricAttemptPayloadDownloadSpeedKBps;
- int64_t payload_download_speed_kbps = payload_download_speed_bps / 1000;
- LOG(INFO) << "Uploading " << payload_download_speed_kbps << " for metric "
- << metric;
- metrics_lib_->SendToUMA(metric,
- payload_download_speed_kbps,
- 0, // min: 0 kB/s
- 10 * 1000, // max: 10000 kB/s = 10 MB/s
- 50); // num_buckets
-
- metric = metrics::kMetricAttemptDownloadSource;
- LOG(INFO) << "Uploading " << download_source << " for metric " << metric;
- metrics_lib_->SendEnumToUMA(metric, download_source, kNumDownloadSources);
metric = metrics::kMetricAttemptResult;
LOG(INFO) << "Uploading " << static_cast<int>(attempt_result)
@@ -301,14 +273,6 @@
static_cast<int>(ErrorCode::kUmaReportedMax));
}
- if (payload_download_error_code != metrics::DownloadErrorCode::kUnset) {
- metric = metrics::kMetricAttemptDownloadErrorCode;
- LOG(INFO) << "Uploading " << static_cast<int>(payload_download_error_code)
- << " for metric " << metric << " (sparse)";
- metrics_lib_->SendSparseToUMA(
- metric, static_cast<int>(payload_download_error_code));
- }
-
base::TimeDelta time_since_last;
if (metrics_utils::WallclockDurationHelper(
system_state,
@@ -337,6 +301,46 @@
30 * 24 * 60, // max: 30 days
50); // num_buckets
}
+}
+
+void MetricsReporterOmaha::ReportUpdateAttemptDownloadMetrics(
+ int64_t payload_bytes_downloaded,
+ int64_t payload_download_speed_bps,
+ DownloadSource download_source,
+ metrics::DownloadErrorCode payload_download_error_code,
+ metrics::ConnectionType connection_type) {
+ string metric = metrics::kMetricAttemptPayloadBytesDownloadedMiB;
+ int64_t payload_bytes_downloaded_mib =
+ payload_bytes_downloaded / kNumBytesInOneMiB;
+ LOG(INFO) << "Uploading " << payload_bytes_downloaded_mib << " for metric "
+ << metric;
+ metrics_lib_->SendToUMA(metric,
+ payload_bytes_downloaded_mib,
+ 0, // min: 0 MiB
+ 1024, // max: 1024 MiB = 1 GiB
+ 50); // num_buckets
+
+ metric = metrics::kMetricAttemptPayloadDownloadSpeedKBps;
+ int64_t payload_download_speed_kbps = payload_download_speed_bps / 1000;
+ LOG(INFO) << "Uploading " << payload_download_speed_kbps << " for metric "
+ << metric;
+ metrics_lib_->SendToUMA(metric,
+ payload_download_speed_kbps,
+ 0, // min: 0 kB/s
+ 10 * 1000, // max: 10000 kB/s = 10 MB/s
+ 50); // num_buckets
+
+ metric = metrics::kMetricAttemptDownloadSource;
+ LOG(INFO) << "Uploading " << download_source << " for metric " << metric;
+ metrics_lib_->SendEnumToUMA(metric, download_source, kNumDownloadSources);
+
+ if (payload_download_error_code != metrics::DownloadErrorCode::kUnset) {
+ metric = metrics::kMetricAttemptDownloadErrorCode;
+ LOG(INFO) << "Uploading " << static_cast<int>(payload_download_error_code)
+ << " for metric " << metric << " (sparse)";
+ metrics_lib_->SendSparseToUMA(
+ metric, static_cast<int>(payload_download_error_code));
+ }
metric = metrics::kMetricAttemptConnectionType;
LOG(INFO) << "Uploading " << static_cast<int>(connection_type)
diff --git a/metrics_reporter_omaha.h b/metrics_reporter_omaha.h
index b209b13..c19fe86 100644
--- a/metrics_reporter_omaha.h
+++ b/metrics_reporter_omaha.h
@@ -105,18 +105,19 @@
metrics::CheckReaction reaction,
metrics::DownloadErrorCode download_error_code) override;
- void ReportUpdateAttemptMetrics(
- SystemState* system_state,
- int attempt_number,
- PayloadType payload_type,
- base::TimeDelta duration,
- base::TimeDelta duration_uptime,
- int64_t payload_size,
+ void ReportUpdateAttemptMetrics(SystemState* system_state,
+ int attempt_number,
+ PayloadType payload_type,
+ base::TimeDelta duration,
+ base::TimeDelta duration_uptime,
+ int64_t payload_size,
+ metrics::AttemptResult attempt_result,
+ ErrorCode internal_error_code) override;
+
+ void ReportUpdateAttemptDownloadMetrics(
int64_t payload_bytes_downloaded,
int64_t payload_download_speed_bps,
DownloadSource download_source,
- metrics::AttemptResult attempt_result,
- ErrorCode internal_error_code,
metrics::DownloadErrorCode payload_download_error_code,
metrics::ConnectionType connection_type) override;
diff --git a/metrics_reporter_omaha_unittest.cc b/metrics_reporter_omaha_unittest.cc
index 7a4102e..76e33c6 100644
--- a/metrics_reporter_omaha_unittest.cc
+++ b/metrics_reporter_omaha_unittest.cc
@@ -133,15 +133,10 @@
TimeDelta duration_uptime = TimeDelta::FromMinutes(1000);
int64_t payload_size = 100 * kNumBytesInOneMiB;
- int64_t payload_bytes_downloaded = 200 * kNumBytesInOneMiB;
- int64_t payload_download_speed_bps = 100 * 1000;
- DownloadSource download_source = kDownloadSourceHttpServer;
+
metrics::AttemptResult attempt_result =
metrics::AttemptResult::kInternalError;
ErrorCode internal_error_code = ErrorCode::kDownloadInvalidMetadataSignature;
- metrics::DownloadErrorCode payload_download_error_code =
- metrics::DownloadErrorCode::kDownloadError;
- metrics::ConnectionType connection_type = metrics::ConnectionType::kCellular;
EXPECT_CALL(*mock_metrics_lib_,
SendToUMA(metrics::kMetricAttemptNumber, attempt_number, _, _, _))
@@ -166,23 +161,6 @@
_))
.Times(2);
- // Check the report of payload download metrics.
- EXPECT_CALL(*mock_metrics_lib_,
- SendToUMA(metrics::kMetricAttemptPayloadSizeMiB, 100, _, _, _))
- .Times(2);
- EXPECT_CALL(
- *mock_metrics_lib_,
- SendToUMA(metrics::kMetricAttemptPayloadBytesDownloadedMiB, 200, _, _, _))
- .Times(2);
- EXPECT_CALL(
- *mock_metrics_lib_,
- SendToUMA(metrics::kMetricAttemptPayloadDownloadSpeedKBps, 100, _, _, _))
- .Times(2);
- EXPECT_CALL(*mock_metrics_lib_,
- SendEnumToUMA(metrics::kMetricAttemptDownloadSource,
- static_cast<int>(download_source),
- _))
- .Times(2);
// Check the report of attempt result.
EXPECT_CALL(
@@ -196,8 +174,7 @@
_))
.Times(2);
EXPECT_CALL(*mock_metrics_lib_,
- SendSparseToUMA(metrics::kMetricAttemptDownloadErrorCode,
- static_cast<int>(payload_download_error_code)))
+ SendToUMA(metrics::kMetricAttemptPayloadSizeMiB, 100, _, _, _))
.Times(2);
// Check the duration between two reports.
@@ -211,25 +188,14 @@
metrics::kMetricAttemptTimeSinceLastAttemptUptimeMinutes, 1, _, _, _))
.Times(1);
- EXPECT_CALL(*mock_metrics_lib_,
- SendEnumToUMA(metrics::kMetricAttemptConnectionType,
- static_cast<int>(connection_type),
- _))
- .Times(2);
-
reporter_.ReportUpdateAttemptMetrics(&fake_system_state,
attempt_number,
payload_type,
duration,
duration_uptime,
payload_size,
- payload_bytes_downloaded,
- payload_download_speed_bps,
- download_source,
attempt_result,
- internal_error_code,
- payload_download_error_code,
- connection_type);
+ internal_error_code);
// Advance the clock by 1 minute and report the same metrics again.
fake_clock.SetWallclockTime(base::Time::FromInternalValue(61000000));
@@ -240,13 +206,46 @@
duration,
duration_uptime,
payload_size,
- payload_bytes_downloaded,
- payload_download_speed_bps,
- download_source,
attempt_result,
- internal_error_code,
- payload_download_error_code,
- connection_type);
+ internal_error_code);
+}
+
+TEST_F(MetricsReporterOmahaTest, ReportUpdateAttemptDownloadMetrics) {
+ int64_t payload_bytes_downloaded = 200 * kNumBytesInOneMiB;
+ int64_t payload_download_speed_bps = 100 * 1000;
+ DownloadSource download_source = kDownloadSourceHttpServer;
+ metrics::DownloadErrorCode payload_download_error_code =
+ metrics::DownloadErrorCode::kDownloadError;
+ metrics::ConnectionType connection_type = metrics::ConnectionType::kCellular;
+
+ EXPECT_CALL(
+ *mock_metrics_lib_,
+ SendToUMA(metrics::kMetricAttemptPayloadBytesDownloadedMiB, 200, _, _, _))
+ .Times(1);
+ EXPECT_CALL(
+ *mock_metrics_lib_,
+ SendToUMA(metrics::kMetricAttemptPayloadDownloadSpeedKBps, 100, _, _, _))
+ .Times(1);
+ EXPECT_CALL(*mock_metrics_lib_,
+ SendEnumToUMA(metrics::kMetricAttemptDownloadSource,
+ static_cast<int>(download_source),
+ _))
+ .Times(1);
+ EXPECT_CALL(*mock_metrics_lib_,
+ SendSparseToUMA(metrics::kMetricAttemptDownloadErrorCode,
+ static_cast<int>(payload_download_error_code)))
+ .Times(1);
+ EXPECT_CALL(*mock_metrics_lib_,
+ SendEnumToUMA(metrics::kMetricAttemptConnectionType,
+ static_cast<int>(connection_type),
+ _))
+ .Times(1);
+
+ reporter_.ReportUpdateAttemptDownloadMetrics(payload_bytes_downloaded,
+ payload_download_speed_bps,
+ download_source,
+ payload_download_error_code,
+ connection_type);
}
TEST_F(MetricsReporterOmahaTest, ReportSuccessfulUpdateMetrics) {
diff --git a/mock_metrics_reporter.h b/mock_metrics_reporter.h
index 60982a7..a0f164b 100644
--- a/mock_metrics_reporter.h
+++ b/mock_metrics_reporter.h
@@ -23,7 +23,7 @@
namespace chromeos_update_engine {
-class MockMetrics : public MetricsReporterInterface {
+class MockMetricsReporter : public MetricsReporterInterface {
public:
MOCK_METHOD0(Initialize, void());
@@ -37,20 +37,22 @@
metrics::CheckReaction reaction,
metrics::DownloadErrorCode download_error_code));
- void ReportUpdateAttemptMetrics(
- SystemState* system_state,
- int attempt_number,
- PayloadType payload_type,
- base::TimeDelta duration,
- base::TimeDelta duration_uptime,
- int64_t payload_size,
- int64_t payload_bytes_downloaded,
- int64_t payload_download_speed_bps,
- DownloadSource download_source,
- metrics::AttemptResult attempt_result,
- ErrorCode internal_error_code,
- metrics::DownloadErrorCode payload_download_error_code,
- metrics::ConnectionType connection_type) override {}
+ MOCK_METHOD8(ReportUpdateAttemptMetrics,
+ void(SystemState* system_state,
+ int attempt_number,
+ PayloadType payload_type,
+ base::TimeDelta duration,
+ base::TimeDelta duration_uptime,
+ int64_t payload_size,
+ metrics::AttemptResult attempt_result,
+ ErrorCode internal_error_code));
+
+ MOCK_METHOD5(ReportUpdateAttemptDownloadMetrics,
+ void(int64_t payload_bytes_downloaded,
+ int64_t payload_download_speed_bps,
+ DownloadSource download_source,
+ metrics::DownloadErrorCode payload_download_error_code,
+ metrics::ConnectionType connection_type));
MOCK_METHOD0(ReportAbnormallyTerminatedUpdateAttemptMetrics, void());
diff --git a/payload_state.cc b/payload_state.cc
index 1f121b0..410a0bf 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -643,11 +643,13 @@
duration,
duration_uptime,
payload_size,
+ attempt_result,
+ internal_error_code);
+
+ system_state_->metrics_reporter()->ReportUpdateAttemptDownloadMetrics(
payload_bytes_downloaded,
payload_download_speed_bps,
download_source,
- attempt_result,
- internal_error_code,
payload_download_error_code,
attempt_connection_type_);
}
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index b277d95..a69c5ac 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -1133,7 +1133,7 @@
TEST(PayloadStateTest, RestartAfterCrash) {
PayloadState payload_state;
FakeSystemState fake_system_state;
- testing::StrictMock<MockMetrics> mock_metrics_reporter;
+ testing::StrictMock<MockMetricsReporter> mock_metrics_reporter;
fake_system_state.set_metrics_reporter(&mock_metrics_reporter);
NiceMock<MockPrefs>* prefs = fake_system_state.mock_prefs();
diff --git a/update_attempter_android.cc b/update_attempter_android.cc
index 0a1c20f..f6b9702 100644
--- a/update_attempter_android.cc
+++ b/update_attempter_android.cc
@@ -433,27 +433,20 @@
// TODO(xunchang): assign proper values to the metrics.
PayloadType payload_type = kNumPayloadTypes;
- DownloadSource download_source = kNumDownloadSources;
metrics::AttemptResult attempt_result =
metrics_utils::GetAttemptResult(error_code);
TimeDelta duration;
TimeDelta duration_uptime;
// Report the android metrics when we terminate the update. Currently we are
// reporting error_code only.
- metrics_reporter_->ReportUpdateAttemptMetrics(
- nullptr, // system_state
- 0, // attempt_number
- payload_type,
- duration,
- duration_uptime,
- 0, // payload_size
- 0, // payload_bytes_downloaded
- 0, // payload_download_speed_bps
- download_source,
- attempt_result,
- error_code,
- metrics::DownloadErrorCode::kUnset,
- metrics::ConnectionType::kUnset);
+ metrics_reporter_->ReportUpdateAttemptMetrics(nullptr, // system_state
+ 0, // attempt_number
+ payload_type,
+ duration,
+ duration_uptime,
+ 0, // payload_size
+ attempt_result,
+ error_code);
}
void UpdateAttempterAndroid::SetStatusAndNotify(UpdateStatus status) {
