Support needed to measure user and service traffic in Chrome.

components/data_use_measurement/content/data_use_measurement.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 <components/data_use_measurement/content/data_use_measurement.h>

[bengr, 2015/09/01 20:16:01]

#include "components/.../data_use_measurement.h" and add a blank line after it

[amohammadkhan, 2015/09/01 23:02:42]

Done.

+#include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
+#include "base/strings/stringprintf.h"
+#include "content/public/browser/resource_request_info.h"
+#include "net/base/network_change_notifier.h"
+#include "net/base/upload_data_stream.h"
+#include "net/url_request/url_request.h"
+
+namespace data_use_measurement {
+
+namespace {
+
+// Records the occurrence of |sample| in |name| histogram. Conventional UMA
+// histograms are not used because the |name| is not static.
+void RecordUMAHistogramCount(const std::string& name, int64_t sample) {
+  base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet(
+      name,
+      1,        // Minimum sample size in Bytes.
+      1000000,  // Maximum sample size in Bytes. Should cover most of the
+                // requests by services.
+      50,       // Bucket count.
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram_pointer->Add(sample);
+}
+
+// This function increases the value of |sample| bucket in |name| sparse
+// histogram by |value|. Conventional UMA histograms are not used because the
+// |name| is not static.
+void RecordSparseHistogramWithValue(const std::string& name,
+                                    int64_t sample,
+                                    int64_t value) {
+  base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(
+      name, base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram->AddCount(sample, value);
+}
+
+}  // namespace
+
+DataUseMeasurement::DataUseMeasurement()
+#if defined(OS_ANDROID)
+    : app_state_(base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES),
+      app_listener_(new base::android::ApplicationStatusListener(
+          base::Bind(&DataUseMeasurement::OnApplicationStateChange,
+                     base::Unretained(this))))
+#endif
+{
+}
+
+DataUseMeasurement::~DataUseMeasurement() {
+#if defined(OS_ANDROID)
+  // |app_listener_| is destroyed before DataUseMeasurement instance is
+  // destroyed, so a callback isn't sent between the DataUseMeasurement instance
+  // being destroyed and the |app_listener_| being destroyed.

[bengr, 2015/09/01 20:16:01]

// Prevent notification between the destruction of the DataUseMeasurement
instance and the destruction of |app_listener_|.

[amohammadkhan, 2015/09/01 23:02:42]

Done.

+  app_listener_.reset();
+#endif
+}
+void DataUseMeasurement::ReportDataUseUMA(const net::URLRequest* request) {
+  bool is_user_traffic = false;
+
+  const content::ResourceRequestInfo* info =
+      content::ResourceRequestInfo::ForRequest(request);
+  // Having |info| is the sign of a request for a web content from user. For now
+  // we could add a condition to check ProcessType in info is
+  // content::PROCESS_TYPE_RENDERER, but it won't be compatible with upcoming
+  // PlzNavigate architecture. So just existence of |info| is verified, and the
+  // current check should be compatible with upcoming changes in PlzNavigate.
+  if (info) {

[bengr, 2015/09/01 20:16:01]

bool is_user_traffic = info != nullptr;

[amohammadkhan, 2015/09/01 23:02:43]

Done.

+    is_user_traffic = true;
+  }
+
+  // These number won't be the number of bytes handed to the kernel because

[bengr, 2015/09/01 20:16:01]

// Counts rely on URLRequest::GetTotalReceivedBytes(), which does not include
the send path, network layer overhead, TLS overhead, and DNS.

[amohammadkhan, 2015/09/01 23:02:43]

Done.

+  // session layer framing and compression is not being accounted for.
+  // TODO(amohammadkhan): Make these measured bytes more in line with number of
+  // bytes in lower levels.
+  int64_t request_body_bytes = 0;
+  int64_t request_header_bytes = 0;
+  int64_t total_upload_bytes = 0;
+  if (request->has_upload())
+    request_body_bytes = request->get_upload()->size();
+  net::HttpRequestHeaders request_headers;
+  if (request->GetFullRequestHeaders(&request_headers))
+    request_header_bytes = request_headers.ToString().length();
+  total_upload_bytes = request_body_bytes + request_header_bytes;
+  int64_t total_received_bytes = request->GetTotalReceivedBytes();
+  ReportDataUsageURLRequest(is_user_traffic ? USER_TRAFFIC : SERVICE_TRAFFIC,
+                            UPSTREAM, total_upload_bytes);
+  ReportDataUsageURLRequest(is_user_traffic ? USER_TRAFFIC : SERVICE_TRAFFIC,
+                            DOWNSTREAM, total_received_bytes);
+  DataUseUserData* attached_service_data = reinterpret_cast<DataUseUserData*>(
+      request->GetUserData(DataUseUserData::kUserDataKey));
+
+  if (!is_user_traffic) {
+    data_use_measurement::DataUseUserData::ServiceType service_type =
+        attached_service_data
+            ? attached_service_data->service_type()
+            : data_use_measurement::DataUseUserData::NOT_TAGGED;
+    ReportDataUsage(service_type, UPSTREAM, total_upload_bytes);
+    ReportDataUsage(service_type, DOWNSTREAM, total_received_bytes);
+  }
+}
+
+#if defined(OS_ANDROID)
+void DataUseMeasurement::OnApplicationStateChange(
+    base::android::ApplicationState application_state) {
+  app_state_ = application_state;
+}
+#endif
+
+#if defined(OS_ANDROID)
+void DataUseMeasurement::OnApplicationStateChangeForTesting(
+    base::android::ApplicationState application_state) {
+  app_state_ = application_state;
+}
+#endif
+
+DataUseMeasurement::AppState DataUseMeasurement::CurrentAppState() {
+#if defined(OS_ANDROID)
+  if (app_state_ != base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES)
+    return BACKGROUND;
+#endif
+  // If the OS is not Android, all the requests are considered Foreground.

[bengr, 2015/09/01 20:16:01]

Make sure this is documented in histograms.xml.

[amohammadkhan, 2015/09/01 23:02:42]

Done.

+  return FOREGROUND;
+}
+
+std::string DataUseMeasurement::GetSuffixForHistogramName(

[bengr, 2015/09/01 20:16:01]

The method should be const.

[amohammadkhan, 2015/09/01 23:02:43]

Done.

+    TrafficDirection dir) {
+  AppState app_state = CurrentAppState();
+  bool is_conn_cellular = net::NetworkChangeNotifier::IsConnectionCellular(
+      net::NetworkChangeNotifier::GetConnectionType());
+  return base::StringPrintf(
+      ".%s.%s.%s", dir == UPSTREAM ? "Upstream" : "Downstream",
+      app_state == BACKGROUND ? "Background" : "Foreground",
+      is_conn_cellular ? "Cellular" : "NotCellular");
+}
+
+void DataUseMeasurement::ReportDataUsage(
+    data_use_measurement::DataUseUserData::ServiceType service,
+    TrafficDirection dir,
+    int64_t message_size) {
+  std::string service_histogram_name =
+      base::StringPrintf("DataUse.MessageSize.%s", GetServiceName(service));
+  std::string combined_histogram_name =
+      "DataUse.Services" + GetSuffixForHistogramName(dir);

[bengr, 2015/09/01 20:16:00]

Why does this prefix not end with '.' and the other prefix does? Be consistent.
And make sure your documentation matches.

[amohammadkhan, 2015/09/01 23:02:42]

There is no inconsistency here. The '.' is needed in the first one because
ServiceName is concatenated to it. 

+  RecordUMAHistogramCount(service_histogram_name, message_size);

[bengr, 2015/09/01 20:16:01]

I would get rid of service_histogram_name and replace with
"DataUse.MessageSize." + GetServiceName().

[amohammadkhan, 2015/09/01 23:02:42]

Done.

+  if (message_size > 0) {
+    RecordSparseHistogramWithValue(combined_histogram_name, service,
+                                   message_size);
+  }
+}
+
+void DataUseMeasurement::ReportDataUsageURLRequest(
+    ChromeTrafficType service_type,
+    TrafficDirection dir,
+    int64_t message_size) {
+  std::string histogram_name = base::StringPrintf(
+      "DataUse.%s%s", service_type == USER_TRAFFIC ? "User" : "NotUser",

[bengr, 2015/09/01 20:16:01]

NotUser -> Service?

[amohammadkhan, 2015/09/01 23:02:42]

Yes service may be better. But basically we had a lot of service in histogram
names and I didn't want to add one more. Based on another reviewer's opinion I
changed it "system".

+      GetSuffixForHistogramName(dir).c_str());
+  RecordUMAHistogramCount(histogram_name, message_size);
+}
+
+}  // namespace data_use_measurement