Log aggregated abort metrics when histograms are uploaded

components/metrics/net/network_metrics_provider.cc

Index: components/metrics/net/network_metrics_provider.cc
diff --git a/components/metrics/net/network_metrics_provider.cc b/components/metrics/net/network_metrics_provider.cc
index 07cdf57a7af5a3e2d9e11cb0c3a5d6703cfa19ff..6ba745061c155093b5fa590bebc608201e2e0da1 100644
--- a/components/metrics/net/network_metrics_provider.cc
+++ b/components/metrics/net/network_metrics_provider.cc
@@ -10,11 +10,15 @@
 #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"
@@ -43,6 +47,7 @@ void NetworkMetricsProvider::ProvideGeneralMetrics(
   // ProvideGeneralMetrics is called on the main thread, at the time a metrics
   // record is being finalized.
   net::NetworkChangeNotifier::FinalizingMetricsLogRecord();
+  LogAggregatedMetrics();
 }
 
 void NetworkMetricsProvider::ProvideSystemProfileMetrics(
@@ -240,4 +245,18 @@ void NetworkMetricsProvider::WriteWifiAccessPointProto(
   }
 }
 
+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