Move metrics time helpers to metrics_utils.
The metrics module reports metrics periodically, for which it needs to
keep track of the duration since some events (for example, the update
finished). These helpers were in the common/utils.h, but they rely on
the global SystemState to access both Prefs and Clock.
Since these helpers are specific to the metric reporting, this CL moves
them to the metrics_utils.h module.
Bug: 25197634
TEST=FEATURES=test emerge-link update_engine; mmma system/update_engine
Change-Id: Ia48091adbdc56c339c69c86c91c5c01aa58c54fb
diff --git a/common/utils.cc b/common/utils.cc
index 788efc8..cfd8db0 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -61,7 +61,6 @@
#include "update_engine/omaha_request_params.h"
#include "update_engine/payload_consumer/file_descriptor.h"
#include "update_engine/payload_consumer/file_writer.h"
-#include "update_engine/system_state.h"
#include "update_engine/update_attempter.h"
using base::Time;
@@ -1247,48 +1246,6 @@
return true;
}
-bool WallclockDurationHelper(SystemState* system_state,
- const string& state_variable_key,
- TimeDelta* out_duration) {
- bool ret = false;
-
- Time now = system_state->clock()->GetWallclockTime();
- int64_t stored_value;
- if (system_state->prefs()->GetInt64(state_variable_key, &stored_value)) {
- Time stored_time = Time::FromInternalValue(stored_value);
- if (stored_time > now) {
- LOG(ERROR) << "Stored time-stamp used for " << state_variable_key
- << " is in the future.";
- } else {
- *out_duration = now - stored_time;
- ret = true;
- }
- }
-
- if (!system_state->prefs()->SetInt64(state_variable_key,
- now.ToInternalValue())) {
- LOG(ERROR) << "Error storing time-stamp in " << state_variable_key;
- }
-
- return ret;
-}
-
-bool MonotonicDurationHelper(SystemState* system_state,
- int64_t* storage,
- TimeDelta* out_duration) {
- bool ret = false;
-
- Time now = system_state->clock()->GetMonotonicTime();
- if (*storage != 0) {
- Time stored_time = Time::FromInternalValue(*storage);
- *out_duration = now - stored_time;
- ret = true;
- }
- *storage = now.ToInternalValue();
-
- return ret;
-}
-
bool GetMinorVersion(const brillo::KeyValueStore& store,
uint32_t* minor_version) {
string result;
diff --git a/common/utils.h b/common/utils.h
index 482b67e..121f0c9 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -32,7 +32,6 @@
#include <base/time/time.h>
#include <brillo/key_value_store.h>
#include <brillo/secure_blob.h>
-#include "metrics/metrics_library.h"
#include "update_engine/common/action.h"
#include "update_engine/common/action_processor.h"
@@ -43,8 +42,6 @@
namespace chromeos_update_engine {
-class SystemState;
-
namespace utils {
// Converts a struct timespec representing a number of seconds since
@@ -355,29 +352,6 @@
// variable and changing environment variables is not thread-safe.
bool ConvertToOmahaInstallDate(base::Time time, int *out_num_days);
-// This function returns the duration on the wallclock since the last
-// time it was called for the same |state_variable_key| value.
-//
-// If the function returns |true|, the duration (always non-negative)
-// is returned in |out_duration|. If the function returns |false|
-// something went wrong or there was no previous measurement.
-bool WallclockDurationHelper(SystemState* system_state,
- const std::string& state_variable_key,
- base::TimeDelta* out_duration);
-
-// This function returns the duration on the monotonic clock since the
-// last time it was called for the same |storage| pointer.
-//
-// You should pass a pointer to a 64-bit integer in |storage| which
-// should be initialized to 0.
-//
-// If the function returns |true|, the duration (always non-negative)
-// is returned in |out_duration|. If the function returns |false|
-// something went wrong or there was no previous measurement.
-bool MonotonicDurationHelper(SystemState* system_state,
- int64_t* storage,
- base::TimeDelta* out_duration);
-
// Look for the minor version value in the passed |store| and set
// |minor_version| to that value. Return whether the value was found and valid.
bool GetMinorVersion(const brillo::KeyValueStore& store,
diff --git a/common/utils_unittest.cc b/common/utils_unittest.cc
index f22e528..02f919e 100644
--- a/common/utils_unittest.cc
+++ b/common/utils_unittest.cc
@@ -34,11 +34,7 @@
#include <brillo/message_loops/message_loop_utils.h>
#include <gtest/gtest.h>
-#include "update_engine/common/fake_clock.h"
-#include "update_engine/common/fake_prefs.h"
-#include "update_engine/common/prefs.h"
#include "update_engine/common/test_utils.h"
-#include "update_engine/fake_system_state.h"
using brillo::FakeMessageLoop;
using std::map;
@@ -506,135 +502,6 @@
EXPECT_EQ(value1, value2 - 7);
}
-TEST(UtilsTest, WallclockDurationHelper) {
- FakeSystemState fake_system_state;
- FakeClock fake_clock;
- base::TimeDelta duration;
- string state_variable_key = "test-prefs";
- FakePrefs fake_prefs;
-
- fake_system_state.set_clock(&fake_clock);
- fake_system_state.set_prefs(&fake_prefs);
-
- // Initialize wallclock to 1 sec.
- fake_clock.SetWallclockTime(base::Time::FromInternalValue(1000000));
-
- // First time called so no previous measurement available.
- EXPECT_FALSE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
-
- // Next time, we should get zero since the clock didn't advance.
- EXPECT_TRUE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
-
- // We can also call it as many times as we want with it being
- // considered a failure.
- EXPECT_TRUE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
- EXPECT_TRUE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
-
- // Advance the clock one second, then we should get 1 sec on the
- // next call and 0 sec on the subsequent call.
- fake_clock.SetWallclockTime(base::Time::FromInternalValue(2000000));
- EXPECT_TRUE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 1);
- EXPECT_TRUE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
-
- // Advance clock two seconds and we should get 2 sec and then 0 sec.
- fake_clock.SetWallclockTime(base::Time::FromInternalValue(4000000));
- EXPECT_TRUE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 2);
- EXPECT_TRUE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
-
- // There's a possibility that the wallclock can go backwards (NTP
- // adjustments, for example) so check that we properly handle this
- // case.
- fake_clock.SetWallclockTime(base::Time::FromInternalValue(3000000));
- EXPECT_FALSE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- fake_clock.SetWallclockTime(base::Time::FromInternalValue(4000000));
- EXPECT_TRUE(utils::WallclockDurationHelper(&fake_system_state,
- state_variable_key,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 1);
-}
-
-TEST(UtilsTest, MonotonicDurationHelper) {
- int64_t storage = 0;
- FakeSystemState fake_system_state;
- FakeClock fake_clock;
- base::TimeDelta duration;
-
- fake_system_state.set_clock(&fake_clock);
-
- // Initialize monotonic clock to 1 sec.
- fake_clock.SetMonotonicTime(base::Time::FromInternalValue(1000000));
-
- // First time called so no previous measurement available.
- EXPECT_FALSE(utils::MonotonicDurationHelper(&fake_system_state,
- &storage,
- &duration));
-
- // Next time, we should get zero since the clock didn't advance.
- EXPECT_TRUE(utils::MonotonicDurationHelper(&fake_system_state,
- &storage,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
-
- // We can also call it as many times as we want with it being
- // considered a failure.
- EXPECT_TRUE(utils::MonotonicDurationHelper(&fake_system_state,
- &storage,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
- EXPECT_TRUE(utils::MonotonicDurationHelper(&fake_system_state,
- &storage,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
-
- // Advance the clock one second, then we should get 1 sec on the
- // next call and 0 sec on the subsequent call.
- fake_clock.SetMonotonicTime(base::Time::FromInternalValue(2000000));
- EXPECT_TRUE(utils::MonotonicDurationHelper(&fake_system_state,
- &storage,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 1);
- EXPECT_TRUE(utils::MonotonicDurationHelper(&fake_system_state,
- &storage,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
-
- // Advance clock two seconds and we should get 2 sec and then 0 sec.
- fake_clock.SetMonotonicTime(base::Time::FromInternalValue(4000000));
- EXPECT_TRUE(utils::MonotonicDurationHelper(&fake_system_state,
- &storage,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 2);
- EXPECT_TRUE(utils::MonotonicDurationHelper(&fake_system_state,
- &storage,
- &duration));
- EXPECT_EQ(duration.InSeconds(), 0);
-}
-
TEST(UtilsTest, GetMinorVersion) {
// Test GetMinorVersion by verifying that it parses the conf file and returns
// the correct value.
diff --git a/metrics.cc b/metrics.cc
index 626897a..b8ebcbf 100644
--- a/metrics.cc
+++ b/metrics.cc
@@ -19,11 +19,13 @@
#include <string>
#include <base/logging.h>
+#include <metrics/metrics_library.h>
#include "update_engine/common/clock_interface.h"
#include "update_engine/common/constants.h"
#include "update_engine/common/prefs_interface.h"
#include "update_engine/common/utils.h"
+#include "update_engine/metrics_utils.h"
#include "update_engine/system_state.h"
using std::string;
@@ -149,9 +151,10 @@
}
base::TimeDelta time_since_last;
- if (utils::WallclockDurationHelper(system_state,
- kPrefsMetricsCheckLastReportingTime,
- &time_since_last)) {
+ if (metrics_utils::WallclockDurationHelper(
+ system_state,
+ kPrefsMetricsCheckLastReportingTime,
+ &time_since_last)) {
metric = kMetricCheckTimeSinceLastCheckMinutes;
LOG(INFO) << "Sending " << utils::FormatTimeDelta(time_since_last)
<< " for metric " << metric;
@@ -165,9 +168,9 @@
base::TimeDelta uptime_since_last;
static int64_t uptime_since_last_storage = 0;
- if (utils::MonotonicDurationHelper(system_state,
- &uptime_since_last_storage,
- &uptime_since_last)) {
+ if (metrics_utils::MonotonicDurationHelper(system_state,
+ &uptime_since_last_storage,
+ &uptime_since_last)) {
metric = kMetricCheckTimeSinceLastCheckUptimeMinutes;
LOG(INFO) << "Sending " << utils::FormatTimeDelta(uptime_since_last)
<< " for metric " << metric;
@@ -308,9 +311,10 @@
}
base::TimeDelta time_since_last;
- if (utils::WallclockDurationHelper(system_state,
- kPrefsMetricsAttemptLastReportingTime,
- &time_since_last)) {
+ if (metrics_utils::WallclockDurationHelper(
+ system_state,
+ kPrefsMetricsAttemptLastReportingTime,
+ &time_since_last)) {
metric = kMetricAttemptTimeSinceLastAttemptMinutes;
LOG(INFO) << "Sending " << utils::FormatTimeDelta(time_since_last)
<< " for metric " << metric;
@@ -324,9 +328,9 @@
static int64_t uptime_since_last_storage = 0;
base::TimeDelta uptime_since_last;
- if (utils::MonotonicDurationHelper(system_state,
- &uptime_since_last_storage,
- &uptime_since_last)) {
+ if (metrics_utils::MonotonicDurationHelper(system_state,
+ &uptime_since_last_storage,
+ &uptime_since_last)) {
metric = kMetricAttemptTimeSinceLastAttemptUptimeMinutes;
LOG(INFO) << "Sending " << utils::FormatTimeDelta(uptime_since_last)
<< " for metric " << metric;
diff --git a/metrics_utils.cc b/metrics_utils.cc
index 354d137..eb99c7d 100644
--- a/metrics_utils.cc
+++ b/metrics_utils.cc
@@ -16,6 +16,17 @@
#include "update_engine/metrics_utils.h"
+#include <string>
+
+#include <base/time/time.h>
+
+#include "update_engine/common/clock_interface.h"
+#include "update_engine/common/prefs_interface.h"
+#include "update_engine/system_state.h"
+
+using base::Time;
+using base::TimeDelta;
+
namespace chromeos_update_engine {
namespace metrics_utils {
@@ -244,5 +255,47 @@
return metrics::ConnectionType::kUnknown;
}
+bool WallclockDurationHelper(SystemState* system_state,
+ const std::string& state_variable_key,
+ TimeDelta* out_duration) {
+ bool ret = false;
+
+ Time now = system_state->clock()->GetWallclockTime();
+ int64_t stored_value;
+ if (system_state->prefs()->GetInt64(state_variable_key, &stored_value)) {
+ Time stored_time = Time::FromInternalValue(stored_value);
+ if (stored_time > now) {
+ LOG(ERROR) << "Stored time-stamp used for " << state_variable_key
+ << " is in the future.";
+ } else {
+ *out_duration = now - stored_time;
+ ret = true;
+ }
+ }
+
+ if (!system_state->prefs()->SetInt64(state_variable_key,
+ now.ToInternalValue())) {
+ LOG(ERROR) << "Error storing time-stamp in " << state_variable_key;
+ }
+
+ return ret;
+}
+
+bool MonotonicDurationHelper(SystemState* system_state,
+ int64_t* storage,
+ TimeDelta* out_duration) {
+ bool ret = false;
+
+ Time now = system_state->clock()->GetMonotonicTime();
+ if (*storage != 0) {
+ Time stored_time = Time::FromInternalValue(*storage);
+ *out_duration = now - stored_time;
+ ret = true;
+ }
+ *storage = now.ToInternalValue();
+
+ return ret;
+}
+
} // namespace metrics_utils
} // namespace chromeos_update_engine
diff --git a/metrics_utils.h b/metrics_utils.h
index 1b8d8ac..7c3b02d 100644
--- a/metrics_utils.h
+++ b/metrics_utils.h
@@ -21,6 +21,9 @@
#include "update_engine/metrics.h"
namespace chromeos_update_engine {
+
+class SystemState;
+
namespace metrics_utils {
// Transforms a ErrorCode value into a metrics::DownloadErrorCode.
@@ -39,6 +42,29 @@
metrics::ConnectionType GetConnectionType(NetworkConnectionType type,
NetworkTethering tethering);
+// This function returns the duration on the wallclock since the last
+// time it was called for the same |state_variable_key| value.
+//
+// If the function returns |true|, the duration (always non-negative)
+// is returned in |out_duration|. If the function returns |false|
+// something went wrong or there was no previous measurement.
+bool WallclockDurationHelper(SystemState* system_state,
+ const std::string& state_variable_key,
+ base::TimeDelta* out_duration);
+
+// This function returns the duration on the monotonic clock since the
+// last time it was called for the same |storage| pointer.
+//
+// You should pass a pointer to a 64-bit integer in |storage| which
+// should be initialized to 0.
+//
+// If the function returns |true|, the duration (always non-negative)
+// is returned in |out_duration|. If the function returns |false|
+// something went wrong or there was no previous measurement.
+bool MonotonicDurationHelper(SystemState* system_state,
+ int64_t* storage,
+ base::TimeDelta* out_duration);
+
} // namespace metrics_utils
} // namespace chromeos_update_engine
diff --git a/metrics_utils_unittest.cc b/metrics_utils_unittest.cc
index 97f0b46..e702c17 100644
--- a/metrics_utils_unittest.cc
+++ b/metrics_utils_unittest.cc
@@ -18,6 +18,10 @@
#include <gtest/gtest.h>
+#include "update_engine/common/fake_clock.h"
+#include "update_engine/common/fake_prefs.h"
+#include "update_engine/fake_system_state.h"
+
namespace chromeos_update_engine {
namespace metrics_utils {
@@ -73,5 +77,134 @@
NetworkTethering::kUnknown));
}
+TEST(MetricsUtilsTest, WallclockDurationHelper) {
+ FakeSystemState fake_system_state;
+ FakeClock fake_clock;
+ base::TimeDelta duration;
+ const std::string state_variable_key = "test-prefs";
+ FakePrefs fake_prefs;
+
+ fake_system_state.set_clock(&fake_clock);
+ fake_system_state.set_prefs(&fake_prefs);
+
+ // Initialize wallclock to 1 sec.
+ fake_clock.SetWallclockTime(base::Time::FromInternalValue(1000000));
+
+ // First time called so no previous measurement available.
+ EXPECT_FALSE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+
+ // Next time, we should get zero since the clock didn't advance.
+ EXPECT_TRUE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+
+ // We can also call it as many times as we want with it being
+ // considered a failure.
+ EXPECT_TRUE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+ EXPECT_TRUE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+
+ // Advance the clock one second, then we should get 1 sec on the
+ // next call and 0 sec on the subsequent call.
+ fake_clock.SetWallclockTime(base::Time::FromInternalValue(2000000));
+ EXPECT_TRUE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 1);
+ EXPECT_TRUE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+
+ // Advance clock two seconds and we should get 2 sec and then 0 sec.
+ fake_clock.SetWallclockTime(base::Time::FromInternalValue(4000000));
+ EXPECT_TRUE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 2);
+ EXPECT_TRUE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+
+ // There's a possibility that the wallclock can go backwards (NTP
+ // adjustments, for example) so check that we properly handle this
+ // case.
+ fake_clock.SetWallclockTime(base::Time::FromInternalValue(3000000));
+ EXPECT_FALSE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ fake_clock.SetWallclockTime(base::Time::FromInternalValue(4000000));
+ EXPECT_TRUE(metrics_utils::WallclockDurationHelper(&fake_system_state,
+ state_variable_key,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 1);
+}
+
+TEST(MetricsUtilsTest, MonotonicDurationHelper) {
+ int64_t storage = 0;
+ FakeSystemState fake_system_state;
+ FakeClock fake_clock;
+ base::TimeDelta duration;
+
+ fake_system_state.set_clock(&fake_clock);
+
+ // Initialize monotonic clock to 1 sec.
+ fake_clock.SetMonotonicTime(base::Time::FromInternalValue(1000000));
+
+ // First time called so no previous measurement available.
+ EXPECT_FALSE(metrics_utils::MonotonicDurationHelper(&fake_system_state,
+ &storage,
+ &duration));
+
+ // Next time, we should get zero since the clock didn't advance.
+ EXPECT_TRUE(metrics_utils::MonotonicDurationHelper(&fake_system_state,
+ &storage,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+
+ // We can also call it as many times as we want with it being
+ // considered a failure.
+ EXPECT_TRUE(metrics_utils::MonotonicDurationHelper(&fake_system_state,
+ &storage,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+ EXPECT_TRUE(metrics_utils::MonotonicDurationHelper(&fake_system_state,
+ &storage,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+
+ // Advance the clock one second, then we should get 1 sec on the
+ // next call and 0 sec on the subsequent call.
+ fake_clock.SetMonotonicTime(base::Time::FromInternalValue(2000000));
+ EXPECT_TRUE(metrics_utils::MonotonicDurationHelper(&fake_system_state,
+ &storage,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 1);
+ EXPECT_TRUE(metrics_utils::MonotonicDurationHelper(&fake_system_state,
+ &storage,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+
+ // Advance clock two seconds and we should get 2 sec and then 0 sec.
+ fake_clock.SetMonotonicTime(base::Time::FromInternalValue(4000000));
+ EXPECT_TRUE(metrics_utils::MonotonicDurationHelper(&fake_system_state,
+ &storage,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 2);
+ EXPECT_TRUE(metrics_utils::MonotonicDurationHelper(&fake_system_state,
+ &storage,
+ &duration));
+ EXPECT_EQ(duration.InSeconds(), 0);
+}
+
} // namespace metrics_utils
} // namespace chromeos_update_engine
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 17aa62b..3e69e96 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -31,6 +31,7 @@
#include <base/strings/stringprintf.h>
#include <base/time/time.h>
#include <expat.h>
+#include <metrics/metrics_library.h>
#include "update_engine/common/action_pipe.h"
#include "update_engine/common/constants.h"
diff --git a/payload_state.cc b/payload_state.cc
index 6b7b0fb..8594f28 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -22,6 +22,7 @@
#include <base/logging.h>
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
+#include <metrics/metrics_library.h>
#include <policy/device_policy.h>
#include "update_engine/common/clock.h"