Implement UMA log throttling for cellular connections

components/metrics/data_use_tracker.cc

Index: components/metrics/data_use_tracker.cc
diff --git a/components/metrics/data_use_tracker.cc b/components/metrics/data_use_tracker.cc
new file mode 100644
index 0000000000000000000000000000000000000000..94e7a2e543e6d4e5d283d70bd48152cf6cd1e447
--- /dev/null
+++ b/components/metrics/data_use_tracker.cc
@@ -0,0 +1,185 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/metrics/data_use_tracker.h"
+
+#include <string>
+
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "components/metrics/metrics_pref_names.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "components/variations/variations_associated_data.h"
+
+namespace metrics {
+
+namespace {
+void UpdateMetricsUsagePrefs(

[Alexei Svitkine (slow), 2016/03/31 20:00:39]

Add a 1-line comment.

Also, put empty lines within the namespace.

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

Done.

+    const UpdateUsagePrefCallbackType& update_on_ui_callback,
+    scoped_refptr<base::SequencedTaskRunner> ui_task_runner,
+    const std::string& service_name,
+    int message_size) {
+  ui_task_runner->PostTask(
+      FROM_HERE, base::Bind(update_on_ui_callback, service_name, message_size));
+}
+}  // namespace
+
+DataUseTracker::DataUseTracker(PrefService* local_state)
+    : local_state_(local_state),
+      uma_quota_for_testing_(0),
+      uma_ratio_for_testing_(0),
+      weak_ptr_factory_(this) {}
+
+DataUseTracker::~DataUseTracker() {}
+
+// static
+void DataUseTracker::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterDictionaryPref(metrics::prefs::kUserCellDataUse);
+  registry->RegisterDictionaryPref(metrics::prefs::kUmaCellDataUse);
+}
+
+UpdateUsagePrefCallbackType DataUseTracker::GetDataUseForwardingCallback(
+    scoped_refptr<base::SequencedTaskRunner> ui_task_runner) {
+  DCHECK(ui_task_runner->RunsTasksOnCurrentThread());
+  return base::Bind(
+      &UpdateMetricsUsagePrefs,
+      base::Bind(&DataUseTracker::UpdateMetricsUsagePrefsOnUIThread,
+                 weak_ptr_factory_.GetWeakPtr()),
+      ui_task_runner);
+}
+
+bool DataUseTracker::CanUploadUMALog(int log_bytes) {
+  RemoveExpiredEntries();

[Alexei Svitkine (slow), 2016/03/31 20:00:39]

Nit: Add a blank line below.

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

Done.

+  int uma_total_data_use = ComputeTotalDataUse(prefs::kUmaCellDataUse);
+  int uma_quota;
+  bool is_quota_specified = GetUmaQuota(&uma_quota);

[Alexei Svitkine (slow), 2016/03/31 20:00:39]

Nit: Inline this into the if below and have a separate if for the math. When you
have a separate if for the math, you can move line 54 to just be above that if.

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

I am not sure I got it right.

+  if (!is_quota_specified || log_bytes + uma_total_data_use <= uma_quota)
+    return true;
+
+  double uma_ratio;
+  bool is_ratio_specified = GetUmaRatio(&uma_ratio);

[Alexei Svitkine (slow), 2016/03/31 20:00:39]

Nit: Just inline this into the if below.

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

Done.

+  if (!is_ratio_specified)
+    return true;
+
+  int user_total_data_use = ComputeTotalDataUse(prefs::kUserCellDataUse);
+  return (log_bytes + uma_total_data_use) /
+             (double)(log_bytes + user_total_data_use) <=

[Alexei Svitkine (slow), 2016/03/31 20:00:39]

static_cast<double>

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

Done.

+         uma_ratio;
+}
+
+void DataUseTracker::UpdateMetricsUsagePrefsOnUIThread(
+    const std::string& service_name,
+    int message_size) {

[Alexei Svitkine (slow), 2016/03/31 20:00:39]

Add a ThreadChecker member in this class and DCHECK on it here and in the other
non-static methods.

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

Done.

+  UpdateUsagePref(prefs::kUserCellDataUse, message_size);
+  if (service_name == "UMA")
+    UpdateUsagePref(prefs::kUmaCellDataUse, message_size);
+}
+
+void DataUseTracker::UpdateUsagePref(const std::string& pref_name,
+                                     int message_size) {
+  DictionaryPrefUpdate pref_updater(local_state_, pref_name);
+  int todays_traffic = 0;
+  std::string todays_key = GetCurrentMeasurementDate();
+  local_state_->GetDictionary(pref_name)->GetInteger(todays_key,

[Alexei Svitkine (slow), 2016/03/31 20:00:39]

Is there any chance local_state_->GetDictionary(pref_name) is null? I notice you
check for it in the function below.

Also, please add a comment below about what cases it can be null in.

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

added the check

+                                                     &todays_traffic);
+  pref_updater->SetInteger(todays_key, todays_traffic + message_size);
+}
+
+void DataUseTracker::RemoveExpiredEntries() {
+  RemoveExpiredEntriesForPref(prefs::kUmaCellDataUse);
+  RemoveExpiredEntriesForPref(prefs::kUserCellDataUse);
+}
+
+void DataUseTracker::RemoveExpiredEntriesForPref(const std::string& pref_name) {
+  const base::DictionaryValue* user_pref_dict =
+      local_state_->GetDictionary(pref_name);
+  if (!user_pref_dict)
+    return;
+
+  base::DictionaryValue user_pref_new_dict;
+  for (base::DictionaryValue::Iterator it(*user_pref_dict); !it.IsAtEnd();
+       it.Advance()) {
+    base::Time current_date;
+    if (!date_for_testing_.empty())
+      base::Time::FromUTCString(date_for_testing_.c_str(), &current_date);
+    else
+      current_date = base::Time::Now().LocalMidnight();
+
+    base::Time week_ago = current_date - base::TimeDelta::FromDays(7);
+    base::Time key_date;
+    base::Time::FromUTCString(it.key().c_str(), &key_date);
+
+    if (key_date > week_ago)

[Alexei Svitkine (slow), 2016/03/31 20:59:31]

Can you just add this logic to ComputeTotalDataUse() and remove the need for the
extra function?

i.e. it can sum up the ones that are in range and remove the others.

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

I guess I can do that, but then I need to name the function something like
FilterAndComputeTotalDataUse
and I cannot test them separately anymore

[Alexei Svitkine (slow), 2016/03/31 22:18:02]

Okay, keeping them separate for tests makes sense.

+      user_pref_new_dict.Set(it.key(), it.value().CreateDeepCopy());
+  }
+  local_state_->Set(pref_name, user_pref_new_dict);
+}
+
+std::string DataUseTracker::GetCurrentMeasurementDate() {
+  if (!date_for_testing_.empty())
+    return date_for_testing_;
+
+  base::Time::Exploded today_exploded;
+  base::Time::Now().LocalMidnight().LocalExplode(&today_exploded);
+  return base::StringPrintf("%04d-%02d-%02d", today_exploded.year,
+                            today_exploded.month, today_exploded.day_of_month);
+}
+
+int DataUseTracker::ComputeTotalDataUse(std::string pref_name) {
+  int total_data_use = 0;
+  const base::DictionaryValue* pref_dict =
+      local_state_->GetDictionary(pref_name);
+  if (!pref_dict)
+    return total_data_use;
+  for (base::DictionaryValue::Iterator it(*pref_dict); !it.IsAtEnd();
+       it.Advance()) {
+    int value = 0;
+    it.value().GetAsInteger(&value);
+    total_data_use += value;
+  }
+  return total_data_use;
+}
+
+bool DataUseTracker::GetUmaQuota(int* quota) {
+  if (uma_quota_for_testing_ > 0) {
+    *quota = uma_quota_for_testing_;
+    return true;
+  }
+
+  std::string param_value_str = variations::GetVariationParamValue(
+      "UMA_EnableCellularLogUpload", "Uma_Quota");
+  if (param_value_str.empty())
+    return false;
+
+  base::StringToInt(param_value_str, quota);
+  return true;
+}
+
+bool DataUseTracker::GetUmaRatio(double* ratio) {
+  if (uma_ratio_for_testing_ > 0) {
+    *ratio = uma_ratio_for_testing_;
+    return true;
+  }
+
+  std::string param_value_str = variations::GetVariationParamValue(
+      "UMA_EnableCellularLogUpload", "Uma_Ratio");
+  if (param_value_str.empty())
+    return false;
+  base::StringToDouble(param_value_str, ratio);
+  return true;
+}
+
+void DataUseTracker::SetMeasurementDateForTesting(const std::string& date) {

[Alexei Svitkine (slow), 2016/03/31 20:00:39]

Instead of these setters, I suggest just making the Get*() methods virtual and
overriding them in a Test subclass of DataUseTracker. Then, you can simplify
this code a lot (no need to have the members and the ifs.

[gayane -on leave until 09-2017, 2016/03/31 21:55:09]

Done.

+  date_for_testing_ = date;
+}
+
+void DataUseTracker::SetUmaQuotaForTesting(int uma_quota) {
+  uma_quota_for_testing_ = uma_quota;
+}
+
+void DataUseTracker::SetUmaRatioForTesting(double uma_ratio) {
+  uma_ratio_for_testing_ = uma_ratio;
+}
+
+}  // namespace metrics