Add log date to the metrics log

components/metrics/persisted_logs.cc

Index: components/metrics/persisted_logs.cc
diff --git a/components/metrics/persisted_logs.cc b/components/metrics/persisted_logs.cc
index 52292da55ccd5d18f99e30898f2424502e8163ef..d4e5e3dbcdabf674019beb9ac46da31cf5defcc9 100644
--- a/components/metrics/persisted_logs.cc
+++ b/components/metrics/persisted_logs.cc
@@ -18,6 +18,9 @@
 namespace metrics {
 
 namespace {

[Alexei Svitkine (slow), 2016/09/28 18:50:30]

Nit: add line after

[gayane -on leave until 09-2017, 2016/09/28 20:50:50]

Done.

+const char kLogHashKey[] = "hash";
+const char kLogTimestampKey[] = "timestamp";
+const char kLogDataKey[] = "data";
 
 PersistedLogs::LogReadStatus MakeRecallStatusHistogram(
     PersistedLogs::LogReadStatus status) {
@@ -37,16 +40,21 @@ bool ReadBase64String(const base::ListValue& list_value,
   return base::Base64Decode(base64_result, result);
 }
 
-// Base64-encodes |str| and appends the result to |list_value|.
-void AppendBase64String(const std::string& str, base::ListValue* list_value) {
-  std::string base64_str;
-  base::Base64Encode(str, &base64_str);
-  list_value->AppendString(base64_str);
+std::string EncodeToBase64(const std::string& to_convert) {
+  std::string base64_result;
+  base::Base64Encode(to_convert, &base64_result);
+  return base64_result;
+}
+
+std::string DecodeFromBase64(const std::string& to_convert) {
+  std::string result;
+  base::Base64Decode(to_convert, &result);
+  return result;
 }
 
 }  // namespace
 
-void PersistedLogs::LogHashPair::Init(const std::string& log_data) {
+void PersistedLogs::LogInfo::Init(const std::string& log_data) {
   DCHECK(!log_data.empty());
 
   if (!compression::GzipCompress(log_data, &compressed_log_data)) {
@@ -59,6 +67,7 @@ void PersistedLogs::LogHashPair::Init(const std::string& log_data) {
       static_cast<int>(100 * compressed_log_data.size() / log_data.size()));
 
   hash = base::SHA1HashString(log_data);
+  timestamp = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds();

[Alexei Svitkine (slow), 2016/09/28 18:50:30]

Isn't return value int64_t but your field is an int?

[gayane -on leave until 09-2017, 2016/09/28 20:50:49]

hmm right. I had a look and dictionaries have only SetInteger and SetDouble and
no long. 
i calculated that 100 years in seconds will fit in 32bits

[Alexei Svitkine (slow), 2016/09/29 16:46:40]

That's a 100 years since unix epoch? So 56 years at this point?

I looked at how we store other timestamps and looks like we just use strings and
store the TimeT of the value.

e.g.

    local_state_->SetInt64(prefs::kInstallDate,
                           client_info_backup->installation_date != 0
                               ? client_info_backup->installation_date
                               : now.ToTimeT());

Which just ends up being:

  SetUserPrefValue(path, new base::StringValue(base::Int64ToString(value)));

So let's just register this as a string pref and use base::Int64ToString().

 }
 
 PersistedLogs::PersistedLogs(PrefService* local_state,
@@ -89,7 +98,7 @@ PersistedLogs::LogReadStatus PersistedLogs::DeserializeLogs() {
 }
 
 void PersistedLogs::StoreLog(const std::string& log_data) {
-  list_.push_back(LogHashPair());
+  list_.push_back(LogInfo());
   list_.back().Init(log_data);
 }
 
@@ -109,7 +118,46 @@ void PersistedLogs::DiscardStagedLog() {
   staged_log_index_ = -1;
 }
 
-void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) const {
+void PersistedLogs::MigrateFromOldFormat(const char* old_pref_name) {
+  ReadLogsFromPrefList_OldFormat(*local_state_->GetList(old_pref_name));
+  ListPrefUpdate update(local_state_, pref_name_);
+  WriteLogsToPrefList(update.Get());

[Alexei Svitkine (slow), 2016/09/28 18:50:30]

I don't see where WriteLogsToPrefList is implemented?

[gayane -on leave until 09-2017, 2016/09/28 20:50:50]

opps deleted that instead of WriteLogsToPrefList_OldFormat

+}

[Alexei Svitkine (slow), 2016/09/28 18:50:30]

I'm not sure we need an explicit migrate function. Just have the write function
always write in new format and things will migrate themselves whenever it's
updated.

[gayane -on leave until 09-2017, 2016/09/28 20:50:49]

But then when we read we need to read from two different prefs in two different
ways. is it preferable ? 

[Alexei Svitkine (slow), 2016/09/29 16:46:40]

Well, you're already reading from both prefs - just one of them is done in the
migrate function... so I don't really see the difference. I guess we need a tiny
bit of extra logic (if statement) about which pref to read from, but that should
be minimal.

We won't ever have data in both at the same time - since I think the full data
is written at once when writing (and so when writing in the new way, we should
clear the old pref with a todo). So then the read function just needs to have an
"if has old data, return readold();" which can have a todo to be removed when we
migrate.

+
+PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList(
+    const base::ListValue& list_value) {
+  if (list_value.empty())
+    return MakeRecallStatusHistogram(LIST_EMPTY);
+
+  const size_t log_count = list_value.GetSize();
+
+  DCHECK(list_.empty());
+  list_.resize(log_count);
+
+  for (size_t i = 0; i < log_count; ++i) {
+    const base::DictionaryValue* dict;
+    if (!list_value.GetDictionary(i, &dict) ||
+        !dict->GetString(kLogDataKey, &list_[i].compressed_log_data) ||
+        !dict->GetString(kLogHashKey, &list_[i].hash)) {
+      list_.clear();
+      return MakeRecallStatusHistogram(LOG_STRING_CORRUPTION);
+    }
+
+    list_[i].compressed_log_data =
+        DecodeFromBase64(list_[i].compressed_log_data);
+    list_[i].hash = DecodeFromBase64(list_[i].hash);
+    // Ignoring the success of this step as timestamp might not be there for
+    // older logs.
+    // NOTE: Should be added to the check with other fields once migration is
+    // over.
+    dict->GetInteger(kLogTimestampKey, &list_[i].timestamp);
+  }
+
+  return MakeRecallStatusHistogram(RECALL_SUCCESS);
+}
+
+void PersistedLogs::WriteLogsToPrefList_OldFormat(

[Alexei Svitkine (slow), 2016/09/28 18:50:30]

Do we need this anymore?

[gayane -on leave until 09-2017, 2016/09/28 20:50:49]

Nope

+    base::ListValue* list_value) const {
   list_value->Clear();
 
   // Keep the most recent logs which are smaller than |max_log_size_|.
@@ -147,7 +195,7 @@ void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) const {
     UMA_HISTOGRAM_COUNTS("UMA.UnsentLogs.Dropped", dropped_logs_num);
 }
 
-PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList(
+PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList_OldFormat(

[Alexei Svitkine (slow), 2016/09/28 18:50:30]

Nit: ReadLogsFromOldFormatPrefList

[gayane -on leave until 09-2017, 2016/09/28 20:50:49]

Done.

     const base::ListValue& list_value) {
   if (list_value.empty())
     return MakeRecallStatusHistogram(LIST_EMPTY);