Histogram for UMA log upload connection type.

chrome/browser/metrics/chrome_metrics_service_client.cc

 // Copyright 2014 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/metrics/chrome_metrics_service_client.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/platform_thread.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/google/google_brand.h"
 #include "chrome/browser/metrics/chrome_stability_metrics_provider.h"
@@ skipping 92 matching lines @@
 #endif
 
 }  // namespace
 
 ChromeMetricsServiceClient::ChromeMetricsServiceClient(
     metrics::MetricsStateManager* state_manager)
     : metrics_state_manager_(state_manager),
       chromeos_metrics_provider_(NULL),
       waiting_for_collect_final_metrics_step_(false),
       num_async_histogram_fetches_in_progress_(0),
       weak_ptr_factory_(this) {

[Ilya Sherman, 2015/03/24 00:14:23]

Please initialize the metrics provider here.

[gayane -on leave until 09-2017, 2015/03/24 20:45:55]

Done.

   DCHECK(thread_checker_.CalledOnValidThread());
   RecordCommandLineMetrics();
   RegisterForNotifications();
 
 #if defined(OS_WIN)
   CountBrowserCrashDumpAttempts();
 #endif  // defined(OS_WIN)
 }
 
 ChromeMetricsServiceClient::~ChromeMetricsServiceClient() {
@@ skipping 59 matching lines @@
   chrome::VersionInfo version_info;
   std::string version = version_info.Version();
 #if defined(ARCH_CPU_64_BITS)
   version += "-64";
 #endif  // defined(ARCH_CPU_64_BITS)
   if (!version_info.IsOfficialBuild())
     version.append("-devel");
   return version;
 }
 
-void ChromeMetricsServiceClient::OnLogUploadComplete() {
+void ChromeMetricsServiceClient::OnLogUploadComplete(bool upload_succeeded) {
+  // Record the network connection type.
+  if (upload_succeeded) {
+    UMA_HISTOGRAM_SPARSE_SLOWLY("UMA.LogUpload.ConnetionType",
+                                network_metrics_provider_->GetConnectionType());

[Ilya Sherman, 2015/03/24 00:14:23]

The connection type is already recorded in the system profile.  Why do we need
to record it as a histogram as well?

[Ilya Sherman, 2015/03/24 00:14:23]

FWIW, this histogram doesn't really need to be sparse. 
UMA_HISTOGRAM_ENUMERATION might be more appropriate here.

[gayane -on leave until 09-2017, 2015/03/24 20:45:55]

As discussed, system profile records the connection type during the collection
of the records while we are interested to know the connection type used for the
log upload. Note: as histogram is recorded after the upload it might not be
exact but should be pretty close.

[gayane -on leave until 09-2017, 2015/03/24 20:45:55]

Done.

+  }
+
   // Collect network stats after each UMA upload.
   network_stats_uploader_.CollectAndReportNetworkStats();
 }
 
 void ChromeMetricsServiceClient::StartGatheringMetrics(
     const base::Closure& done_callback) {
   finished_gathering_initial_metrics_callback_ = done_callback;
   base::Closure got_hardware_class_callback =
       base::Bind(&ChromeMetricsServiceClient::OnInitTaskGotHardwareClass,
                  weak_ptr_factory_.GetWeakPtr());
@@ skipping 41 matching lines @@
   return scoped_ptr<metrics::MetricsLogUploader>(
       new metrics::NetMetricsLogUploader(
           g_browser_process->system_request_context(),
           metrics::kDefaultMetricsServerUrl,
           metrics::kDefaultMetricsMimeType,
           on_upload_complete));
 }
 
 base::TimeDelta ChromeMetricsServiceClient::GetStandardUploadInterval() {
 #if defined(OS_ANDROID) || defined(OS_IOS)
-  bool is_cellular = false;
-  cellular_callback_.Run(&is_cellular);
-
-  if (is_cellular && IsCellularEnabledByExperiment())
+  if (network_metrics_provider_->IsCellularConnection() &&
+      IsCellularEnabledByExperiment())
     return base::TimeDelta::FromSeconds(kStandardUploadIntervalCellularSeconds);
 #endif
   return base::TimeDelta::FromSeconds(kStandardUploadIntervalSeconds);
 }
 
 base::string16 ChromeMetricsServiceClient::GetRegistryBackupKey() {
 #if defined(OS_WIN)
   return L"Software\\" PRODUCT_STRING_PATH L"\\StabilityMetrics";
 #else
   return base::string16();
@@ skipping 15 matching lines @@
 
   // Register metrics providers.
 #if defined(ENABLE_EXTENSIONS)
   metrics_service_->RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(
           new ExtensionsMetricsProvider(metrics_state_manager_)));
 #endif
   scoped_ptr<metrics::NetworkMetricsProvider> network_metrics_provider(
       new metrics::NetworkMetricsProvider(
           content::BrowserThread::GetBlockingPool()));
-  cellular_callback_ = network_metrics_provider->GetConnectionCallback();
+
+  // GetConnectionCallback should be called from non const pointer.

[Ilya Sherman, 2015/03/24 00:14:23]

I don't understand this comment.  Why does this need to be documented?

[gayane -on leave until 09-2017, 2015/03/24 20:45:55]

As network_metrics_provider_ as a const pointer I couldn't use it to call
GetConnectionCallback as its not const. However, I have removed const from
network_metrics_provider_ to be similar to other metrics providers. As a result
moved profiler_metrics_provider_ back where it was and used
network_metrics_provider_.

+  profiler_metrics_provider_ = new metrics::ProfilerMetricsProvider(
+      network_metrics_provider.get()->GetConnectionCallback());
+  metrics_service_->RegisterMetricsProvider(
+      scoped_ptr<metrics::MetricsProvider>(profiler_metrics_provider_));
+
+  network_metrics_provider_ = network_metrics_provider.get();
   metrics_service_->RegisterMetricsProvider(network_metrics_provider.Pass());
 
   metrics_service_->RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(new OmniboxMetricsProvider));
   metrics_service_->RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(new ChromeStabilityMetricsProvider));
   metrics_service_->RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(new metrics::GPUMetricsProvider()));
 
-  profiler_metrics_provider_ =
-      new metrics::ProfilerMetricsProvider(cellular_callback_);
-  metrics_service_->RegisterMetricsProvider(
-      scoped_ptr<metrics::MetricsProvider>(profiler_metrics_provider_));
-
 #if defined(OS_ANDROID)
   metrics_service_->RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(
           new AndroidMetricsProvider(g_browser_process->local_state())));
 #endif  // defined(OS_ANDROID)
 
 #if defined(OS_WIN)
   google_update_metrics_provider_ = new GoogleUpdateMetricsProviderWin;
   metrics_service_->RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(google_update_metrics_provider_));
@@ skipping 247 matching lines @@
   // Capture the histogram samples.
   if (dumps_with_crash != 0)
     UMA_HISTOGRAM_COUNTS("Chrome.BrowserDumpsWithCrash", dumps_with_crash);
   if (dumps_with_no_crash != 0)
     UMA_HISTOGRAM_COUNTS("Chrome.BrowserDumpsWithNoCrash", dumps_with_no_crash);
   int total_dumps = dumps_with_crash + dumps_with_no_crash;
   if (total_dumps != 0)
     UMA_HISTOGRAM_COUNTS("Chrome.BrowserCrashDumpAttempts", total_dumps);
 }
 #endif  // defined(OS_WIN)