Support needed to measure user and service traffic in Chrome.

chrome/browser/net/data_use_measurement.cc

Index: chrome/browser/net/data_use_measurement.cc
diff --git a/chrome/browser/net/data_use_measurement.cc b/chrome/browser/net/data_use_measurement.cc
new file mode 100644
index 0000000000000000000000000000000000000000..91181333277ef54e143c849a7e0752c552e21128
--- /dev/null
+++ b/chrome/browser/net/data_use_measurement.cc
@@ -0,0 +1,185 @@
+// 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 <string>
+
+#include "base/metrics/histogram.h"
+#include "chrome/browser/net/data_use_measurement.h"
+#include "content/public/browser/resource_request_info.h"
+#include "content/public/common/process_type.h"
+#include "net/base/network_change_notifier.h"
+#include "net/base/upload_data_stream.h"
+#include "net/url_request/url_request.h"
+
+#if defined(OS_ANDROID)
+#include "base/android/application_status_listener.h"
+#endif
+
+// Copied from chromecast/base/metrics/cast_histograms.h

[bengr, 2015/08/07 18:00:01]

Why are you copying code? Add a TODO to unify these macros with those used by
cast_histograms.h

[amohammadkhan, 2015/08/11 22:04:34]

I didn't like copying myself too, but I didn't want to include that file and I
needed exact same functionality, so I copied the code. It was good if we had
these macros as a part of histogram macros but UMA team may not be interested to
add these macros to their header.

+#define STATIC_HISTOGRAM_POINTER_BLOCK_NO_CACHE(                               \
+    constant_histogram_name, histogram_add_method_invocation,                  \
+    histogram_factory_get_invocation)                                          \
+  do {                                                                         \
+    base::HistogramBase* histogram_pointer = histogram_factory_get_invocation; \
+    if (DCHECK_IS_ON())                                                        \
+      histogram_pointer->CheckName(constant_histogram_name);                   \
+    histogram_pointer->histogram_add_method_invocation;                        \
+  } while (0)
+
+// Copied from chromecast/base/metrics/cast_histograms.h
+#define UMA_HISTOGRAM_CUSTOM_COUNTS_NO_CACHE(name, sample, min, max, \
+                                             bucket_count) \
+    STATIC_HISTOGRAM_POINTER_BLOCK_NO_CACHE(name, Add(sample), \
+        base::Histogram::FactoryGet(name, min, max, bucket_count, \
+            base::HistogramBase::kUmaTargetedHistogramFlag))
+
+// NO_CACHE version of UMA_HISTOGRAM_CUSTOM_COUNT is used because the names of

[bengr, 2015/08/07 18:00:00]

I can't parse this comment.

[amohammadkhan, 2015/08/11 22:04:35]

Sorry. I tried to rephrase it. Please let me know if still it is not clear.

