Basic UkmService implementation

Basic UkmService implementation

A basic Url-Keyed Metrics (UKM) service behind a flag.

This implementation will send messages to the UKM server which only contain UKM client ids.  Other contents are left for a later CL.

This version includes a manual copy of the protobuf specs, adapted
for the chromium tree, which we'll want to replace with some more
automated sync later.

BUG=673020
Review-Url: https://codereview.chromium.org/2567263003
Cr-Commit-Position: refs/heads/master@{#445535}
Committed: https://chromium.googlesource.com/chromium/src/+/7b74c62cf72ddd74d30777d71898556c8a0eb499

[Zhen Wang, 2016-12-15 20:10:02]

Description was changed from

==========
Basic UkmService implementation

(This is currently incomplete, and not ready for submit)

BUG=673020
==========

to

==========
Basic UkmService implementation

(This is currently incomplete, and not ready for submit)

BUG=673020
==========

[Steven Holte, 2016-12-29 00:37:23]

Description was changed from

==========
Basic UkmService implementation

(This is currently incomplete, and not ready for submit)

BUG=673020
==========

to

==========
Basic UkmService implementation

A basic UKM service behind a flag.

This implementation will send messages to the UKM server which only contain UKM
client ids.  Other contents are left for a later CL.

This version includes a manual copy of the protobuf specs, adapted
for the chromium tree, which we'll want to replace with some more
automated sync later.

BUG=673020
==========

[Steven Holte, 2016-12-29 00:37:23]

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

[rkaplow, 2017-01-02 23:23:04]

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/metr...
components/metrics/metrics_service_client.h:23: namespace ukm {
nit, alpha

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/pers...
File components/metrics/persisted_logs.cc (right):

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/pers...
components/metrics/persisted_logs.cc:99: if (outdated_pref_name_ &&
local_state_->HasPrefPath(outdated_pref_name_))
how is this related?

https://codereview.chromium.org/2567263003/diff/60001/components/metrics_serv...
File components/metrics_services_manager/metrics_services_manager.h (right):

https://codereview.chromium.org/2567263003/diff/60001/components/metrics_serv...
components/metrics_services_manager/metrics_services_manager.h:67: // Returns
the RapporServiceImpl, creating it if it hasn't been created yet.
update comment

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/BUILD.gn
File components/ukm/BUILD.gn (right):

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/BUILD.gn...
components/ukm/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
nit 2017, also elsewhere

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/OWNERS
File components/ukm/OWNERS (right):

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/OWNERS#n...
components/ukm/OWNERS:2: rkaplow@chromium.org
=alpha

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/persiste...
File components/ukm/persisted_logs_metrics_impl.cc (right):

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/persiste...
components/ukm/persisted_logs_metrics_impl.cc:27:
UMA_HISTOGRAM_COUNTS("UKM.UnsentLogs.Dropped.Size",
nit, we should use _number for these macros. Looking at the UMA ones, 1M (which
this is) seems good

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/persiste...
components/ukm/persisted_logs_metrics_impl.cc:32:
UMA_HISTOGRAM_COUNTS("UKM.UnsentLogs.Dropped", dropped_logs_num);
Looking at the UMA ones, I'd suggest using 10000 is probably ok

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:38: // Constant for the finch parameter name for
the server URL
prefer not using finch since it's technically internal, would use variations.
also trailing period.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:41: // The ukm server's URL.
UKM

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:44: // The number of UKM logs that must be stored.
comment

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:47: // The number of UKM logs that must be stored.
can you comment more on this

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:50: // The number of bytes UKM logs that must be
stored.
would reword

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:53: const int kMinPersistedBytes = 300000;
how did you get this value.

I also think it might make sense to make some of these constant parametrized in
case we want to test them - for example this and the # persisted logs and the
max upload log size

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:59: const size_t kUploadLogAvoidRetransmitSize =
100 * 1024;
i see this is the same name as ni metrics_service but I kind of feel 'Max'
should be in the name somewhere. might be better to be consistant though

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:66: else
nit, would prefer omitting the else

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:70: uint64_t LoadClientId(PrefService*
pref_service) {
i wonder if this names hides a bit too much logic. Could be
LoadOrGenerateClientId - but that is a bit wordy. Up to you.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:74: client_id = base::RandUint64();
i know we keep a string GUID for UMA, which ends up being hashed. under the hood
GUID uses this method, so I assume it will work out to basically the same.

So this seems fine byut maybe there's some reason we should store as a string,
maybe alexei has more context in the history of the UMA id.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:102: "Ukm.PersistedLogs.DeserializeLogs.Status",
UKM

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:194: persisted_logs_.StageLog();
just as a reminder can you add a TODO for various bandwidth issues (cellular,
etc.)

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:212: // Log a histogram to track response success
vs. failure rates.
I'd omit this comment - in the original metrics_service it made more sense since
the code was filtered but since we don't filter it isn't helpful

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:219: const size_t log_size =
persisted_logs_.staged_log().length();
nit, would add _bytes for clarity

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:223:
UMA_HISTOGRAM_COUNTS("UKM.OversizeRejected.LogSize",
prefer UMA_HISTOGRAM_COUNTS_1M

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:228: discard_log = true;
should we add a metric to see if this occurs?

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:243: client_->OnLogUploadComplete();
I was looking for what this was doing and it seems like it's recording some
extra UMA metrics:

https://codesearch.chromium.org/chromium/src/chrome/browser/metrics/chrome_me...

which calls this:
https://codesearch.chromium.org/chromium/src/chrome/browser/metrics/time_tick...

I don't think this is needed here (and in fact not sure this is needed at all,
we should follow up since the metrics are unowned)

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

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:40: // Enable/disable transmission of accumulated
logs.
it may be worth mentioning what would happen to logs that have already been
collected in the meanwhile, would they persist to the next enabling, etc.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:52: // Called when initialization tasks are
complete.
Prefer if this described what this method actually does rather than when it
should be used, or at least in addition to

https://codereview.chromium.org/2567263003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2567263003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68606: + 
<owner>holte@chromium.org</owner>
may want to add me as well to these

https://codereview.chromium.org/2567263003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68645: +<histogram
name="UKM.UnsentLogs.Dropped">
nit, i would probably have named these as 
UnsentLogs.NumDropped
UnsentLogs.DroppedSize (the extra . seems excessive)

but up to you

[Steven Holte, 2017-01-03 21:16:04]

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/metr...
components/metrics/metrics_service_client.h:23: namespace ukm {
On 2017/01/02 23:23:03, rkaplow wrote:
> nit, alpha

base < ukm ?

or do you want two namespace metrics blocks?

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/pers...
File components/metrics/persisted_logs.cc (right):

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/pers...
components/metrics/persisted_logs.cc:99: if (outdated_pref_name_ &&
local_state_->HasPrefPath(outdated_pref_name_))
On 2017/01/02 23:23:03, rkaplow wrote:
> how is this related?

This is allowing Persisted logs to be created with a NULL outdated_pref_name_,
since there is no outdated_pref_name for UKM persisted logs.

https://codereview.chromium.org/2567263003/diff/60001/components/metrics_serv...
File components/metrics_services_manager/metrics_services_manager.h (right):

https://codereview.chromium.org/2567263003/diff/60001/components/metrics_serv...
components/metrics_services_manager/metrics_services_manager.h:67: // Returns
the RapporServiceImpl, creating it if it hasn't been created yet.
On 2017/01/02 23:23:03, rkaplow wrote:
> update comment

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/OWNERS
File components/ukm/OWNERS (right):

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/OWNERS#n...
components/ukm/OWNERS:2: rkaplow@chromium.org
On 2017/01/02 23:23:03, rkaplow wrote:
> =alpha

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/persiste...
File components/ukm/persisted_logs_metrics_impl.cc (right):

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/persiste...
components/ukm/persisted_logs_metrics_impl.cc:27:
UMA_HISTOGRAM_COUNTS("UKM.UnsentLogs.Dropped.Size",
On 2017/01/02 23:23:03, rkaplow wrote:
> nit, we should use _number for these macros. Looking at the UMA ones, 1M
(which
> this is) seems good

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/persiste...
components/ukm/persisted_logs_metrics_impl.cc:32:
UMA_HISTOGRAM_COUNTS("UKM.UnsentLogs.Dropped", dropped_logs_num);
On 2017/01/02 23:23:03, rkaplow wrote:
> Looking at the UMA ones, I'd suggest using 10000 is probably ok

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:38: // Constant for the finch parameter name for
the server URL
On 2017/01/02 23:23:04, rkaplow wrote:
> prefer not using finch since it's technically internal, would use variations.
> also trailing period.

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:41: // The ukm server's URL.
On 2017/01/02 23:23:03, rkaplow wrote:
> UKM

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:44: // The number of UKM logs that must be stored.
On 2017/01/02 23:23:04, rkaplow wrote:
> comment

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:47: // The number of UKM logs that must be stored.
On 2017/01/02 23:23:04, rkaplow wrote:
> can you comment more on this

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:50: // The number of bytes UKM logs that must be
stored.
On 2017/01/02 23:23:04, rkaplow wrote:
> would reword 

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:53: const int kMinPersistedBytes = 300000;
On 2017/01/02 23:23:04, rkaplow wrote:
> how did you get this value.
> 
> I also think it might make sense to make some of these constant parametrized
in
> case we want to test them - for example this and the # persisted logs and the
> max upload log size

I've just copied these values from the ones being used in UMA for ongoing logs.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:59: const size_t kUploadLogAvoidRetransmitSize =
100 * 1024;
On 2017/01/02 23:23:03, rkaplow wrote:
> i see this is the same name as ni metrics_service but I kind of feel 'Max'
> should be in the name somewhere. might be better to be consistant though

Renamed to kMaxLogRetransmitSize

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:66: else
On 2017/01/02 23:23:04, rkaplow wrote:
> nit, would prefer omitting the else

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:70: uint64_t LoadClientId(PrefService*
pref_service) {
On 2017/01/02 23:23:04, rkaplow wrote:
> i wonder if this names hides a bit too much logic. Could be
> LoadOrGenerateClientId - but that is a bit wordy. Up to you.

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:74: client_id = base::RandUint64();
On 2017/01/02 23:23:04, rkaplow wrote:
> i know we keep a string GUID for UMA, which ends up being hashed. under the
hood
> GUID uses this method, so I assume it will work out to basically the same.
> 
> So this seems fine byut maybe there's some reason we should store as a string,
> maybe alexei has more context in the history of the UMA id.

GUIDs are actually 128 bits, our current proto only has a 64 bits.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:102: "Ukm.PersistedLogs.DeserializeLogs.Status",
On 2017/01/02 23:23:04, rkaplow wrote:
> UKM

Removed this metric, since it's already recorded by the
PersistedLogsMetricsImpl.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:194: persisted_logs_.StageLog();
On 2017/01/02 23:23:03, rkaplow wrote:
> just as a reminder can you add a TODO for various bandwidth issues (cellular,
> etc.)

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:212: // Log a histogram to track response success
vs. failure rates.
On 2017/01/02 23:23:04, rkaplow wrote:
> I'd omit this comment - in the original metrics_service it made more sense
since
> the code was filtered but since we don't filter it isn't helpful

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:219: const size_t log_size =
persisted_logs_.staged_log().length();
On 2017/01/02 23:23:04, rkaplow wrote:
> nit, would add _bytes for clarity

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:223:
UMA_HISTOGRAM_COUNTS("UKM.OversizeRejected.LogSize",
On 2017/01/02 23:23:03, rkaplow wrote:
> prefer UMA_HISTOGRAM_COUNTS_1M

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:228: discard_log = true;
On 2017/01/02 23:23:04, rkaplow wrote:
> should we add a metric to see if this occurs?

This should be covered by the ResponseCode histogram.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:243: client_->OnLogUploadComplete();
On 2017/01/02 23:23:04, rkaplow wrote:
> I was looking for what this was doing and it seems like it's recording some
> extra UMA metrics:
> 
>
https://codesearch.chromium.org/chromium/src/chrome/browser/metrics/chrome_me...
> 
> which calls this:
>
https://codesearch.chromium.org/chromium/src/chrome/browser/metrics/time_tick...
> 
> I don't think this is needed here (and in fact not sure this is needed at all,
> we should follow up since the metrics are unowned)
> 

Done.

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

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:40: // Enable/disable transmission of accumulated
logs.
On 2017/01/02 23:23:04, rkaplow wrote:
> it may be worth mentioning what would happen to logs that have already been
> collected in the meanwhile, would they persist to the next enabling, etc.

Done.

https://codereview.chromium.org/2567263003/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:52: // Called when initialization tasks are
complete.
On 2017/01/02 23:23:04, rkaplow wrote:
> Prefer if this described what this method actually does rather than when it
> should be used, or at least in addition to

Done.

https://codereview.chromium.org/2567263003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2567263003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68606: + 
<owner>holte@chromium.org</owner>
On 2017/01/02 23:23:04, rkaplow wrote:
> may want to add me as well to these

Done.

https://codereview.chromium.org/2567263003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68645: +<histogram
name="UKM.UnsentLogs.Dropped">
On 2017/01/02 23:23:04, rkaplow wrote:
> nit, i would probably have named these as 
> UnsentLogs.NumDropped
> UnsentLogs.DroppedSize (the extra . seems excessive)
> 
> but up to you

Done.

[rkaplow, 2017-01-04 17:57:57]

lgtm

lgtm but probably should have alexei review

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/metr...
components/metrics/metrics_service_client.h:23: namespace ukm {
On 2017/01/03 21:16:03, Steven Holte wrote:
> On 2017/01/02 23:23:03, rkaplow wrote:
> > nit, alpha
> 
> base < ukm ?
> 
> or do you want two namespace metrics blocks?

oh NM - misread that the metrics namespace was used for the rest of the file,
thought it should be above. Not sure if the style suggests doing 2 metrics
namespaces - either seems fine to me.

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/prot...
File components/metrics/proto/ukm/source.proto (right):

https://codereview.chromium.org/2567263003/diff/60001/components/metrics/prot...
components/metrics/proto/ukm/source.proto:18: // appear in FirstTimes, it should
be stripped before recording to logs.
I would reword this comment to not mention internal logic.

https://codereview.chromium.org/2567263003/diff/80001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:107: // MetricsServiceClient outlives
MetricsService, and
UKMService

https://codereview.chromium.org/2567263003/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:193: // TODO (holte): Handle data usage on
cellular, etc.
extra ws

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

https://codereview.chromium.org/2567263003/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:26: // Manages the generation and uploading of UKM
reports.
I'd suggest a bit more top-level documentation on what this is about here

[Steven Holte, 2017-01-04 22:47:55]

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

[Steven Holte, 2017-01-04 22:47:55]

+asvitkine

Will rebase after these CLs land:
https://codereview.chromium.org/2610653003/
https://codereview.chromium.org/2614533002/

https://codereview.chromium.org/2567263003/diff/80001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:107: // MetricsServiceClient outlives
MetricsService, and
On 2017/01/04 17:57:57, rkaplow wrote:
> UKMService

Done.

https://codereview.chromium.org/2567263003/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:193: // TODO (holte): Handle data usage on
cellular, etc.
On 2017/01/04 17:57:57, rkaplow wrote:
> extra ws

Done.

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

https://codereview.chromium.org/2567263003/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:26: // Manages the generation and uploading of UKM
reports.
On 2017/01/04 17:57:57, rkaplow wrote:
> I'd suggest a bit more top-level documentation on what this is about here

Expanded this a bit.

[Steven Holte, 2017-01-05 19:29:27]

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

[Alexei Svitkine (slow), 2017-01-06 16:34:32]

https://codereview.chromium.org/2567263003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2567263003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:568: ukm_service_.reset(
Should we gate this on a base::Feature?

https://codereview.chromium.org/2567263003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:569: new
ukm::UkmService(local_state, this));
Nit: Fits on line above?

https://codereview.chromium.org/2567263003/diff/100001/components/metrics/met...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2567263003/diff/100001/components/metrics/met...
components/metrics/metrics_service_client.h:45: virtual ukm::UkmService*
GetUkmService() = 0;
There's a bunch of other implementations of MetricsServiceClient - e.g. cast, aw
(android webview), blimp (maybe going away?), ios.

Probably we're OK with most of those not having one (except for iOS). If so,
please document that it's valid for this to be null.

Also, maybe provide a default implementation that returns null so that you don't
have to change all those subclasses?

But you *should* change the iOS one, though!

[Steven Holte, 2017-01-06 20:21:26]

https://codereview.chromium.org/2567263003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2567263003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:568: ukm_service_.reset(
On 2017/01/06 16:34:32, Alexei Svitkine (slow) wrote:
> Should we gate this on a base::Feature?

Moved the feature check here.

https://codereview.chromium.org/2567263003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:569: new
ukm::UkmService(local_state, this));
On 2017/01/06 16:34:32, Alexei Svitkine (slow) wrote:
> Nit: Fits on line above?

Done.

https://codereview.chromium.org/2567263003/diff/100001/components/metrics/met...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/2567263003/diff/100001/components/metrics/met...
components/metrics/metrics_service_client.h:45: virtual ukm::UkmService*
GetUkmService() = 0;
On 2017/01/06 16:34:32, Alexei Svitkine (slow) wrote:
> There's a bunch of other implementations of MetricsServiceClient - e.g. cast,
aw
> (android webview), blimp (maybe going away?), ios.
> 
> Probably we're OK with most of those not having one (except for iOS). If so,
> please document that it's valid for this to be null.
> 
> Also, maybe provide a default implementation that returns null so that you
don't
> have to change all those subclasses?
> 
> But you *should* change the iOS one, though!

Made the default null, added null checks in metrics_service_manager, and added
iOS implementation.

[Alexei Svitkine (slow), 2017-01-10 17:37:19]

https://codereview.chromium.org/2567263003/diff/140001/components/metrics/met...
File components/metrics/metrics_service_client.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/metrics/met...
components/metrics/metrics_service_client.cc:12: return NULL;
nullptr

https://codereview.chromium.org/2567263003/diff/140001/components/metrics_ser...
File components/metrics_services_manager/metrics_services_manager.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/metrics_ser...
components/metrics_services_manager/metrics_services_manager.cc:106: if
(may_upload_) {
Add some TODOs that UKM needs to check sync state?

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/metrics...
File components/ukm/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/metrics...
components/ukm/metrics_reporting_scheduler.cc:9: using base::TimeDelta;
Nit: Remove

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/persist...
File components/ukm/persisted_logs_metrics_impl.h (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/persist...
components/ukm/persisted_logs_metrics_impl.h:17: ~PersistedLogsMetricsImpl() {}
override

and the base class's dtor should be marked virtual

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/persist...
components/ukm/persisted_logs_metrics_impl.h:21: 
Nit: No need for blank lines between override methods. Also put //
metrics::PersistedLogsMetrics: above them all.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_pre...
File components/ukm/ukm_pref_names.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_pre...
components/ukm/ukm_pref_names.cc:10: // A random value unique for each chrome
install.
Mention what the type is.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:62: kUkmRolloutFieldTrialName,
kUkmRolloutServerUrlParam);
Nit: Use GetVariationParamValueByFeature() and then you can remove
UkmRolloutFieldTrialName.

Also, I would just inline "ServerUrl" here since that's the only place you use
it.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:70: if (!client_id) {
What if 0 is legitimately generated? You can use  FindPreference() to see if it
exists.

(The other alternative is to explicitly disallow 0 and do a loop while
generating it to exclude 0's).

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:73: pref_service->SetInteger(prefs::kUkmClientId,
client_id);
SetInt64() and GetInt64()

Integer may be 32 bit otherwise.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:211: } else if (log_size_bytes >
kMaxLogRetransmitSize) {
Nit: Wondering if we really need it? I would suggest leaving this logic out for
now until/if we find we need it.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
File components/ukm/ukm_service_unittest.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service_unittest.cc:11: #include <utility>
I don't think all these includes should be needed with the current state of this
file...

[Steven Holte, 2017-01-10 22:52:45]

https://codereview.chromium.org/2567263003/diff/140001/components/metrics/met...
File components/metrics/metrics_service_client.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/metrics/met...
components/metrics/metrics_service_client.cc:12: return NULL;
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> nullptr

Done.

https://codereview.chromium.org/2567263003/diff/140001/components/metrics_ser...
File components/metrics_services_manager/metrics_services_manager.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/metrics_ser...
components/metrics_services_manager/metrics_services_manager.cc:106: if
(may_upload_) {
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> Add some TODOs that UKM needs to check sync state?

Done

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/metrics...
File components/ukm/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/metrics...
components/ukm/metrics_reporting_scheduler.cc:9: using base::TimeDelta;
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> Nit: Remove

Done.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/persist...
File components/ukm/persisted_logs_metrics_impl.h (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/persist...
components/ukm/persisted_logs_metrics_impl.h:17: ~PersistedLogsMetricsImpl() {}
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> override
> 
> and the base class's dtor should be marked virtual

Done.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/persist...
components/ukm/persisted_logs_metrics_impl.h:21: 
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> Nit: No need for blank lines between override methods. Also put //
> metrics::PersistedLogsMetrics: above them all.

Done.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_pre...
File components/ukm/ukm_pref_names.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_pre...
components/ukm/ukm_pref_names.cc:10: // A random value unique for each chrome
install.
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> Mention what the type is.

Done.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:62: kUkmRolloutFieldTrialName,
kUkmRolloutServerUrlParam);
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> Nit: Use GetVariationParamValueByFeature() and then you can remove
> UkmRolloutFieldTrialName.
> 
> Also, I would just inline "ServerUrl" here since that's the only place you use
> it.

Done.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:70: if (!client_id) {
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> What if 0 is legitimately generated? You can use  FindPreference() to see if
it
> exists.
> 
> (The other alternative is to explicitly disallow 0 and do a loop while
> generating it to exclude 0's).

Added the loop.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:73: pref_service->SetInteger(prefs::kUkmClientId,
client_id);
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> SetInt64() and GetInt64()
> 
> Integer may be 32 bit otherwise.

Done.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:211: } else if (log_size_bytes >
kMaxLogRetransmitSize) {
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> Nit: Wondering if we really need it? I would suggest leaving this logic out
for
> now until/if we find we need it.

Done.

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
File components/ukm/ukm_service_unittest.cc (right):

https://codereview.chromium.org/2567263003/diff/140001/components/ukm/ukm_ser...
components/ukm/ukm_service_unittest.cc:11: #include <utility>
On 2017/01/10 17:37:19, Alexei Svitkine (slow) wrote:
> I don't think all these includes should be needed with the current state of
this
> file...

Done.

[rkaplow, 2017-01-11 22:05:13]

lgtm

https://codereview.chromium.org/2567263003/diff/160001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2567263003/diff/160001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:569: if
(base::FeatureList::IsEnabled(kUkmFeature))
should be ukm::kUkmFeature

[Alexei Svitkine (slow), 2017-01-11 22:12:17]

lgtm

[Steven Holte, 2017-01-11 22:52:03]

holte@chromium.org changed reviewers:
+ battre@chromium.org, olivierrobin@chromium.org

[Steven Holte, 2017-01-11 22:52:03]

+battre for components/prefs DEP and protos
+olivierrobin for ios/chrome/browser/metrics

https://codereview.chromium.org/2567263003/diff/160001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2567263003/diff/160001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:569: if
(base::FeatureList::IsEnabled(kUkmFeature))
On 2017/01/11 22:05:13, rkaplow wrote:
> should be ukm::kUkmFeature

Done.

[rkaplow, 2017-01-11 23:03:54]

https://codereview.chromium.org/2567263003/diff/160001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/160001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:136:
registry->RegisterIntegerPref(prefs::kUkmClientId, 0);
ah, found another bug while testing locally. This should be 
registry->RegisterInt64Pref(prefs::kUkmClientId, 0);

[Steven Holte, 2017-01-11 23:50:40]

https://codereview.chromium.org/2567263003/diff/160001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/160001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:136:
registry->RegisterIntegerPref(prefs::kUkmClientId, 0);
On 2017/01/11 23:03:53, rkaplow wrote:
> ah, found another bug while testing locally. This should be 
> registry->RegisterInt64Pref(prefs::kUkmClientId, 0);

Done, and fixed a couple other small bugs.

[battre, 2017-01-12 08:12:56]

https://codereview.chromium.org/2567263003/diff/200001/components/metrics/pro...
File components/metrics/proto/ukm/source.proto (right):

https://codereview.chromium.org/2567263003/diff/200001/components/metrics/pro...
components/metrics/proto/ukm/source.proto:18: // appear in FirstTimes, it should
be stripped before recording to logs.
If the URL has not been discovered by Google's crawler, it must be stripped to
the security origin before recording to logs. If the security origin has not
been discovered by the crawler, the security origin must not be logged.

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/metrics...
File components/ukm/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/metrics...
components/ukm/metrics_reporting_scheduler.cc:17:
MetricsReportingScheduler::~MetricsReportingScheduler() {}
= default?

(also in other places)

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_pre...
File components/ukm/ukm_pref_names.cc (right):

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_pre...
components/ukm/ukm_pref_names.cc:14: const char kUkmPersistedLogs[] =
"ukm.persisted_logs";
Do we really want to store this in prefs? Any update will trigger a rewrite of
prefs and prefs are not particularly fast to read and write.

Also, please make sure that clearing your browsing data (i.e. history) erases
all buffered UKMs.

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:30: const char kMimeType[] =
"application/vnd.chrome.ukm";
I have seen a number of CLs, where this has been expressed as
constexpr char kMimeType[] = "...";

(same applies to the consts below)

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_ser...
File components/ukm/ukm_service_unittest.cc (right):

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_ser...
components/ukm/ukm_service_unittest.cc:18: }
add some more test coverage?

[Olivier, 2017-01-13 13:04:57]

ios lgtm

[Steven Holte, 2017-01-13 23:15:26]

https://codereview.chromium.org/2567263003/diff/200001/components/metrics/pro...
File components/metrics/proto/ukm/source.proto (right):

https://codereview.chromium.org/2567263003/diff/200001/components/metrics/pro...
components/metrics/proto/ukm/source.proto:18: // appear in FirstTimes, it should
be stripped before recording to logs.
On 2017/01/12 08:12:55, battre wrote:
> If the URL has not been discovered by Google's crawler, it must be stripped to
> the security origin before recording to logs. If the security origin has not
> been discovered by the crawler, the security origin must not be logged.

Done.

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/metrics...
File components/ukm/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/metrics...
components/ukm/metrics_reporting_scheduler.cc:17:
MetricsReportingScheduler::~MetricsReportingScheduler() {}
On 2017/01/12 08:12:55, battre wrote:
> = default?
> 
> (also in other places)

Done.

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_pre...
File components/ukm/ukm_pref_names.cc (right):

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_pre...
components/ukm/ukm_pref_names.cc:14: const char kUkmPersistedLogs[] =
"ukm.persisted_logs";
On 2017/01/12 08:12:55, battre wrote:
> Do we really want to store this in prefs? Any update will trigger a rewrite of
> prefs and prefs are not particularly fast to read and write.
> 
> Also, please make sure that clearing your browsing data (i.e. history) erases
> all buffered UKMs.

I'm pretty much just reusing the UMA logic here.  These logs are generated on a
similar interval to UMA (about every 5 minutes), and usually only get persisted
to disk on shutdown/background, so I wouldn't expect the pref write overhead to
be too large, but I'd be interested if there is a better suggestion for where to
persist buffered logs.

Added code to clear this data on Clear Browsing History.

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:30: const char kMimeType[] =
"application/vnd.chrome.ukm";
On 2017/01/12 08:12:55, battre wrote:
> I have seen a number of CLs, where this has been expressed as
> constexpr char kMimeType[] = "...";
> 
> (same applies to the consts below)

Done.

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_ser...
File components/ukm/ukm_service_unittest.cc (right):

https://codereview.chromium.org/2567263003/diff/200001/components/ukm/ukm_ser...
components/ukm/ukm_service_unittest.cc:18: }
On 2017/01/12 08:12:56, battre wrote:
> add some more test coverage?

Done.

[Steven Holte, 2017-01-14 00:01:37]

Added some code to observe history deletions on ios as well, but not sure how/if
I need to listen for additional browser states being created.

olivierrobin@ : Do you know if there is some event I should be listening for
this?

[Steven Holte, 2017-01-14 01:01:59]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-14 01:02:43]

Dry run: 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-01-14 01:05:06]

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

[commit-bot: I haz the power, 2017-01-14 01:05:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Steven Holte, 2017-01-17 20:44:39]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 20:45:13]

Dry run: 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-01-17 20:55:20]

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

[commit-bot: I haz the power, 2017-01-17 20:55:21]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Steven Holte, 2017-01-19 20:07:07]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 20:07:46]

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

[Steven Holte, 2017-01-19 20:08:26]

battre@ PTAL

[commit-bot: I haz the power, 2017-01-19 21:04:02]

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

[commit-bot: I haz the power, 2017-01-19 21:04:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Steven Holte, 2017-01-20 00:31:53]

The CQ bit was checked by holte@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 00:32:58]

Dry run: 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-01-20 01:56:19]

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

[commit-bot: I haz the power, 2017-01-20 01:56:20]

Dry run: This issue passed the CQ dry run.

[battre, 2017-01-20 07:42:30]

LGTM with comments.

Can you please make sure that somewhere in the repository we explain what UKM
stands for? Maybe a readme, maybe in the UkmService header, maybe in the gn
file, or maybe in all three of these places.

https://codereview.chromium.org/2567263003/diff/440001/components/metrics_ser...
File components/metrics_services_manager/metrics_services_manager.cc (right):

https://codereview.chromium.org/2567263003/diff/440001/components/metrics_ser...
components/metrics_services_manager/metrics_services_manager.cc:108: //
TODO(holte): Make UKM checks sync state
Can you add something like this into EnableReporting of the UKM?

#ifdef OFFICIAL_BUILD
  CHECK(false);  // Remove this, once UKM checks the sync state (see
MetricsServicesManager::UpdateRunningServices())
#endif

https://codereview.chromium.org/2567263003/diff/440001/components/ukm/ukm_ser...
File components/ukm/ukm_service.h (right):

https://codereview.chromium.org/2567263003/diff/440001/components/ukm/ukm_ser...
components/ukm/ukm_service.h:31: // Manages the generation and uploading of UKM
reports.
UKM is not defined anywhere.

[Steven Holte, 2017-01-20 20:03:30]

Description was changed from

==========
Basic UkmService implementation

A basic UKM service behind a flag.

This implementation will send messages to the UKM server which only contain UKM
client ids.  Other contents are left for a later CL.

This version includes a manual copy of the protobuf specs, adapted
for the chromium tree, which we'll want to replace with some more
automated sync later.

BUG=673020
==========

to

==========
Basic UkmService implementation

A basic Url-Keyed Metrics (UKM) service behind a flag.

This implementation will send messages to the UKM server which only contain UKM
client ids.  Other contents are left for a later CL.

This version includes a manual copy of the protobuf specs, adapted
for the chromium tree, which we'll want to replace with some more
automated sync later.

BUG=673020
==========

[Steven Holte, 2017-01-20 20:07:37]

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

[Steven Holte, 2017-01-20 20:07:39]

+sdefresne for the dependency on components/history/core/browser from
components/ukm/observers

https://codereview.chromium.org/2567263003/diff/440001/components/metrics_ser...
File components/metrics_services_manager/metrics_services_manager.cc (right):

https://codereview.chromium.org/2567263003/diff/440001/components/metrics_ser...
components/metrics_services_manager/metrics_services_manager.cc:108: //
TODO(holte): Make UKM checks sync state
On 2017/01/20 07:42:30, battre wrote:
> Can you add something like this into EnableReporting of the UKM?
> 
> #ifdef OFFICIAL_BUILD
>   CHECK(false);  // Remove this, once UKM checks the sync state (see
> MetricsServicesManager::UpdateRunningServices())
> #endif

