Add network information to UKM

Add network information to UKM

Setup metric providers for UKM logs, and add network as a first client.
Small refactor of ChromeMetricsServiceClient::Initialize to put the registrations in their own methods.


BUG=678682
Review-Url: https://codereview.chromium.org/2671603002
Cr-Commit-Position: refs/heads/master@{#448330}
Committed: https://chromium.googlesource.com/chromium/src/+/b8d63f435e6c869164a0ec63eec5be867d2b7157

[rkaplow, 2017-02-02 00:20:13]

Description was changed from

==========
Add network information to UKM

Setup metric providers for UKM logs, and add network as a first client.

BUG=678682
==========

to

==========
Add network information to UKM

Setup metric providers for UKM logs, and add network as a first client.
Small refactor of ChromeMetricsServiceClient::Initialize to put the
registrations in their own methods.


BUG=678682
==========

[rkaplow, 2017-02-02 00:20:33]

rkaplow@chromium.org changed reviewers:
+ asvitkine@chromium.org, bmcquade@chromium.org

[rkaplow, 2017-02-02 00:23:59]

I was uncertain if the network metricsprovider should be shared. Currently the
model is the metricsservice owns its own providers, and I think it could be
possible for the same NetworkMetricsProvider to be used by both metrics
services.
The advantage is there should be a little less work done if it is shared, but
downside is ownership is more complicated (i.e. if the metricsservice owns it
and UKM has a pointer, what happens if metricsservice is destructed... maybe
have the client own it..?)

Anyways, went with the simpler/cleaner solution of creating two
NetworkMetricsProvider since the amount of work appears to be pretty small.

[rkaplow, 2017-02-02 16:27:06]

rkaplow@chromium.org changed reviewers:
+ holte@chromium.org

[Alexei Svitkine (slow), 2017-02-02 17:34:44]

lg, but can you add some test coverage?

https://codereview.chromium.org/2671603002/diff/20001/components/ukm/ukm_serv...
File components/ukm/ukm_service.h (right):

https://codereview.chromium.org/2671603002/diff/20001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:68: std::unique_ptr<metrics::MetricsProvider>
provider);
Indent is wrong. git cl format?

[Bryan McQuade, 2017-02-02 17:43:50]

LGTM, thanks! I suppose the only risk here is that the network providers assume
they're only instantiated once. I didn't see any evidence of that in the
implementations, but might be worth double checking with those who know more
(tbansal?).

[rkaplow, 2017-02-02 18:19:13]

rkaplow@chromium.org changed reviewers:
+ tbansal@chromium.org

[rkaplow, 2017-02-02 18:19:31]

On 2017/02/02 17:43:50, Bryan McQuade wrote:
> LGTM, thanks! I suppose the only risk here is that the network providers
assume
> they're only instantiated once. I didn't see any evidence of that in the
> implementations, but might be worth double checking with those who know more
> (tbansal?).

tbansal mind taking a look in case there's something we're missing?

[tbansal1, 2017-02-02 18:29:25]

On 2017/02/02 18:19:31, rkaplow wrote:
> On 2017/02/02 17:43:50, Bryan McQuade wrote:
> > LGTM, thanks! I suppose the only risk here is that the network providers
> assume
> > they're only instantiated once. I didn't see any evidence of that in the
> > implementations, but might be worth double checking with those who know more
> > (tbansal?).
> 
> tbansal mind taking a look in case there's something we're missing?

IIUC, Bryan is correct. At least the network metrics provider
keeps track of state between UMA pings. e.g., if the connection
type changes between two consecutive UMA pings, the connection
type state in the provider is marked as ambiguous.

If UKM and MetricsService query the same provider, then these
internal states would get messed up.

[rkaplow, 2017-02-02 18:41:32]

On 2017/02/02 18:29:25, tbansal1 wrote:
> On 2017/02/02 18:19:31, rkaplow wrote:
> > On 2017/02/02 17:43:50, Bryan McQuade wrote:
> > > LGTM, thanks! I suppose the only risk here is that the network providers
> > assume
> > > they're only instantiated once. I didn't see any evidence of that in the
> > > implementations, but might be worth double checking with those who know
more
> > > (tbansal?).
> > 
> > tbansal mind taking a look in case there's something we're missing?
> 
> IIUC, Bryan is correct. At least the network metrics provider
> keeps track of state between UMA pings. e.g., if the connection
> type changes between two consecutive UMA pings, the connection
> type state in the provider is marked as ambiguous.
> 
> If UKM and MetricsService query the same provider, then these
> internal states would get messed up.

I see. So this solution which instantiates separate providers is best then.

[tbansal1, 2017-02-02 18:43:22]

