Add histograms for ExternalDataUseObserver

chrome/browser/android/data_usage/external_data_use_observer.cc

 // Copyright 2015 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 "chrome/browser/android/data_usage/external_data_use_observer.h"
 
 #include <utility>
 
 #include "base/android/context_utils.h"
 #include "base/android/jni_string.h"
 #include "base/containers/hash_tables.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/android/data_usage/data_use_tab_model.h"
 #include "components/data_usage/core/data_use.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "jni/ExternalDataUseObserver_jni.h"
 #include "third_party/re2/re2/re2.h"
 #include "url/gurl.h"
 
 using base::android::ConvertUTF8ToJavaString;
 using base::android::ToJavaArrayOfStrings;
 
 namespace {
 
+// Result of data usage report submission.
+enum DataUsageReportSubmissionResult {
+  // Submission of data use report to the external observer was successful.
+  DATAUSAGE_REPORT_SUBMISSION_SUCCESSFUL = 0,
+  // Submission of data use report to the external observer returned error.
+  DATAUSAGE_REPORT_SUBMISSION_FAILED = 1,
+  // Submission of data use report to the external observer timed out.
+  DATAUSAGE_REPORT_SUBMISSION_TIMED_OUT = 2,
+  // Data use report was lost before an attempt was made to submit it.
+  DATAUSAGE_REPORT_SUBMISSION_LOST = 3,
+  DATAUSAGE_REPORT_SUBMISSION_BOUNDARY = 4
+};
+
+// Record the result of data use report submission. |bytes| is the sum of send
+// and received bytes in the report.
+void RecordDataUsageReportSubmission(DataUsageReportSubmissionResult result,
+                                     int64_t bytes) {

[sclittle, 2015/12/07 21:44:08]

nit: just for clarity, could you add a DCHECK_LE(0, bytes)?

[tbansal1, 2015/12/08 00:08:16]

Done.

+  UMA_HISTOGRAM_ENUMERATION("DataUsage.ReportSubmissionResult", result,
+                            DATAUSAGE_REPORT_SUBMISSION_BOUNDARY);
+  // Cap to the maximum sample value.
+  const int32_t bytes_capped = bytes <= base::HistogramBase::kSampleType_MAX - 1

[sclittle, 2015/12/07 21:44:08]

optional nit: You could use std::min here.

[tbansal1, 2015/12/08 00:08:16]

I tried that before but it does not work because one of them is 32-bit, other is
64-bit. I can use static_cast with std::min but that does not look much better
either.

+                                   ? bytes
+                                   : base::HistogramBase::kSampleType_MAX - 1;
+  switch (result) {
+    case DATAUSAGE_REPORT_SUBMISSION_SUCCESSFUL:
+      UMA_HISTOGRAM_COUNTS("DataUsage.ReportSubmission.Successful.Bytes",
+                           bytes_capped);
+      break;
+    case DATAUSAGE_REPORT_SUBMISSION_FAILED:
+      UMA_HISTOGRAM_COUNTS("DataUsage.ReportSubmission.Failed.Bytes",
+                           bytes_capped);
+      break;
+    case DATAUSAGE_REPORT_SUBMISSION_TIMED_OUT:
+      UMA_HISTOGRAM_COUNTS("DataUsage.ReportSubmission.TimedOut.Bytes",
+                           bytes_capped);
+      break;
+    case DATAUSAGE_REPORT_SUBMISSION_LOST:
+      UMA_HISTOGRAM_COUNTS("DataUsage.ReportSubmission.Lost.Bytes",
+                           bytes_capped);
+      break;
+    default:
+      NOTIMPLEMENTED();
+      break;
+  }
+}
+
 // Default duration after which matching rules are periodically fetched. May be
 // overridden by the field trial.
 const int kDefaultFetchMatchingRulesDurationSeconds = 60 * 15;  // 15 minutes.
 
+// Default duration after which a pending data use report is considered timed
+// out. May be overridden by the field trial.
+const int kDefaultDataUseReportSubmitTimeoutMsec = 60 * 2 * 1000;  // 2 minutes.
+
 // Default value of the minimum number of bytes that should be buffered before
 // a data use report is submitted. May be overridden by the field trial.
 const int64_t kDefaultDataUseReportMinBytes = 100 * 1024;  // 100 KB.
 
 // Populates various parameters from the values specified in the field trial.
 int32_t GetFetchMatchingRulesDurationSeconds() {
   int32_t duration_seconds = -1;
-  std::string variation_value = variations::GetVariationParamValue(
+  const std::string variation_value = variations::GetVariationParamValue(
       chrome::android::ExternalDataUseObserver::
           kExternalDataUseObserverFieldTrial,
       "fetch_matching_rules_duration_seconds");
   if (!variation_value.empty() &&
       base::StringToInt(variation_value, &duration_seconds)) {
     DCHECK_LE(0, duration_seconds);
     return duration_seconds;
   }
   return kDefaultFetchMatchingRulesDurationSeconds;
 }
 
 // Populates various parameters from the values specified in the field trial.
+int32_t GetDataReportSubmitTimeoutMsec() {
+  int32_t duration_seconds = -1;
+  const std::string variation_value = variations::GetVariationParamValue(
+      chrome::android::ExternalDataUseObserver::
+          kExternalDataUseObserverFieldTrial,
+      "data_report_submit_timeout_msec");
+  if (!variation_value.empty() &&
+      base::StringToInt(variation_value, &duration_seconds)) {
+    DCHECK_LE(0, duration_seconds);
+    return duration_seconds;
+  }
+  return kDefaultDataUseReportSubmitTimeoutMsec;
+}
+
+// Populates various parameters from the values specified in the field trial.
 int64_t GetMinBytes() {
   int64_t min_bytes = -1;
-  std::string variation_value = variations::GetVariationParamValue(
+  const std::string variation_value = variations::GetVariationParamValue(
       chrome::android::ExternalDataUseObserver::
           kExternalDataUseObserverFieldTrial,
       "data_use_report_min_bytes");
   if (!variation_value.empty() &&
       base::StringToInt64(variation_value, &min_bytes)) {
     DCHECK_LE(0, min_bytes);
     return min_bytes;
   }
   return kDefaultDataUseReportMinBytes;
 }
@@ skipping 10 matching lines @@
 
 // static
 const size_t ExternalDataUseObserver::kMaxBufferSize = 100;
 
 ExternalDataUseObserver::ExternalDataUseObserver(
     data_usage::DataUseAggregator* data_use_aggregator,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
     : data_use_aggregator_(data_use_aggregator),
       matching_rules_fetch_pending_(false),
-      submit_data_report_pending_(false),
+      last_data_report_submitted_ticks_(base::TimeTicks()),
+      pending_report_bytes_(0),
       registered_as_observer_(false),
       io_task_runner_(io_task_runner),
       ui_task_runner_(ui_task_runner),
       previous_report_time_(base::Time::Now()),
       last_matching_rules_fetch_time_(base::TimeTicks::Now()),
       total_bytes_buffered_(0),
       fetch_matching_rules_duration_(
           base::TimeDelta::FromSeconds(GetFetchMatchingRulesDurationSeconds())),
       data_use_report_min_bytes_(GetMinBytes()),
+      data_report_submit_timeout_(
+          base::TimeDelta::FromMilliseconds(GetDataReportSubmitTimeoutMsec())),
       io_weak_factory_(this),
       ui_weak_factory_(this) {
   DCHECK(data_use_aggregator_);
   DCHECK(io_task_runner_);
   DCHECK(ui_task_runner_);
+  DCHECK(last_data_report_submitted_ticks_.is_null());
 
   ui_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&ExternalDataUseObserver::CreateJavaObjectOnUIThread,
                  GetUIWeakPtr()));
 
   ui_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&ExternalDataUseObserver::FetchMatchingRulesOnUIThread,
                  GetUIWeakPtr()));
@@ skipping 88 matching lines @@
 base::WeakPtr<ExternalDataUseObserver> ExternalDataUseObserver::GetIOWeakPtr() {
   return io_weak_factory_.GetWeakPtr();
 }
 
 base::WeakPtr<ExternalDataUseObserver> ExternalDataUseObserver::GetUIWeakPtr() {
   return ui_weak_factory_.GetWeakPtr();
 }
 
 void ExternalDataUseObserver::OnReportDataUseDoneOnIOThread(bool success) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(submit_data_report_pending_);
+  DCHECK(!last_data_report_submitted_ticks_.is_null());
 
-  // TODO(tbansal): If not successful, record UMA.
+  if (success) {
+    RecordDataUsageReportSubmission(DATAUSAGE_REPORT_SUBMISSION_SUCCESSFUL,
+                                    pending_report_bytes_);
+  } else {
+    RecordDataUsageReportSubmission(DATAUSAGE_REPORT_SUBMISSION_FAILED,
+                                    pending_report_bytes_);
+  }
 
-  submit_data_report_pending_ = false;
+  last_data_report_submitted_ticks_ = base::TimeTicks();
+  pending_report_bytes_ = 0;
 
   SubmitBufferedDataUseReport();
 }
 
 void ExternalDataUseObserver::OnDataUse(const data_usage::DataUse& data_use) {
   DCHECK(thread_checker_.CalledOnValidThread());
   const base::TimeTicks now_ticks = base::TimeTicks::Now();
   const base::Time now_time = base::Time::Now();
 
   // If the time when the matching rules were last fetched is more than
@@ skipping 24 matching lines @@
 
 void ExternalDataUseObserver::BufferDataUseReport(
     const data_usage::DataUse& data_use,
     const std::string& label,
     const base::Time& start_time,
     const base::Time& end_time) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!label.empty());
   DCHECK_LE(0, data_use.rx_bytes);
   DCHECK_LE(0, data_use.tx_bytes);