Done, and added a #if around the call in MSM to make sure it isn't called.

https://codereview.chromium.org/2567263003/diff/440001/components/ukm/ukm_ser...
File components/ukm/ukm_service.h (right):

https://codereview.chromium.org/2567263003/diff/440001/components/ukm/ukm_ser...
components/ukm/ukm_service.h:31: // Manages the generation and uploading of UKM
reports.
On 2017/01/20 07:42:30, battre wrote:
> UKM is not defined anywhere.

Rewrote this comment to define UKM, and included it in BUILD.gn as well.

[sdefresne, 2017-01-23 09:53:05]

DEPS on components/history/core/browser lgtm

Some other comments on the rest of the code though.

https://codereview.chromium.org/2567263003/diff/460001/components/ukm/observe...
File components/ukm/observers/history_delete_observer.h (right):

https://codereview.chromium.org/2567263003/diff/460001/components/ukm/observe...
components/ukm/observers/history_delete_observer.h:38:
std::set<history::HistoryService*> observed_history_services_;
I would recommend using ScopedObserver here. It already implements listening to
multiple object, and cleanup in its destructor (see base/scoped_observer.h).

  ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
      scoped_observer_;

https://codereview.chromium.org/2567263003/diff/460001/ios/chrome/browser/met...
File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm (right):

