Log aggregated abort metrics when histograms are uploaded

components/metrics/net/network_metrics_provider.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 "components/metrics/net/network_metrics_provider.h"
 
 #include <stdint.h>
 
 #include <string>
 #include <vector>
 
 #include "base/compiler_specific.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/task_runner_util.h"
 #include "build/build_config.h"
+#include "net/base/net_errors.h"
 
 #if defined(OS_CHROMEOS)
 #include "components/metrics/net/wifi_access_point_info_provider_chromeos.h"
 #endif  // OS_CHROMEOS
 
 namespace metrics {
 
 NetworkMetricsProvider::NetworkMetricsProvider(
     base::TaskRunner* io_task_runner)
     : io_task_runner_(io_task_runner),
       connection_type_is_ambiguous_(false),
       wifi_phy_layer_protocol_is_ambiguous_(false),
       wifi_phy_layer_protocol_(net::WIFI_PHY_LAYER_PROTOCOL_UNKNOWN),
       weak_ptr_factory_(this) {
   net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
   connection_type_ = net::NetworkChangeNotifier::GetConnectionType();
   ProbeWifiPHYLayerProtocol();
 }
 
 NetworkMetricsProvider::~NetworkMetricsProvider() {
   net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
 }
 
 void NetworkMetricsProvider::ProvideGeneralMetrics(
     ChromeUserMetricsExtension*) {
   // ProvideGeneralMetrics is called on the main thread, at the time a metrics
   // record is being finalized.
   net::NetworkChangeNotifier::FinalizingMetricsLogRecord();
+  LogAggregatedMetrics();
 }
 
 void NetworkMetricsProvider::ProvideSystemProfileMetrics(
     SystemProfileProto* system_profile) {
   SystemProfileProto::Network* network = system_profile->mutable_network();
   network->set_connection_type_is_ambiguous(connection_type_is_ambiguous_);
   network->set_connection_type(GetConnectionType());
   network->set_wifi_phy_layer_protocol_is_ambiguous(
       wifi_phy_layer_protocol_is_ambiguous_);
   network->set_wifi_phy_layer_protocol(GetWifiPHYLayerProtocol());
@@ skipping 177 matching lines @@
   for (const base::StringPiece& oui_str : base::SplitStringPiece(
            info.oui_list, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL)) {
     uint32_t oui;
     if (base::HexStringToUInt(oui_str, &oui))
       vendor->add_element_identifier(oui);
     else
       NOTREACHED();
   }
 }
 
+void NetworkMetricsProvider::LogAggregatedMetrics() {
+  base::HistogramBase* error_codes = base::SparseHistogram::FactoryGet(
+      "Net.ErrorCodesForMainFrame3",
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+  scoped_ptr<base::HistogramSamples> samples = error_codes->SnapshotSamples();

[Ilya Sherman, 2016/03/16 22:33:41]

Hmm, I think this will give you the total number of samples seen since the
launch of Chrome, rather than since the last upload.  Have you tested that this
behaves as you'd expect?  (I was kind of thinking that you'd store this count
separately, outside of the histogram, rather than trying to read from the
histogram.  I guess reading from the histogram has the nice benefit that you
don't have to duplicate the storage (and maybe have a bug in that logic), but it
has the disadvantage that you have to keep track of deltas somehow.  Unless
you're intentionally uploading data from the launch of Chrome, rather than
deltas?)

[Charlie Harrison, 2016/03/17 14:03:20]

Ah I misunderstood and thought that SnapshotSamples only tracked values from the
last upload. This count is maintained on the IO thread so for simplicity I've
used a delta based approach. Is there a simple way to force these methods to
fire for testing?

+  base::HistogramBase::Count aborts = samples->GetCount(-net::ERR_ABORTED);
+  base::HistogramBase::Count total_codes = samples->TotalCount();
+  if (total_codes > 0) {
+    UMA_HISTOGRAM_COUNTS("Net.ErrAborted.CountPerUpload", aborts);
+    UMA_HISTOGRAM_PERCENTAGE("Net.ErrAborted.ProportionPerUpload",
+                             (100.0 * aborts) / total_codes);

[Ilya Sherman, 2016/03/16 22:33:41]

Hmm, this looks like an implicit conversion from float to int.  Please make the
conversion explicit.  (Note: I'm referring to the conversion *back* to int, when
emitting to the histogram -- not the upconversion to floating point that comes
from multiplying by 100.0.  Though, I think multiplying by 100 rather than 100.0
would probably give you the desired result.)

[Charlie Harrison, 2016/03/17 14:03:20]

Changed to a multiplication by 100.

+  }
+}
+
 }  // namespace metrics