Add UKM initial_url and session_id fields.

Add UKM initial_url and session_id fields.

This change adds support for logging the following new fields in UKM:
1. initial_url: the initial URL of a source, in cases where the URL of a source
   changes
2. session_id: a counter that's incremented for each browsing session (similar
   to the UMA session_id field)

Logging of both of these fields is gated on field trial params, and is disabled
by default. We'll start logging once the server-side changes for these fields are
in place.

BUG=697944
Review-Url: https://codereview.chromium.org/2722983006
Cr-Commit-Position: refs/heads/master@{#454382}
Committed: https://chromium.googlesource.com/chromium/src/+/7894b9acb1c4de15bf778042b826a72f3e358069

[Bryan McQuade, 2017-03-02 15:31:08]

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

[commit-bot: I haz the power, 2017-03-02 15:31:29]

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-03-02 16:18:17]

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

[commit-bot: I haz the power, 2017-03-02 16:18:18]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-03-02 16:21:14]

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

[commit-bot: I haz the power, 2017-03-02 16:21:34]

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

[Bryan McQuade, 2017-03-02 16:51:44]

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

[commit-bot: I haz the power, 2017-03-02 16:52:08]

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

[Bryan McQuade, 2017-03-02 17:34:54]

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

[commit-bot: I haz the power, 2017-03-02 17:35:03]

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-03-02 17:39:57]

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

[commit-bot: I haz the power, 2017-03-02 17:39:57]

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

[Bryan McQuade, 2017-03-02 17:50:59]

Description was changed from

==========
Add UKM initial_url and session_id fields.

BUG=
==========

to

==========
Add UKM initial_url and session_id fields.

BUG=697944
==========

[Bryan McQuade, 2017-03-02 17:54:44]

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

[commit-bot: I haz the power, 2017-03-02 17:55:03]

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

[Bryan McQuade, 2017-03-02 18:01:37]

Description was changed from

==========
Add UKM initial_url and session_id fields.

BUG=697944
==========

to

==========
Add UKM initial_url and session_id fields.

This change adds support for logging the following new fields in UKM:
1. initial_url: the initial URL of a source, in cases where the URL of a source
   changes
2. session_id: a counter that's incremented for each browsing session (similar
   to the UMA session_id field)

Logging of both of these fields is gated on field trial params, and is disabled
by default. We'll start logging once the server-side changes for these fields
are
in place.

BUG=697944
==========

[Bryan McQuade, 2017-03-02 18:01:48]

Description was changed from

==========
Add UKM initial_url and session_id fields.

This change adds support for logging the following new fields in UKM:
1. initial_url: the initial URL of a source, in cases where the URL of a source
   changes
2. session_id: a counter that's incremented for each browsing session (similar
   to the UMA session_id field)

Logging of both of these fields is gated on field trial params, and is disabled
by default. We'll start logging once the server-side changes for these fields
are
in place.

BUG=697944
==========

to

==========
Add UKM initial_url and session_id fields.

This change adds support for logging the following new fields in UKM:
1. initial_url: the initial URL of a source, in cases where the URL of a source
   changes
2. session_id: a counter that's incremented for each browsing session (similar
   to the UMA session_id field)

Logging of both of these fields is gated on field trial params, and is disabled
by default. We'll start logging once the server-side changes for these fields
are
in place.

BUG=697944
==========

[Bryan McQuade, 2017-03-02 18:01:57]

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

[Bryan McQuade, 2017-03-02 18:02:05]

rkaplow, PTAL, thanks!

[commit-bot: I haz the power, 2017-03-02 18:19:41]

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

