Introduce a MetricsDataProvider interface.

Introduce a MetricsProvider interface.

Also, introduces a way to register MetricsProviders
on MetricsService.

BUG=374229
R=isherman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271652

[Ilya Sherman, 2014-05-19 14:45:00]

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:512: 
nit: Spurious newline.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.h:97: const
ScopedVector<metrics::MetricsDataProvider>& data_providers,
nit: Passing ScopedVectors is a little odd.  Please pass the underlying
std::vector instead.  (Ditto throughout.)

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.h:97: const
ScopedVector<metrics::MetricsDataProvider>& data_providers,
nit: Please update the docs.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.h:126: const
ScopedVector<metrics::MetricsDataProvider>& data_providers,
nit: Please update the docs.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1181:
current_log->RecordGeneralMetrics(data_providers_);
Does it make sense to fold this into RecordEnvironment?

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1437: MetricsLog*
initial_stability_log_ptr = initial_stability_log.get();
Optiona nit: Perhaps it would be cleaner to access the log as
log_manager_.current_log()?  (Imagine it without the static_cast, since
eventually that will go away.)

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1474: incremental_uptime, uptime);
Are you intentionally not recording general metrics as part of this log?

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1739: 
Optional nit: I'd omit this blank line.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:31: #include
"components/metrics/metrics_data_provider.h"
nit: Can this be forward-declared?

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:496: // Additional metrics data
providers that have been registered.
nit: Additional over what?  I'd probably just omit the word, and write this as
"Registered metrics data providers."

https://codereview.chromium.org/296483004/diff/20001/components/metrics/metri...
File components/metrics/metrics_data_provider.cc (right):

https://codereview.chromium.org/296483004/diff/20001/components/metrics/metri...
components/metrics/metrics_data_provider.cc:27: }  // namespace metrics
nit: I'd inline the entire contents of this file.  If it's good enough for
WebContentsObserver, it's probably good enough for us :)

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

(I just remembered that the content API does this pretty consistently
throughout.)

https://codereview.chromium.org/296483004/diff/20001/components/metrics/metri...
File components/metrics/metrics_data_provider.h (right):

https://codereview.chromium.org/296483004/diff/20001/components/metrics/metri...
components/metrics/metrics_data_provider.h:34: // Provides general metrics that
are neither system profile or stability
nit: "or" -> "nor"

[Alexei Svitkine (slow), 2014-05-19 15:23:01]

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:512: 
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> nit: Spurious newline.

Done.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.h:97: const
ScopedVector<metrics::MetricsDataProvider>& data_providers,
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> nit: Passing ScopedVectors is a little odd.  Please pass the underlying
> std::vector instead.  (Ditto throughout.)

Didn't realise it's possible to access the underlying vector. Done.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.h:97: const
ScopedVector<metrics::MetricsDataProvider>& data_providers,
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> nit: Please update the docs.

Done.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.h:126: const
ScopedVector<metrics::MetricsDataProvider>& data_providers,
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> nit: Please update the docs.

Done.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1181:
current_log->RecordGeneralMetrics(data_providers_);
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> Does it make sense to fold this into RecordEnvironment?

I don't think it does. RecordEnvironment() records the system profile proto.
Whereas general metrics would be things like omnibox events, which shouldn't
necessarily be recorded every time we record the system profile.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1437: MetricsLog*
initial_stability_log_ptr = initial_stability_log.get();
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> Optiona nit: Perhaps it would be cleaner to access the log as
> log_manager_.current_log()?  (Imagine it without the static_cast, since
> eventually that will go away.)

Fair enough, the static cast is why I did this and you're right that it will go
away. Done.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1474: incremental_uptime, uptime);
Oops, it was indeed not intentional. There was also an additional issue with
RecordStabilityMetrics not being moved below BeginLogginWithLog(), so I fixed
this now here too. (And yes, we should consolidate these methods more... but not
today.)

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1739: 
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> Optional nit: I'd omit this blank line.

Done.

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:31: #include
"components/metrics/metrics_data_provider.h"
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> nit: Can this be forward-declared?

Doesn't look like it:

../../base/stl_util.h:44:5: error: deleting pointer to incomplete type
'metrics::MetricsDataProvider' may cause undefined behavior
[-Werror,-Wdelete-incomplete]
    delete *temp;
    ^      ~~~~~
../../base/stl_util.h:140:3: note: in instantiation of function template
specialization
'STLDeleteContainerPointers<__gnu_cxx::__normal_iterator<metrics::MetricsDataProvider
**, std::vector<metrics::MetricsDataProvider *,
std::allocator<metrics::MetricsDataProvider *> > > >' requested here
  STLDeleteContainerPointers(container->begin(), container->end());
  ^
