Log operator code histogram on new metric log

chrome/browser/chrome_browser_metrics_log_observer.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/chrome_browser_metrics_log_observer.h"
+
+#if defined(OS_ANDROID)
+#include "base/metrics/sparse_histogram.h"
+#include "base/strings/string_number_conversions.h"
+#include "net/android/network_library.h"
+#endif
+
+ChromeBrowserMetricsLogObserver::ChromeBrowserMetricsLogObserver() {
+  MetricsServiceHelper::AddMetricsLogObserver(this);
+}
+
+ChromeBrowserMetricsLogObserver::~ChromeBrowserMetricsLogObserver() {
+  MetricsServiceHelper::RemoveMetricsLogObserver(this);
+}
+
+void ChromeBrowserMetricsLogObserver::OnNewMetricsLog() {
+#if defined(OS_ANDROID)
+  net::NetworkChangeNotifier::ConnectionType type =
+      net::NetworkChangeNotifier::GetConnectionType();
+  unsigned mcc_mnc = 0;
+  if (type == net::NetworkChangeNotifier::CONNECTION_2G ||
+      type == net::NetworkChangeNotifier::CONNECTION_3G ||
+      type == net::NetworkChangeNotifier::CONNECTION_4G) {
+    // Log zero if not perfectly converted.
+    if (!base::StringToUint(
+        net::android::GetTelephonyNetworkOperator(), &mcc_mnc)) {
+      mcc_mnc = 0;
+    }
+  }
+  UMA_HISTOGRAM_SPARSE_SLOWLY(
+      "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc);

[Ilya Sherman, 2014/05/07 01:03:16]

Hmm, I was expecting that you'd emit to a single histogram, e.g.
"Network.NetworkOperatorMCCMNC".  What's the advantage of separating
_NewMetricsLog from _ConnectionChange?

[bolian, 2014/05/07 02:20:26]

It is more about the accurate of the name, this one is not due to connection
change.
The alternative could be _ConnectionChangeOrNewMetricLog, but then we have to
assume the first one always the NewMetricsLog and later ones are due to network
change. Separating them is clearer to me.

[Ilya Sherman, 2014/05/07 23:57:40]

Why does it matter which one is first?  If there are multiple changes during a
single log period, you wouldn't be able to sequence those anyway.  Do you really
need the sequencing data?

[bolian, 2014/05/08 00:54:51]

Mainly, I want to differentiate below two cases:
  A: no connection change during the log (with only _NewMetricsLog and no
_ConnectionChange)
  B: connection changed during the long (with one or more _ConnectionChanges).

In case A, we can trust all the data happened on the same cellular network, in
case B, we would take the result with a grain of salt.

Combining them makes it not so obvious if even possible.

[Ilya Sherman, 2014/05/08 01:10:11]

If you emit to a single histogram, you're in case A if the histogram has just a
single sample, and case B if the histogram has more than one sample.

[bolian, 2014/05/08 01:29:58]

hmm, I can do that (and remove the metric to be
_ConnectionChangeOrNewMetricsLog), but I still worry that there might be cases
(now or in the future), the current _NewMetricsLog is not logged for some bug
and there is one _ConnectionChange logged. That will mess up our analysis.
Separating them could avoid that. What is the main benefit of combing them other
than less histograms to maintain?

[Ilya Sherman, 2014/05/08 03:09:40]

The benefit is indeed having fewer histograms to maintain.  That's a real and
important benefit ;)

If there is a bug in your metrics emission code, the analysis is going to run
into problems anyway.  If you use two separate histograms, and sometimes nothing
is emitted to _NewMetricsLog, then sometimes you won't see any data at all for a
given upload, which would also mess up your analysis.

(Btw, I would recommend dropping the suffix entirely, rather than using
"_ConnectionChangeOrNewMetricsLog".)

[bolian, 2014/05/08 03:47:18]

fair enough. I combined them and rename it by removing the suffix.

+#endif
+}