[commit-bot: I haz the power, 2017-03-02 18:19:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Bryan McQuade, 2017-03-02 18:21:09]

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

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service_unittest.cc:33: class ScopedUkmFeatureParams {
just to give credit, this class is based on a similar class in
components/subresource_filter/core/browser/subresource_filter_features_test_support.h

[rkaplow, 2017-03-02 18:46:54]

lgtm

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

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:98: pref_service->SetInteger(prefs::kUkmSessionId,
0);
I don't super love the setting of session inside a method called
GenerateClientId, similar later on we load 
session_id_ = LoadSessionId(pref_service_);

within ResetClientID. It's not terrible but it's a bit hidden.



I wonder if it would be more straightforward to replace this with a method
called

ResetClientState, with everything the same except the return being void.

Anyways this is a bit minor. This still LG but if you agree that version would
be better I'd add a TODO

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

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service_unittest.cc:33: class ScopedUkmFeatureParams {
On 2017/03/02 18:21:08, Bryan McQuade wrote:
> just to give credit, this class is based on a similar class in
>
components/subresource_filter/core/browser/subresource_filter_features_test_support.h

This isn't really UKM specific either. If this has been useful, maybe we should
move this into variations? Can you add a TODO on this and assign to me?

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_sour...
File components/ukm/ukm_source.h (right):

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_sour...
components/ukm/ukm_source.h:31: void UpdateUrl(const GURL& url);
can you comment on this (with some init url details)

[Bryan McQuade, 2017-03-02 19:00:28]

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

[commit-bot: I haz the power, 2017-03-02 19:00:49]

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

[Bryan McQuade, 2017-03-02 19:07:38]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-03-02 19:07:39]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2722983006/#ps120001
(title: "address comments")

[commit-bot: I haz the power, 2017-03-02 19:08:16]

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-03-02 19:34:06]

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

[commit-bot: I haz the power, 2017-03-02 19:34:07]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Bryan McQuade, 2017-03-02 20:06:18]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-03-02 20:06:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2722983006/#ps140001
(title: "fix android build take 2")

[commit-bot: I haz the power, 2017-03-02 20:07:00]

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-03-02 21:31:17]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488485178149490,
"parent_rev": "e2cdb910981e41e82987ca6d46ac64633e42637d", "commit_rev":
"7894b9acb1c4de15bf778042b826a72f3e358069"}

[commit-bot: I haz the power, 2017-03-02 21:32:01]

Description was changed from

==========
Add UKM initial_url and session_id fields.

This change adds support for logging the following new fields in UKM:
1. initial_url: the initial URL of a source, in cases where the URL of a source
   changes
2. session_id: a counter that's incremented for each browsing session (similar
   to the UMA session_id field)

Logging of both of these fields is gated on field trial params, and is disabled
by default. We'll start logging once the server-side changes for these fields
are
in place.

BUG=697944
==========

to

==========
Add UKM initial_url and session_id fields.

This change adds support for logging the following new fields in UKM:
1. initial_url: the initial URL of a source, in cases where the URL of a source
   changes
2. session_id: a counter that's incremented for each browsing session (similar
   to the UMA session_id field)

Logging of both of these fields is gated on field trial params, and is disabled
by default. We'll start logging once the server-side changes for these fields
are
in place.

BUG=697944

Review-Url: https://codereview.chromium.org/2722983006
Cr-Commit-Position: refs/heads/master@{#454382}
Committed:
https://chromium.googlesource.com/chromium/src/+/7894b9acb1c4de15bf778042b826...
==========

[commit-bot: I haz the power, 2017-03-02 21:32:02]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/7894b9acb1c4de15bf778042b826...

[Bryan McQuade, 2017-03-02 22:18:32]

Forgot to post these comments earlier. I addressed them, thanks!

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

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:98: pref_service->SetInteger(prefs::kUkmSessionId,
0);
On 2017/03/02 at 18:46:54, rkaplow (slow) wrote:
> I don't super love the setting of session inside a method called
GenerateClientId, similar later on we load 
> session_id_ = LoadSessionId(pref_service_);
> 
> within ResetClientID. It's not terrible but it's a bit hidden.
> 
> 
> 
> I wonder if it would be more straightforward to replace this with a method
called
> 
> ResetClientState, with everything the same except the return being void.
> 
> Anyways this is a bit minor. This still LG but if you agree that version would
be better I'd add a TODO

Yeah, I think it makes sense to rename to something more generic. I'll defer
that to another change since we're close to branch point, but I've added a TODO
for this.

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

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service_unittest.cc:33: class ScopedUkmFeatureParams {
On 2017/03/02 at 18:46:54, rkaplow (slow) wrote:
> On 2017/03/02 18:21:08, Bryan McQuade wrote:
> > just to give credit, this class is based on a similar class in
> >
components/subresource_filter/core/browser/subresource_filter_features_test_support.h
> 
> This isn't really UKM specific either. If this has been useful, maybe we
should move this into variations? Can you add a TODO on this and assign to me?

Sure, I added a TODO for you.

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_sour...
File components/ukm/ukm_source.h (right):

https://codereview.chromium.org/2722983006/diff/80001/components/ukm/ukm_sour...
components/ukm/ukm_source.h:31: void UpdateUrl(const GURL& url);
On 2017/03/02 at 18:46:54, rkaplow (slow) wrote:
> can you comment on this (with some init url details)

Done