Log operator code histogram on new metric log

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index d84585d1cdef1629afad49d2ab019c8f900cff10..4c8411dc271194c125d3a6e5d02ff4ad711d6ca8 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -42,6 +42,7 @@
 #include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/chrome_browser_main_extra_parts.h"
+#include "chrome/browser/chrome_browser_metrics_log_observer.h"
 #include "chrome/browser/component_updater/cld_component_installer.h"
 #include "chrome/browser/component_updater/component_updater_service.h"
 #include "chrome/browser/component_updater/flash_component_installer.h"
@@ -559,9 +560,11 @@ void ChromeBrowserMainParts::SetupMetricsAndFieldTrials() {
   MetricsLog::set_version_extension("-64");
 #endif  // defined(ARCH_CPU_64_BITS)
 
+  MetricsService* metrics = browser_process_->metrics_service();
+  // Create the metrics log observer.
+  browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver());

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

I am not an owner for this code, but modifying chrome_browser_main.cc for this
change seems unlikely to be appropriate -- it's too low a level.  Is there not
some reasonable //net component that this can be dangled off of?  (He asks,
being largely unfamiliar with the organization of the //net module.)

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

This is not resolved yet. It is here to make sure the observer is added as soon
as the metrics service is created. I am open to better suggestions.

[bolian, 2014/05/07 18:11:44]

I explored this more. //net still does not look like a good place for this, as
metrics service is created and maintained in //chrome/browser, and the observer
should be added as soon as it created. The current place is also where other
metrics related stuff happened. I keep it here for now. But I am open to
suggestions.

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

I still think this is unlikely to be the correct layer for making this change:

(1) First of all, is g_network_change_notifier guaranteed to be instantiated by
the time this code is reached?  If not, is it a problem to emit UNKNOWN for the
connection type for the initial log?

(2) Regardless of anything else, the observer itself can probably live in //net,
rather than in //chrome/browser.

(3) If you definitely want to register the observer as soon as the
MetricsService is constructed, the best place to do that is from the
MetricsService constructor.  That also has the advantage that we can remove the
friend declaration.

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

I keep remind myself that MetricsServiceObserver has nothing to do with //net
:).  It just happen to record the connection status when notification is
received, so why move it to //net.

I actually prefer register in MetricsService constructor, but that idea was shot
down during the meeting. Should we do that and move everything to
//chrome/browser/metrics, I think that still make sense as long as the observer
only log the environment info like the operator code.

Another place could be BrowserProcessImpl::CreateMetricsService().

What do you think?

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

Perhaps there was some miscommunication.  I don't recall discussing where the
AddObserver() call should take place.  Note: I'm suggesting a one-line change to
the constructor: just adding the observer.  I'm not suggesting getting rid of
the observer pattern.

The code to emit to "NCN.NetworkOperatorMCCMNC_ConnectionChange" currently lives
under //net.  Why isn't it appropriate for the observer code to reside there as
well?  The reason that I prefer to have the observer live under //net is that
it's recording //net-level concepts.  It's not recording anything to do with
Chrome's initialization (other than recording a value at startup), and it's not
recording a general-purpose metric.  Neither
//chrome/browser/chrome_browser_main.cc nor //chrome/browser/metrics should be a
dumping ground.

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

We have abstracted it to be an observer to MetricsService,
MetricsServiceObserver could be used to do other things on new metrics log in
the future, not just //net-level things. So, it is not proper to live under
//net.

We can add observers in constructor, but we still need to put the observers in
somewhere, so it does not change things much -- unless we move the observer
implementation itself into //chrome/browser/metrics, for this case. Other
observers in the future can live elsewhere. That will improve/simply things a
lot.

What do think?

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

Hmm, I think I understand your concern: You don't want //net to depend on
//chrome.  That's fair.  How about //chrome/browser/net, then?

   // Initialize FieldTrialList to support FieldTrials that use one-time
   // randomization.
-  MetricsService* metrics = browser_process_->metrics_service();
   field_trial_list_.reset(
       new base::FieldTrialList(metrics->CreateEntropyProvider().release()));