-  if (data_use.rx_bytes < 0 || data_use.tx_bytes < 0)
+  if (data_use.rx_bytes < 0 || data_use.tx_bytes < 0 ||

[sclittle, 2015/12/07 21:44:08]

nit: The < 0 checks here aren't necessary, and disallowed by the style guide
since you already DCHECK these conditions above.

See
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

[tbansal1, 2015/12/08 00:08:16]

Right, removed.

+      (data_use.rx_bytes == 0 && data_use.tx_bytes == 0)) {
     return;
+  }
 
   DataUseReportKey data_use_report_key =
       DataUseReportKey(label, data_use.connection_type, data_use.mcc_mnc);
 
   DataUseReport report =
       DataUseReport(start_time, end_time, data_use.rx_bytes, data_use.tx_bytes);
 
   // Check if the |data_use_report_key| is already in the buffered reports.
   DataUseReports::iterator it =
       buffered_data_reports_.find(data_use_report_key);
   if (it == buffered_data_reports_.end()) {
     // Limit the buffer size.
     if (buffered_data_reports_.size() == kMaxBufferSize) {
-      // TODO(tbansal): Add UMA to track impact of lost reports.
+      RecordDataUsageReportSubmission(DATAUSAGE_REPORT_SUBMISSION_LOST,
+                                      data_use.rx_bytes + data_use.tx_bytes);
       return;
     }
     buffered_data_reports_.insert(std::make_pair(data_use_report_key, report));
   } else {
     DataUseReport existing_report = DataUseReport(it->second);
     DataUseReport merged_report = DataUseReport(
         std::min(existing_report.start_time, report.start_time),
         std::max(existing_report.end_time, report.end_time),
         existing_report.bytes_downloaded + report.bytes_downloaded,
         existing_report.bytes_uploaded + report.bytes_uploaded);
     buffered_data_reports_.erase(it);
     buffered_data_reports_.insert(
         std::make_pair(data_use_report_key, merged_report));
   }
   total_bytes_buffered_ += (data_use.rx_bytes + data_use.tx_bytes);
 