On 2017/02/02 18:41:32, rkaplow wrote:
> On 2017/02/02 18:29:25, tbansal1 wrote:
> > On 2017/02/02 18:19:31, rkaplow wrote:
> > > On 2017/02/02 17:43:50, Bryan McQuade wrote:
> > > > LGTM, thanks! I suppose the only risk here is that the network providers
> > > assume
> > > > they're only instantiated once. I didn't see any evidence of that in the
> > > > implementations, but might be worth double checking with those who know
> more
> > > > (tbansal?).
> > > 
> > > tbansal mind taking a look in case there's something we're missing?
> > 
> > IIUC, Bryan is correct. At least the network metrics provider
> > keeps track of state between UMA pings. e.g., if the connection
> > type changes between two consecutive UMA pings, the connection
> > type state in the provider is marked as ambiguous.
> > 
> > If UKM and MetricsService query the same provider, then these
> > internal states would get messed up.
> 
> I see. So this solution which instantiates separate providers is best then.

Correct. Other option is to build knowledge in the provider about who
is calling it (UKM vs. MetricsService) which is very messy.

[rkaplow, 2017-02-03 20:36:27]

PTAL

[Steven Holte, 2017-02-04 00:09:38]

lgtm

[Alexei Svitkine (slow), 2017-02-06 16:07:40]

lgtm

https://codereview.chromium.org/2671603002/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/2671603002/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:105: // Register
providers to the MetricsService. These provide data from alternate
Nit: Registers

https://codereview.chromium.org/2671603002/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:107: void
RegisterMetricsServiceProviders();
Nit: Add a new line after this.

https://codereview.chromium.org/2671603002/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:108: // Register
providers to the UkmService. These provide data from alternate
Nit: Registers

https://codereview.chromium.org/2671603002/diff/40001/components/ukm/ukm_serv...
File components/ukm/ukm_service.h (right):

https://codereview.chromium.org/2671603002/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:65: // Register the specified |provider| to provide
additional metrics into the
Nit: Registers

Also can you change the tense of the two method comments above for Flush and
Purge?

[rkaplow, 2017-02-06 16:31:41]

https://codereview.chromium.org/2671603002/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/2671603002/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:105: // Register
providers to the MetricsService. These provide data from alternate
On 2017/02/06 16:07:39, Alexei Svitkine (very slow) wrote:
> Nit: Registers

Done.

https://codereview.chromium.org/2671603002/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:107: void
RegisterMetricsServiceProviders();
On 2017/02/06 16:07:39, Alexei Svitkine (very slow) wrote:
> Nit: Add a new line after this.

Done.

https://codereview.chromium.org/2671603002/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:108: // Register
providers to the UkmService. These provide data from alternate
On 2017/02/06 16:07:39, Alexei Svitkine (very slow) wrote:
> Nit: Registers

Done.

https://codereview.chromium.org/2671603002/diff/40001/components/ukm/ukm_serv...
File components/ukm/ukm_service.h (right):

https://codereview.chromium.org/2671603002/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:65: // Register the specified |provider| to provide
additional metrics into the
On 2017/02/06 16:07:40, Alexei Svitkine (very slow) wrote:
> Nit: Registers
> 
> Also can you change the tense of the two method comments above for Flush and
> Purge?

fixed those and a few other comment

[rkaplow, 2017-02-06 17:54:21]

The CQ bit was checked by rkaplow@chromium.org

[rkaplow, 2017-02-06 17:54:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, asvitkine@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2671603002/#ps60001
(title: "fix comments and includes")

[commit-bot: I haz the power, 2017-02-06 17:54:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 19:00:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486403661549230,
"parent_rev": "06742d0d6225a9b04cb79a1204630f9b828cedd2", "commit_rev":
"b8d63f435e6c869164a0ec63eec5be867d2b7157"}

[commit-bot: I haz the power, 2017-02-06 19:01:25]

Description was changed from

==========
Add network information to UKM

Setup metric providers for UKM logs, and add network as a first client.
Small refactor of ChromeMetricsServiceClient::Initialize to put the
registrations in their own methods.


BUG=678682
==========

to

==========
Add network information to UKM

Setup metric providers for UKM logs, and add network as a first client.
Small refactor of ChromeMetricsServiceClient::Initialize to put the
registrations in their own methods.


BUG=678682

Review-Url: https://codereview.chromium.org/2671603002
Cr-Commit-Position: refs/heads/master@{#448330}
Committed:
https://chromium.googlesource.com/chromium/src/+/b8d63f435e6c869164a0ec63eec5...
==========

[commit-bot: I haz the power, 2017-02-06 19:01:26]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b8d63f435e6c869164a0ec63eec5...