+// histograms may change when measurement is done for different services
+// the maximum size of the request is about 5MB which may not be enough for
+// large contents.
+#define TRAFFIC_MEASUREMENT_UMA_HISTOGRAM(name, sample) \
+    UMA_HISTOGRAM_CUSTOM_COUNTS_NO_CACHE(name, sample, 1, 5000000, 50)
+
+namespace {
+
+// Enumerate the direction of data. Specifies the measured data was sent or

[bengr, 2015/08/07 18:00:01]

Rewrite comment:

// Specifies that data is received or sent, respectively.

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+// received data
+enum TrafficDirection {
+  DOWNLOAD,
+  UPLOAD
+};
+
+enum AppState {

[bengr, 2015/08/07 18:00:00]

// The state of the application. Only available on Android.

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+  BACKGROUND,
+  FOREGROUND
+};
+
+enum ConnectionType {

[bengr, 2015/08/07 18:00:01]

The type of network connection that was used.

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+  CELLULAR,
+  WIFI

[bengr, 2015/08/07 18:00:00]

Should this be tri-state: CELLULAR, WIFI, OTHER?

[amohammadkhan, 2015/08/11 22:04:35]

Basically I am checking isCellular, so I changed it to Cellular and
Not_Cellular.

+};
+
+enum ChromeServiceType {

[bengr, 2015/08/07 18:00:01]

The reason a request was sent.

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+  USER_TRAFFIC,
+  NOT_USER_TRAFFIC

[bengr, 2015/08/07 18:00:00]

Maybe all this SERVICE_TRAFFIC.

[amohammadkhan, 2015/08/11 22:04:34]

Done.

+};
+
+#if defined(OS_ANDROID)
+// It seems it is not updated at the beginning, so if we start in background
+// somehow, it might be buggy until an event happens.

[bengr, 2015/08/07 18:00:00]

Don't try to land buggy code. We should know for sure if we are in the
background or foreground.

[amohammadkhan, 2015/08/11 22:04:35]

I think I was over thinking. The Chrome should always start in foreground and it
should not be buggy.

+// These are for just android, do I need to add if here?

[bengr, 2015/08/07 18:00:01]

Strange comment. Is it needed?

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+base::android::ApplicationState app_state =
+    base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES;
+base::android::ApplicationStatusListener* app_listener;
+#endif
+
+bool listener_made = false;

[bengr, 2015/08/07 18:00:00]

Don't use file scope variables. If you have state that needs to persist across
function calls, your best bet is to create a class.

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+
+#if defined(OS_ANDROID)
+void OnApplicationStateChange(
+    base::android::ApplicationState application_state) {
+  app_state = application_state;
+}
+#endif
+
+std::string MakeHistogramName(ChromeServiceType service_type) {

[bengr, 2015/08/07 18:00:00]

Delete this function and see my comment on line 126.

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+  std::string histogram_prefix = "DataUse.";
+  switch (service_type) {
+    case USER_TRAFFIC:
+      return histogram_prefix + "UserData";
+    case NOT_USER_TRAFFIC:
+      return histogram_prefix + "NotUserData";
+  }
+  assert(false);
+  return histogram_prefix;
+}
+
+AppState CurrentAppState() {

[bengr, 2015/08/07 18:00:01]

AppState CurrentAppState() {
#if defined(OS_ANDROID)
  if (app_state_ != base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES)
    return BACKGROUND;
#endif
  return FOREGROUND;
}

[amohammadkhan, 2015/08/11 22:04:35]

I'm gonna use the similar approach to your proposed approach, but I'll separate
the dimension parts because I want to use it in few different parts so it may be
useful to define it as a function.

+#if defined(OS_ANDROID)
+  if (app_state == base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES)

[bengr, 2015/08/07 18:00:00]

Is this really the right way to check?

[amohammadkhan, 2015/08/11 22:04:35]

I think so. At least it is working well in my tests.

+    return FOREGROUND;
+  else
+    return BACKGROUND;
+#endif
+  // If the OS is not Android, all the requests are considered foreground so for
+  // fair comparison between foreground and background data use, -A histograms
+  // should be chosen in UMA.
+  return FOREGROUND;
+}
+
+ConnectionType CurrentConnectionType() {
+  if (net::NetworkChangeNotifier::IsConnectionCellular(
+      net::NetworkChangeNotifier::GetConnectionType())) {
+    return CELLULAR;
+  }
+  return WIFI;
+}
+
+void ReportDataUsage(ChromeServiceType service_type,
+                      TrafficDirection dir, int message_size) {
+  AppState appState = CurrentAppState();
+  ConnectionType connType = CurrentConnectionType();
+  std::string first_suffix = "";
+  std::string second_suffix = "";
+  std::string third_suffix = "";
+  if (dir == UPLOAD) {

[bengr, 2015/08/07 18:00:01]

Remove curly braces. Also, I think you have the direction backwards. Just do:

std::string full_histrogram_name = StringPrintf("DataUse.%s.%s.%s.%s",
    service_type == USER_TRAFFIC ? "UserData" : "ServiceData",
    dir == UPLOAD ? "Upload" : "Download",
    CurrentAppState() == BACKGROUND ? "Background" : "Foreground",
    CurrentConnectionType() == WIFI ? "WiFi" : "Cell";

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+    first_suffix = ".Download";
+  } else {
+    first_suffix = ".Upload";
+  }
+  if (appState == BACKGROUND) {
+    second_suffix = ".Background";
+  } else {
+    second_suffix = ".Foreground";
+  }
+  if (connType == WIFI) {
+    third_suffix = ".Wifi";
+  } else {
+    third_suffix = ".Cell";
+  }
+  std::string histogram_name = MakeHistogramName(service_type);
+  std::string final_histogram_name = histogram_name + first_suffix +
+                                     second_suffix + third_suffix;
+  TRAFFIC_MEASUREMENT_UMA_HISTOGRAM(final_histogram_name, message_size);
+}
+
+} // namespace
+
+void ReportDataUsage(const net::URLRequest* request) {
+#if defined(OS_ANDROID)
+  if (!listener_made) {

[bengr, 2015/08/07 18:00:01]

What does listener_made mean?

[amohammadkhan, 2015/08/11 22:04:35]

Removed in new version.

+    listener_made = true;
+    app_listener = new base::android::ApplicationStatusListener(
+        base::Bind(&OnApplicationStateChange));
+  }
+#endif
+  bool renderer_flag = false;
+  const content::ResourceRequestInfo* info =
+        content::ResourceRequestInfo::ForRequest(request);
+  if (!info) {

[bengr, 2015/08/07 18:00:01]

Do:

// All data from a renderer process is assumed to be user data.
if (info && info->GetProcessType() == content::PROCESS_TYPE_RENDERER)
  renderer_flag = true;

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+    // Requests without info are categorized as NOT_USER_TRAFFIC data
+  } else if (info->GetProcessType() == content::PROCESS_TYPE_RENDERER) {
+    // We assume all data from renderer process are USER_DATA
+    renderer_flag = true;
+  }
+  int request_body_bytes = 0;
+  int request_header_bytes = 0;

[bengr, 2015/08/07 18:00:01]

Add a comment explaining that these won't be the number of bytes handed to the
kernel because session layer framing and compression is not being accounted for.
Then add a TODO to fix.

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+  int total_upload_bytes = 0;
+  if (request->has_upload()){
+    request_body_bytes = request->get_upload()->size();
+  }
+  net::HttpRequestHeaders request_headers;
+  if (request->GetFullRequestHeaders(&request_headers)) {

[bengr, 2015/08/07 18:00:01]

Remove curly braces.

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+    request_header_bytes = request_headers.ToString().length();
+  }
+  total_upload_bytes =  request_body_bytes + request_header_bytes;

[bengr, 2015/08/07 18:00:00]

Remove extra space

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+  int total_received_bytes = request->GetTotalReceivedBytes();
+  if (renderer_flag) {

[bengr, 2015/08/07 18:00:00]

Instead of having two cases, do:
ReportDataUsage(renderer_flag ? USER_TRAFFIC : SERVICE_TRAFFIC, UPLOAD,
total_upload_bytes);

[amohammadkhan, 2015/08/11 22:04:35]

Done.

+    ReportDataUsage(USER_TRAFFIC, UPLOAD, total_upload_bytes);
+    ReportDataUsage(USER_TRAFFIC, DOWNLOAD, total_received_bytes);
+  } else {
+    ReportDataUsage(NOT_USER_TRAFFIC, UPLOAD, total_upload_bytes);
+    ReportDataUsage(NOT_USER_TRAFFIC, DOWNLOAD, total_received_bytes);
+  }
+}