+  DCHECK_LT(0U, buffered_data_reports_.size());
   DCHECK_LE(buffered_data_reports_.size(), kMaxBufferSize);
 }
 
 void ExternalDataUseObserver::SubmitBufferedDataUseReport() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (submit_data_report_pending_ || buffered_data_reports_.empty())
+  const base::TimeTicks ticks_now = base::TimeTicks::Now();
+
+  // Return if a data use report has been pending for less than
+  // |data_report_submit_timeout_| duration.
+  if (!last_data_report_submitted_ticks_.is_null() &&
+      ticks_now - last_data_report_submitted_ticks_ <
+          data_report_submit_timeout_) {
+    return;
+  }
+
+  if (buffered_data_reports_.empty())
     return;
 
   if (total_bytes_buffered_ < data_use_report_min_bytes_)
     return;
 
+  if (!last_data_report_submitted_ticks_.is_null()) {

[sclittle, 2015/12/07 21:44:08]

nit: add a comment like "Cancel a pending DataUsage report if it has timed out"
or something

[tbansal1, 2015/12/08 00:08:16]

Done.

+    RecordDataUsageReportSubmission(DATAUSAGE_REPORT_SUBMISSION_TIMED_OUT,
+                                    pending_report_bytes_);
+    pending_report_bytes_ = 0;
+    last_data_report_submitted_ticks_ = base::TimeTicks();
+  }
+
   // Send one data use report.
   DataUseReports::iterator it = buffered_data_reports_.begin();
   DataUseReportKey key = it->first;
   DataUseReport report = it->second;
 
+  DCHECK_EQ(0, pending_report_bytes_);
+  DCHECK(last_data_report_submitted_ticks_.is_null());
+  pending_report_bytes_ = report.bytes_downloaded + report.bytes_uploaded;
+  last_data_report_submitted_ticks_ = ticks_now;
+
   // Remove the entry from the map.
   buffered_data_reports_.erase(it);
   total_bytes_buffered_ -= (report.bytes_downloaded + report.bytes_uploaded);
 
-  submit_data_report_pending_ = true;
-
   ui_task_runner_->PostTask(
       FROM_HERE, base::Bind(&ExternalDataUseObserver::ReportDataUseOnUIThread,
                             GetIOWeakPtr(), key, report));
 }
 
 void ExternalDataUseObserver::ReportDataUseOnUIThread(
     const DataUseReportKey& key,
     const DataUseReport& report) const {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   JNIEnv* env = base::android::AttachCurrentThread();
@@ skipping 162 matching lines @@
   return label_;
 }
 
 bool RegisterExternalDataUseObserver(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace android
 
 }  // namespace chrome