Starting a refactor to allow adding metrics on upload.

Record document mode histogram in UMA on every upload.

* Refactors the code to allow adding metrics as part of metrics provider
  interface
* Implements a function in android_metrics_provider to record a new
  histogram which records the current document mode. This is needed for
  better analysis in Hera. I am adding a new histogram because the
  semantics are a bit different from DocumentActivity.RunningMode. The
  previous histogram is used only to record eligible devices that have
  manually opted in or out. The new histogram will record for all devices
  regardless of whether they could possibly be in document mode.

BUG=418642
Committed: https://crrev.com/191028987824922561697fe1cf7056164495dfa9
Cr-Commit-Position: refs/heads/master@{#300375}

[Maria, 2014-10-15 21:23:05]

mariakhomenko@chromium.org changed reviewers:
+ asvitkine@google.com

[Maria, 2014-10-15 21:23:05]

So this is not yet the full code, but I wanted to see if you think this is the
right approach.

The idea being that I would create a new metrics provider for document mode and
do recording in the new method I added.

The part I wanted to make sure about, does this guarantee that every
ChromeUserMetricsExtension message that we record has the histogram entry in it?

[Alexei Svitkine (slow), 2014-10-15 21:39:01]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-10-15 21:39:02]

LGTM % comment - looks great!

"The part I wanted to make sure about, does this guarantee that every
ChromeUserMetricsExtension message that we record has the histogram entry in
it?"

It will be the case except for two edge cases that I know of:
  - The initial stability log that has metrics from the previous session (i.e.
crash counts). This is tricky - if we wanted to correctly tag those, we'd need
some code to save this to prefs and reload for the stability log - I don't think
it's worth it for the initial version of the CL - but should add a TODO.
   - On a clean shutdown, the current UMA log will be closed without hitting
this codepath. However, in this case, providers will have OnRecordingDisabled()
called on them, so they can also emit to the histogram from that method.

https://codereview.chromium.org/658903002/diff/20001/chrome/browser/metrics/s...
File chrome/browser/metrics/signin_status_metrics_provider.h (right):

https://codereview.chromium.org/658903002/diff/20001/chrome/browser/metrics/s...
chrome/browser/metrics/signin_status_metrics_provider.h:37: virtual void
OnCollectFinalMetrics();
Nit: OVERRIDE and add change comment to:

// metrics::MetricsProvider:

Move the more descriptive comment (minus the last sentence) into the
implementation of the method.

[Alexei Svitkine (slow), 2014-10-15 21:39:59]

By the way, for the new bits - maybe they can be added to the existing
AndroidMetricsProvider class, rather than making a new one.

[Maria, 2014-10-17 01:22:03]

mariakhomenko@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Maria, 2014-10-17 01:22:04]

This is now ready for review, please take a look.

[Ted C, 2014-10-17 18:12:11]