https://codereview.chromium.org/2567263003/diff/460001/ios/chrome/browser/met...
ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm:198:
ukm_service_.reset(new ukm::UkmService(local_state, this));
Please use base::MakeUnique here:

  if (base::FeatureList::IsEnabled(ukm:kUkmFeature))
    ukm_service_ = base::MakeUnique<ukm::UkmService>(local_state, this);

[Steven Holte, 2017-01-23 20:37:30]

https://codereview.chromium.org/2567263003/diff/460001/components/ukm/observe...
File components/ukm/observers/history_delete_observer.h (right):

https://codereview.chromium.org/2567263003/diff/460001/components/ukm/observe...
components/ukm/observers/history_delete_observer.h:38:
std::set<history::HistoryService*> observed_history_services_;
On 2017/01/23 09:53:04, sdefresne wrote:
> I would recommend using ScopedObserver here. It already implements listening
to
> multiple object, and cleanup in its destructor (see base/scoped_observer.h).
> 
>   ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
>       scoped_observer_;

Done, thanks for the tip.

https://codereview.chromium.org/2567263003/diff/460001/ios/chrome/browser/met...
File ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm (right):

https://codereview.chromium.org/2567263003/diff/460001/ios/chrome/browser/met...
ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm:198:
ukm_service_.reset(new ukm::UkmService(local_state, this));
On 2017/01/23 09:53:04, sdefresne wrote:
> Please use base::MakeUnique here:
> 
>   if (base::FeatureList::IsEnabled(ukm:kUkmFeature))
>     ukm_service_ = base::MakeUnique<ukm::UkmService>(local_state, this);

