Network traffic annotation added to NetMetricsLogUploader.

components/metrics/net/net_metrics_log_uploader.cc

Index: components/metrics/net/net_metrics_log_uploader.cc
diff --git a/components/metrics/net/net_metrics_log_uploader.cc b/components/metrics/net/net_metrics_log_uploader.cc
index 0064177a58c8991cec99004971d86c6df20d1231..c9cd9f66060f36f75c06ce5af78a4201db22e688 100644
--- a/components/metrics/net/net_metrics_log_uploader.cc
+++ b/components/metrics/net/net_metrics_log_uploader.cc
@@ -8,6 +8,7 @@
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/metrics/metrics_log_uploader.h"
 #include "net/base/load_flags.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
 #include "net/url_request/url_fetcher.h"
 #include "url/gurl.h"
 
@@ -30,21 +31,92 @@ NetMetricsLogUploader::~NetMetricsLogUploader() {
 
 void NetMetricsLogUploader::UploadLog(const std::string& compressed_log_data,
                                       const std::string& log_hash) {
-  current_fetch_ =
-      net::URLFetcher::Create(GURL(server_url_), net::URLFetcher::POST, this);
-
   auto service = data_use_measurement::DataUseUserData::UMA;
 
   switch (service_type_) {
-    case MetricsLogUploader::UMA:
+    case MetricsLogUploader::UMA: {
       service = data_use_measurement::DataUseUserData::UMA;
+      net::NetworkTrafficAnnotationTag traffic_annotation =

[Alexei Svitkine (slow), 2017/02/27 18:24:08]

Can you declare NetworkTrafficAnnotationTag traffic_annotation outside of the
switch and assign to it within? Then, the URLFetcher::Create() code can be
common below the switch. You also won't need the {}'s.

[Ramin Halavati, 2017/02/28 08:55:39]

That makes the clang tool that extracts annotations quite more complicated as it
should follow two paths.

[Alexei Svitkine (slow), 2017/02/28 16:01:50]

I see. Please add a comment about this - so that someone doesn't refactor this
in the future.

[battre, 2017/02/28 16:06:14]

Why is this the case? Didn't we discuss that we could have even static global
const objects for the traffic annotation?

[Ramin Halavati, 2017/03/01 05:52:49]

This is a different issue, I had changed it this way because our current report
follows up the annotation down to the function that uses it and if two different
annotations are given to one function, it should follow this path through case
and if blocks as well. But as you mentioned before this is not an important part
of the report and we can drop it for cases that clang tool cannot find it. So I
will change the code to the way Alexei suggested.

+          net::DefineNetworkTrafficAnnotation("metrics_report", R"(
+            semantics {
+              sender: "Metrics Log Uploader"
+              description:
+                "Report of usage statistics and crash-related data about "
+                "Chromium. Usage statistics contain information such as "
+                "preferences, button clicks, and memory usage and do not "
+                "include web page URLs or personal information. See more at "
+                "https://www.google.com/chrome/browser/privacy/ under 'Usage "
+                "statistics and crash reports'."
+              trigger:
+                "Reports are automatically generated on startup and at "
+                "intervals while Chromium is running."
+              data:
+                "A protocol buffer with usage statistics and crash related "
+                "data."
+              destination: GOOGLE_OWNED_SERVICE
+            }
+            policy {
+              cookies_allowed: false
+              setting:
+                "Users can enable or disable this feature by disabling "
+                "'Automatically send usage statistics and crash reports to "
+                "Google' in Chromium's settings under Advanced Settings, "
+                "Privacy. The feature is enabled by default."
+              policy {
+                MetricsReportingEnabled {
+                  policy_options {mode: MANDATORY}
+                  value: false
+                }
+              }
+            })");
+      current_fetch_ = net::URLFetcher::Create(
+          GURL(server_url_), net::URLFetcher::POST, this, traffic_annotation);
       break;
-    case MetricsLogUploader::UKM:
+    }
+    case MetricsLogUploader::UKM: {
       service = data_use_measurement::DataUseUserData::UKM;
+      net::NetworkTrafficAnnotationTag traffic_annotation =
+          net::DefineNetworkTrafficAnnotation("metrics_report", R"(
+            semantics {
+              sender: "Metrics Log Uploader"

[rkaplow, 2017/02/27 22:52:58]

how is the sender field used?

[Ramin Halavati, 2017/02/28 08:55:39]

It's used in the report stating which module is making this request. It does not
need to be unique.

+              description:
+                "Report of usage statistics and crash-related data about "

[rkaplow, 2017/02/27 22:52:58]

this is not correct for UKM. 
This is the draft of the current policy wording:
https://docs.google.com/document/d/1hKVEH7DjVMeu5xgHde4XsNABh4L2QluL7PmeaoDcY...

Would you like me to suggest wording for this section?

[Ramin Halavati, 2017/02/28 08:55:39]

Yes please, thank you.

+                "Chromium. Usage statistics contain information such as "
+                "preferences, button clicks, and memory usage and do not "
+                "include web page URLs or personal information. See more at "
+                "https://www.google.com/chrome/browser/privacy/ under 'Usage "
+                "statistics and crash reports'."
+              trigger:
+                "Reports are automatically generated on startup and at "
+                "intervals while Chromium is running."
+              data:
+                "A protocol buffer with usage statistics and crash related "
+                "data."
+              destination: GOOGLE_OWNED_SERVICE
+            }
+            policy {
+              cookies_allowed: false
+              setting:
+                "Users can enable or disable this feature by disabling "

[rkaplow, 2017/02/27 22:52:58]

this is also not fully correct, you should mention this can also be disabling by
disabling History Sync (or Sync)

[Ramin Halavati, 2017/02/28 08:55:39]

Could you please suggest wording for this one as well?

+                "'Automatically send usage statistics and crash reports to "
+                "Google' in Chromium's settings under Advanced Settings, "
+                "Privacy. The feature is enabled by default."
+              policy {
+                MetricsReportingEnabled {
+                  policy_options {mode: MANDATORY}
+                  value: false
+                }
+              }
+            })");
+      current_fetch_ = net::URLFetcher::Create(
+          GURL(server_url_), net::URLFetcher::POST, this, traffic_annotation);
       break;
+    }
+    default: { DCHECK(false); }

[Alexei Svitkine (slow), 2017/02/27 18:24:08]

Compiler will error when an enum is not covered by the switch, so no need to add
this case.

[Ramin Halavati, 2017/02/28 08:55:39]

Done.

   }
   data_use_measurement::DataUseUserData::AttachToFetcher(current_fetch_.get(),
                                                          service);
+
   current_fetch_->SetRequestContext(request_context_getter_);
   current_fetch_->SetUploadData(mime_type_, compressed_log_data);