androidy bits - lgtm

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/android/f...
File chrome/browser/android/feature_utilities.cc (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/android/f...
chrome/browser/android/feature_utilities.cc:12: static bool
document_mode_enabled = false;
hmm...should this be an empty namespace to keep it more isolated?

[Alexei Svitkine (slow), 2014-10-17 18:30:52]

Looks great, some comments below.

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/android/f...
File chrome/browser/android/feature_utilities.h (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/android/f...
chrome/browser/android/feature_utilities.h:13: static const int DOCUMENT_MODE =
0;
Is there a reason this isn't an enum?

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider.cc (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider.cc:43: "DocumentMode.Enabled",
Please don't introduce a new top-level prefix. Maybe use the same prefix as the
other histograms logged by this file - Chrome.Android.

Also, since this histogram is recorded by the open source code, can you put its
description in histograms.xml in the Chromium code - in the same CL?

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider.h (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider.h:22: //
metrics::MetricsProvider:
Can you put this in the same section as ProvideStabilityMetrics() below?

https://codereview.chromium.org/658903002/diff/60001/components/metrics/metri...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/658903002/diff/60001/components/metrics/metri...
components/metrics/metrics_service.cc:853:
metrics_providers_[i]->OnCollectFinalMetrics();
Can you call this from DisableRecording() too?

Per my previous reply - this will cover cases where a log is closed without
hitting CollectFinalMetrics().

[Maria, 2014-10-17 21:44:50]

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/android/f...
File chrome/browser/android/feature_utilities.cc (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/android/f...
chrome/browser/android/feature_utilities.cc:12: static bool
document_mode_enabled = false;
On 2014/10/17 18:12:11, Ted C wrote:
> hmm...should this be an empty namespace to keep it more isolated?

Done.

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/android/f...
File chrome/browser/android/feature_utilities.h (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/android/f...
chrome/browser/android/feature_utilities.h:13: static const int DOCUMENT_MODE =
0;
On 2014/10/17 18:30:51, Alexei Svitkine wrote:
> Is there a reason this isn't an enum?

Done.

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider.cc (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider.cc:43: "DocumentMode.Enabled",
On 2014/10/17 18:30:51, Alexei Svitkine wrote:
> Please don't introduce a new top-level prefix. Maybe use the same prefix as
the
> other histograms logged by this file - Chrome.Android.
> 
> Also, since this histogram is recorded by the open source code, can you put
its
> description in histograms.xml in the Chromium code - in the same CL?

I am reusing a top level prefix that we use in internal histograms now. Is that
ok? Also added the histograms.xml entry.

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider.h (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider.h:22: //
metrics::MetricsProvider:
On 2014/10/17 18:30:52, Alexei Svitkine wrote:
> Can you put this in the same section as ProvideStabilityMetrics() below?

This file needs to be generally reordered as the cc implementation order doesn't
match the declared order in this file. I added this here to avoid making the
problem worse. I am planning to put in a CL immediately after this to fix the
ordering issue. How about I reorder it then?

https://codereview.chromium.org/658903002/diff/60001/components/metrics/metri...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/658903002/diff/60001/components/metrics/metri...
components/metrics/metrics_service.cc:853:
metrics_providers_[i]->OnCollectFinalMetrics();
On 2014/10/17 18:30:52, Alexei Svitkine wrote:
> Can you call this from DisableRecording() too?
> 
> Per my previous reply - this will cover cases where a log is closed without
> hitting CollectFinalMetrics().

It's a little weird I think to call onCollectFinalMetrics from DisableRecording
without calling CollectFinalMetrics. What's the reason we don't call that method
before disabling?

Do you think it makes sense to add a call from OnDisableRecording to
OnCollectFinalMetrics inside the provider?

[Alexei Svitkine (slow), 2014-10-20 15:47:00]

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider.cc (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider.cc:43: "DocumentMode.Enabled",
On 2014/10/17 21:44:50, Maria wrote:
> On 2014/10/17 18:30:51, Alexei Svitkine wrote:
> > Please don't introduce a new top-level prefix. Maybe use the same prefix as
> the
> > other histograms logged by this file - Chrome.Android.
> > 
> > Also, since this histogram is recorded by the open source code, can you put
> its
> > description in histograms.xml in the Chromium code - in the same CL?
> 
> I am reusing a top level prefix that we use in internal histograms now. Is
that
> ok? Also added the histograms.xml entry.

Ah fair enough, OK,

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
File chrome/browser/metrics/android_metrics_provider.h (right):

https://codereview.chromium.org/658903002/diff/60001/chrome/browser/metrics/a...
chrome/browser/metrics/android_metrics_provider.h:22: //
metrics::MetricsProvider:
On 2014/10/17 21:44:50, Maria wrote:
> On 2014/10/17 18:30:52, Alexei Svitkine wrote:
> > Can you put this in the same section as ProvideStabilityMetrics() below?
> 
> This file needs to be generally reordered as the cc implementation order
doesn't
> match the declared order in this file. I added this here to avoid making the
> problem worse. I am planning to put in a CL immediately after this to fix the
> ordering issue. How about I reorder it then?

That sounds fine to me, thanks!

https://codereview.chromium.org/658903002/diff/60001/components/metrics/metri...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/658903002/diff/60001/components/metrics/metri...
components/metrics/metrics_service.cc:853:
metrics_providers_[i]->OnCollectFinalMetrics();
On 2014/10/17 21:44:50, Maria wrote:
> On 2014/10/17 18:30:52, Alexei Svitkine wrote:
> > Can you call this from DisableRecording() too?
> > 
> > Per my previous reply - this will cover cases where a log is closed without
> > hitting CollectFinalMetrics().
> 
> It's a little weird I think to call onCollectFinalMetrics from
DisableRecording
> without calling CollectFinalMetrics. What's the reason we don't call that
method
> before disabling?

It's not done because CollectFinalMetrics() involves some async work (i.e.
contact subprocesses to get their metrics) and the DisableRecording codepath is
executed on a shutdown when closing the final log. So I think the idea is to not
block shut down due to async work.

> 
> Do you think it makes sense to add a call from OnDisableRecording to
> OnCollectFinalMetrics inside the provider?

I'd prefer we do it in metrics service, so that each provider that wants to
record such a histogram doesn't need to worry about making sure to do this thing
itself. I agree that OnCollectFinalMetrics would be misleading in this case.

Thinking about this more, let's not introduce the new OnCollectFinalMetrics()
method at all and instead use the existing ProvideGeneralMetrics() method.
Current users of that method only provide non-histogram metrics (i.e. set the
protobuf fields themselves), but it looks like we can easily make that support
logging histograms from it.

To do that, the two calls to RecordGeneralMetrics() in metrics_service.cc have
to be moved just above  RecordCurrentHistograms(). I checked all the current
implementors of ProvideGeneralMetrics() and it should be safe to do (none of
their functionality depends on the ordering w.r.t. to histogram recording).

When you do this, please also expand the comment on ProvideGeneralMetrics() in
metrics_provider.h to mention that this method can also be used to record
histograms that will recorded with every metrics log.

[Maria, 2014-10-20 20:10:32]

Made suggested changes, PTAL

[Alexei Svitkine (slow), 2014-10-20 21:19:27]

LGTM % nit below

Please also cc me on the corresponding internal change.

https://codereview.chromium.org/658903002/diff/100001/chrome/browser/android/...
File chrome/browser/android/feature_utilities.cc (right):

https://codereview.chromium.org/658903002/diff/100001/chrome/browser/android/...
chrome/browser/android/feature_utilities.cc:10: static bool
document_mode_enabled = false;
Nit: No need for "static" inside an anon namespace. Also, de-indent this.

[Maria, 2014-10-20 21:25:22]

The CQ bit was checked by mariakhomenko@chromium.org

[commit-bot: I haz the power, 2014-10-20 21:29:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/658903002/120001

[commit-bot: I haz the power, 2014-10-20 23:23:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-20 23:23:55]

Patchset 7 (id:??) landed as
https://crrev.com/191028987824922561697fe1cf7056164495dfa9
Cr-Commit-Position: refs/heads/master@{#300375}