Done.

[Steven Holte, 2017-01-23 20:39:46]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2017-01-23 20:39:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, asvitkine@chromium.org, olivierrobin@chromium.org,
battre@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2567263003/#ps500001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-23 20:40:27]

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-01-23 23:13:13]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1485203986580050,
"parent_rev": "fb9fa9e5693926b6807bd1f48db5b8a751c1aa51", "commit_rev":
"7b74c62cf72ddd74d30777d71898556c8a0eb499"}

[commit-bot: I haz the power, 2017-01-23 23:13:46]

Description was changed from

==========
Basic UkmService implementation

A basic Url-Keyed Metrics (UKM) service behind a flag.

This implementation will send messages to the UKM server which only contain UKM
client ids.  Other contents are left for a later CL.

This version includes a manual copy of the protobuf specs, adapted
for the chromium tree, which we'll want to replace with some more
automated sync later.

BUG=673020
==========

to

==========
Basic UkmService implementation

A basic Url-Keyed Metrics (UKM) service behind a flag.

This implementation will send messages to the UKM server which only contain UKM
client ids.  Other contents are left for a later CL.

This version includes a manual copy of the protobuf specs, adapted
for the chromium tree, which we'll want to replace with some more
automated sync later.

BUG=673020

Review-Url: https://codereview.chromium.org/2567263003
Cr-Commit-Position: refs/heads/master@{#445535}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b74c62cf72ddd74d30777d71898...
==========

[commit-bot: I haz the power, 2017-01-23 23:13:48]

Committed patchset #26 (id:500001) as
https://chromium.googlesource.com/chromium/src/+/7b74c62cf72ddd74d30777d71898...

[Nico, 2017-01-25 01:05:39]

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

[Nico, 2017-01-25 01:05:43]

https://codereview.chromium.org/2567263003/diff/500001/components/ukm/ukm_ser...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2567263003/diff/500001/components/ukm/ukm_ser...
components/ukm/ukm_service.cc:130: CHECK(false);  // Remove this once UKM checks
sync state.
This means that the tests this added fail on bots that do official builds, e.g.
here:

https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds...

Please make it so that this code doesn't break tests in official builds.

[Steven Holte, 2017-01-25 01:29:31]

On 2017/01/25 01:05:43, Nico wrote:
>
https://codereview.chromium.org/2567263003/diff/500001/components/ukm/ukm_ser...
> File components/ukm/ukm_service.cc (right):
> 
>
https://codereview.chromium.org/2567263003/diff/500001/components/ukm/ukm_ser...
> components/ukm/ukm_service.cc:130: CHECK(false);  // Remove this once UKM
checks
> sync state.
> This means that the tests this added fail on bots that do official builds,
e.g.
> here:
> 
>
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds...
> 
> Please make it so that this code doesn't break tests in official builds.

Doh.  Sent you https://codereview.chromium.org/2652013004/ to remove the check.