../../base/memory/scoped_vector.h:97:18: note: in instantiation of function
template specialization
'STLDeleteElements<std::vector<metrics::MetricsDataProvider *,
std::allocator<metrics::MetricsDataProvider *> > >' requested here
  void clear() { STLDeleteElements(&v_); }
                 ^
../../base/memory/scoped_vector.h:37:21: note: in instantiation of member
function 'ScopedVector<metrics::MetricsDataProvider>::clear' requested here
  ~ScopedVector() { clear(); }

https://codereview.chromium.org/296483004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:496: // Additional metrics data
providers that have been registered.
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> nit: Additional over what?  I'd probably just omit the word, and write this as
> "Registered metrics data providers."

Done.

https://codereview.chromium.org/296483004/diff/20001/components/metrics/metri...
File components/metrics/metrics_data_provider.cc (right):

https://codereview.chromium.org/296483004/diff/20001/components/metrics/metri...
components/metrics/metrics_data_provider.cc:27: }  // namespace metrics
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> nit: I'd inline the entire contents of this file.  If it's good enough for
> WebContentsObserver, it's probably good enough for us :)
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> 
> (I just remembered that the content API does this pretty consistently
> throughout.)

Done.

https://codereview.chromium.org/296483004/diff/20001/components/metrics/metri...
File components/metrics/metrics_data_provider.h (right):

https://codereview.chromium.org/296483004/diff/20001/components/metrics/metri...
components/metrics/metrics_data_provider.h:34: // Provides general metrics that
are neither system profile or stability
On 2014/05/19 14:45:00, Ilya Sherman wrote:
> nit: "or" -> "nor"

Done.

[Ilya Sherman, 2014-05-19 16:05:52]

LGTM, thanks.

https://codereview.chromium.org/296483004/diff/40001/components/metrics/metri...
File components/metrics/metrics_data_provider.h (right):

https://codereview.chromium.org/296483004/diff/40001/components/metrics/metri...
components/metrics/metrics_data_provider.h:24: // is a no-op.
nit: Probably no need to mention the default implementation for this method,
unless you think it's worth mentioning for all the methods.

[Alexei Svitkine (slow), 2014-05-19 16:08:56]

https://codereview.chromium.org/296483004/diff/40001/components/metrics/metri...
File components/metrics/metrics_data_provider.h (right):

https://codereview.chromium.org/296483004/diff/40001/components/metrics/metri...
components/metrics/metrics_data_provider.h:24: // is a no-op.
On 2014/05/19 16:05:53, Ilya Sherman wrote:
> nit: Probably no need to mention the default implementation for this method,
> unless you think it's worth mentioning for all the methods.

Done.

[Alexei Svitkine (slow), 2014-05-19 16:09:01]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-19 16:09:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/296483004/60001

[commit-bot: I haz the power, 2014-05-19 18:34:51]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-19 18:37:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-19 18:37:46]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Alexei Svitkine (slow), 2014-05-19 21:35:49]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-19 21:36:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/296483004/90001

[commit-bot: I haz the power, 2014-05-20 01:42:55]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-20 02:16:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 02:16:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Alexei Svitkine (slow), 2014-05-20 06:16:18]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-20 06:17:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/296483004/90001

[Alexei Svitkine (slow), 2014-05-20 07:06:52]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-20 07:07:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/296483004/110001

[Alexei Svitkine (slow), 2014-05-20 12:06:17]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-20 12:10:41]

Added recording enabled / disabled methods as discussed offline, PTAL.

[Ilya Sherman, 2014-05-20 12:12:29]

Still LGTM

[Alexei Svitkine (slow), 2014-05-20 12:35:48]

Rebased and renamed MetricsDataProvider to MetricsProvider per offline
dicussion, PTAL.

[Ilya Sherman, 2014-05-20 12:52:16]

Still LGTM

[Alexei Svitkine (slow), 2014-05-20 13:03:28]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-20 13:04:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/296483004/190001

[Alexei Svitkine (slow), 2014-05-20 13:16:47]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-20 13:17:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/296483004/210001

[commit-bot: I haz the power, 2014-05-20 14:53:21]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-20 14:58:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 14:58:36]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Alexei Svitkine (slow), 2014-05-20 15:05:15]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-20 15:05:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/296483004/230001

[Alexei Svitkine (slow), 2014-05-20 15:19:00]

Committed patchset #9 manually as r271652 (presubmit successful).