Log operator code histogram on new metric log

chrome/browser/chrome_browser_main.cc

 // Copyright (c) 2012 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_main.h"
 
 #include <set>
 #include <string>
 #include <vector>
 
@@ skipping 24 matching lines @@
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/about_flags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_impl.h"
 #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"
 #include "chrome/browser/component_updater/pnacl/pnacl_component_installer.h"
 #include "chrome/browser/component_updater/recovery_component_installer.h"
 #include "chrome/browser/component_updater/swiftshader_component_installer.h"
 #include "chrome/browser/component_updater/widevine_cdm_component_installer.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/startup_helper.h"
@@ skipping 497 matching lines @@
 // This will be called after the command-line has been mutated by about:flags
 void ChromeBrowserMainParts::SetupMetricsAndFieldTrials() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::SetupMetricsAndFieldTrials");
   // Must initialize metrics after labs have been converted into switches,
   // but before field trials are set up (so that client ID is available for
   // one-time randomized field trials).
 #if defined(ARCH_CPU_64_BITS)
   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()));
 
   const CommandLine* command_line = CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kEnableBenchmarking))
     base::FieldTrial::EnableBenchmarking();
 
   // Ensure any field trials specified on the command line are initialized.
   // Also stop the metrics service so that we don't pollute UMA.
   if (command_line->HasSwitch(switches::kForceFieldTrials)) {
@@ skipping 1078 matching lines @@
   chromeos::CrosSettings::Shutdown();
 #endif
 #endif
 }
 
 // Public members:
 
 void ChromeBrowserMainParts::AddParts(ChromeBrowserMainExtraParts* parts) {
   chrome_extra_parts_.push_back(parts);
 }