Move UKM PageLoad SourceId generation to PageLoadTracker.

Move UKM PageLoad SourceId generation to PageLoadTracker.

We'd like to enable all observers to record UKM data sharing the same SourceId,
so metrics from different observers can be queried/joined together.

This change migrates SourceId generation from the UKM observer to the
PageLoadTracker, so all observers can share the same SourceId.

BUG=727286
Review-Url: https://codereview.chromium.org/2909033002
Cr-Commit-Position: refs/heads/master@{#475606}
Committed: https://chromium.googlesource.com/chromium/src/+/0bff3c633799c676eadce377337cc24a27795151

[Bryan McQuade, 2017-05-29 11:31:05]

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

[commit-bot: I haz the power, 2017-05-29 11:31:15]

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-05-29 11:38:02]

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

[commit-bot: I haz the power, 2017-05-29 11:38:14]

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-05-29 11:44:01]

Description was changed from

==========
Move UKM PageLoad SourceId generation to PageLoadTracker.

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

to

==========
Move UKM PageLoad SourceId generation to PageLoadTracker.

We'd like to enable all observers to record UKM data sharing the same SourceId,
so metrics from different observers can be queried/joined together.

This change migrates SourceId generation from the UKM observer to the
PageLoadTracker, so all observers can share the same SourceId.

BUG=727286
==========

[commit-bot: I haz the power, 2017-05-29 12:22:19]

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

[commit-bot: I haz the power, 2017-05-29 12:22:20]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-05-30 16:26:22]

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

[RyanSturm, 2017-05-30 16:26:23]

lgtm % a couple of comments, but most of them are no-ops.

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:104:
ukm::SourceId source_id) override {
nit: For all of these changed files, I'd like the #include
"components/ukm/ukm_source.h" to explicit in each observer's header file instead
of included from page_load_metrics_observer.h.

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:338: if
(PageLoadMetricsObserver::IsStandardWebPageMimeType(mime_type))
So this would rule out offline previews. I'll file a bug around deciding if we
need a way to support offline previews with PLM UKM.

crbug.com/727724

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:375:
ukm_recorder->UpdateSourceURL(source_id_, start_url_);
nit: Maybe this should record the whole redirect chain (I'm not sure if it would
provide value later to see how many redirects we hit). Consider the case of
really slow FCP due to N redirects.

GetRedirectChain() is exposed off of NavigationHandle.

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.h (left):

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.h:294: void
MaybeUpdateURL(content::NavigationHandle* navigation_handle);
Was this ever defined?

[Bryan McQuade, 2017-05-30 17:16:35]

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

[commit-bot: I haz the power, 2017-05-30 17:17:01]

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-05-30 17:17:06]

Thanks!

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:104:
ukm::SourceId source_id) override {
On 2017/05/30 at 16:26:23, RyanSturm wrote:
> nit: For all of these changed files, I'd like the #include
"components/ukm/ukm_source.h" to explicit in each observer's header file instead
of included from page_load_metrics_observer.h.

Sure, done.

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:338: if
(PageLoadMetricsObserver::IsStandardWebPageMimeType(mime_type))
On 2017/05/30 at 16:26:23, RyanSturm wrote:
> So this would rule out offline previews. I'll file a bug around deciding if we
need a way to support offline previews with PLM UKM.
> 
> crbug.com/727724

Yeah, I think it should be easy to drop this requirement in the future, when
offline previews wants to log metrics. For now I decided to focus on the core
page loads.

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:375:
ukm_recorder->UpdateSourceURL(source_id_, start_url_);
On 2017/05/30 at 16:26:23, RyanSturm wrote:
> nit: Maybe this should record the whole redirect chain (I'm not sure if it
would provide value later to see how many redirects we hit). Consider the case
of really slow FCP due to N redirects.
> 
> GetRedirectChain() is exposed off of NavigationHandle.

Interestingly IIUC GetRedirectChain returns the list of client-side or
content-initiated redirections, e.g. <meta refresh> or JS-based location
changes. This is super subtle and confusing but if I'm remembering correctly
that's what it's intended for.

On the UKM side we decided that logging the first and last URLs struct a nice
balance between amount of data and usefulness. The UKM data format has 'url' and
'initial_url' fields only. I think we decided to avoid having an unbounded list
of URLs here.

What we could do is add a metric with the redirection count, to at least
understand the full number of redirects encountered.

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.h (left):

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.h:294: void
MaybeUpdateURL(content::NavigationHandle* navigation_handle);
On 2017/05/30 at 16:26:23, RyanSturm wrote:
> Was this ever defined?

I had added this method in a pending change, then during review refactored
things and got rid of it, but I seem to have forgotten to remove the method
declaration. This is just a cleanup to address that (I happened to notice it
while working on this change).

[RyanSturm, 2017-05-30 17:27:11]

still lgtm

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:375:
ukm_recorder->UpdateSourceURL(source_id_, start_url_);
On 2017/05/30 17:17:06, Bryan McQuade wrote:
> On 2017/05/30 at 16:26:23, RyanSturm wrote:
> > nit: Maybe this should record the whole redirect chain (I'm not sure if it
> would provide value later to see how many redirects we hit). Consider the case
> of really slow FCP due to N redirects.
> > 
> > GetRedirectChain() is exposed off of NavigationHandle.
> 
> Interestingly IIUC GetRedirectChain returns the list of client-side or
> content-initiated redirections, e.g. <meta refresh> or JS-based location
> changes. This is super subtle and confusing but if I'm remembering correctly
> that's what it's intended for.
> 
> On the UKM side we decided that logging the first and last URLs struct a nice
> balance between amount of data and usefulness. The UKM data format has 'url'
and
> 'initial_url' fields only. I think we decided to avoid having an unbounded
list
> of URLs here.
> 
> What we could do is add a metric with the redirection count, to at least
> understand the full number of redirects encountered.

I see this pseudo stack is possible from URLRequest:
URLRequest::NotifyReceivedRedirect
ResourceLoader::OnReceivedRedirect
NavigationResourceHandler::OnRequestRedirected
NavigationURLLoaderImplCore::NotifyRequestRedirected

Post to:

NavigationURLLoaderImpl::NotifyRequestRedirected
NavigationRequest::OnRequestRedirected
NavigationHandleImpl::WillRedirectRequest
redirect_chain_.push_back(new_url);

If client redirects happen later than OnCommit, this should be good.


Agreed that count is probably sufficient (but needs another CL and review from
UKM).

[Bryan McQuade, 2017-05-30 17:29:13]

On 2017/05/30 at 17:27:11, ryansturm wrote:
> still lgtm
> 
>
https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
> File chrome/browser/page_load_metrics/page_load_tracker.cc (right):
> 
>
https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/page_load_tracker.cc:375:
ukm_recorder->UpdateSourceURL(source_id_, start_url_);
> On 2017/05/30 17:17:06, Bryan McQuade wrote:
> > On 2017/05/30 at 16:26:23, RyanSturm wrote:
> > > nit: Maybe this should record the whole redirect chain (I'm not sure if it
> > would provide value later to see how many redirects we hit). Consider the
case
> > of really slow FCP due to N redirects.
> > > 
> > > GetRedirectChain() is exposed off of NavigationHandle.
> > 
> > Interestingly IIUC GetRedirectChain returns the list of client-side or
> > content-initiated redirections, e.g. <meta refresh> or JS-based location
> > changes. This is super subtle and confusing but if I'm remembering correctly
> > that's what it's intended for.
> > 
> > On the UKM side we decided that logging the first and last URLs struct a
nice
> > balance between amount of data and usefulness. The UKM data format has 'url'
and
> > 'initial_url' fields only. I think we decided to avoid having an unbounded
list
> > of URLs here.
> > 
> > What we could do is add a metric with the redirection count, to at least
> > understand the full number of redirects encountered.
> 
> I see this pseudo stack is possible from URLRequest:
> URLRequest::NotifyReceivedRedirect
> ResourceLoader::OnReceivedRedirect
> NavigationResourceHandler::OnRequestRedirected
> NavigationURLLoaderImplCore::NotifyRequestRedirected
> 
> Post to:
> 
> NavigationURLLoaderImpl::NotifyRequestRedirected
> NavigationRequest::OnRequestRedirected
> NavigationHandleImpl::WillRedirectRequest
> redirect_chain_.push_back(new_url);
> 
> If client redirects happen later than OnCommit, this should be good.
> 
> 
> Agreed that count is probably sufficient (but needs another CL and review from
UKM).

Thanks for digging into this! That's interesting - IIRC clamy had told me this
was used to track content initiated rather than normal HTTP redirects.

[RyanSturm, 2017-05-30 17:36:56]

On 2017/05/30 17:29:13, Bryan McQuade wrote:
> On 2017/05/30 at 17:27:11, ryansturm wrote:
> > still lgtm
> > 
> >
>
https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
> > File chrome/browser/page_load_metrics/page_load_tracker.cc (right):
> > 
> >
>
https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
> > chrome/browser/page_load_metrics/page_load_tracker.cc:375:
> ukm_recorder->UpdateSourceURL(source_id_, start_url_);
> > On 2017/05/30 17:17:06, Bryan McQuade wrote:
> > > On 2017/05/30 at 16:26:23, RyanSturm wrote:
> > > > nit: Maybe this should record the whole redirect chain (I'm not sure if
it
> > > would provide value later to see how many redirects we hit). Consider the
> case
> > > of really slow FCP due to N redirects.
> > > > 
> > > > GetRedirectChain() is exposed off of NavigationHandle.
> > > 
> > > Interestingly IIUC GetRedirectChain returns the list of client-side or
> > > content-initiated redirections, e.g. <meta refresh> or JS-based location
> > > changes. This is super subtle and confusing but if I'm remembering
correctly
> > > that's what it's intended for.
> > > 
> > > On the UKM side we decided that logging the first and last URLs struct a
> nice
> > > balance between amount of data and usefulness. The UKM data format has
'url'
> and
> > > 'initial_url' fields only. I think we decided to avoid having an unbounded
> list
> > > of URLs here.
> > > 
> > > What we could do is add a metric with the redirection count, to at least
> > > understand the full number of redirects encountered.
> > 
> > I see this pseudo stack is possible from URLRequest:
> > URLRequest::NotifyReceivedRedirect
> > ResourceLoader::OnReceivedRedirect
> > NavigationResourceHandler::OnRequestRedirected
> > NavigationURLLoaderImplCore::NotifyRequestRedirected
> > 
> > Post to:
> > 
> > NavigationURLLoaderImpl::NotifyRequestRedirected
> > NavigationRequest::OnRequestRedirected
> > NavigationHandleImpl::WillRedirectRequest
> > redirect_chain_.push_back(new_url);
> > 
> > If client redirects happen later than OnCommit, this should be good.
> > 
> > 
> > Agreed that count is probably sufficient (but needs another CL and review
from
> UKM).
> 
> Thanks for digging into this! That's interesting - IIRC clamy had told me this
> was used to track content initiated rather than normal HTTP redirects.

Pretty odd because it doesn't seem that way (AFAICT).

I printed during DidFinishNavigation and under plznavigate and non-plznavigate I
see:

[43747:43747:0530/103518.126366:WARNING:previews_infobar_tab_helper.cc(72)]
http://reddit.com/r/pics
[43747:43747:0530/103518.126495:WARNING:previews_infobar_tab_helper.cc(72)]
https://reddit.com/r/pics
[43747:43747:0530/103518.126538:WARNING:previews_infobar_tab_helper.cc(72)]
https://www.reddit.com/r/pics
[43747:43747:0530/103518.126578:WARNING:previews_infobar_tab_helper.cc(72)]
https://www.reddit.com/r/pics/

which are pretty typical server redirects, so I guess you should file a bug and
follow up with clamy@ about what she meant.

[RyanSturm, 2017-05-30 17:38:33]

On 2017/05/30 17:36:56, RyanSturm wrote:
> On 2017/05/30 17:29:13, Bryan McQuade wrote:
> > On 2017/05/30 at 17:27:11, ryansturm wrote:
> > > still lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
> > > File chrome/browser/page_load_metrics/page_load_tracker.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2909033002/diff/20001/chrome/browser/page_loa...
> > > chrome/browser/page_load_metrics/page_load_tracker.cc:375:
> > ukm_recorder->UpdateSourceURL(source_id_, start_url_);
> > > On 2017/05/30 17:17:06, Bryan McQuade wrote:
> > > > On 2017/05/30 at 16:26:23, RyanSturm wrote:
> > > > > nit: Maybe this should record the whole redirect chain (I'm not sure
if
> it
> > > > would provide value later to see how many redirects we hit). Consider
the
> > case
> > > > of really slow FCP due to N redirects.
> > > > > 
> > > > > GetRedirectChain() is exposed off of NavigationHandle.
> > > > 
> > > > Interestingly IIUC GetRedirectChain returns the list of client-side or
> > > > content-initiated redirections, e.g. <meta refresh> or JS-based location
> > > > changes. This is super subtle and confusing but if I'm remembering
> correctly
> > > > that's what it's intended for.
> > > > 
> > > > On the UKM side we decided that logging the first and last URLs struct a
> > nice
> > > > balance between amount of data and usefulness. The UKM data format has
> 'url'
> > and
> > > > 'initial_url' fields only. I think we decided to avoid having an
unbounded
> > list
> > > > of URLs here.
> > > > 
> > > > What we could do is add a metric with the redirection count, to at least
> > > > understand the full number of redirects encountered.
> > > 
> > > I see this pseudo stack is possible from URLRequest:
> > > URLRequest::NotifyReceivedRedirect
> > > ResourceLoader::OnReceivedRedirect
> > > NavigationResourceHandler::OnRequestRedirected
> > > NavigationURLLoaderImplCore::NotifyRequestRedirected
> > > 
> > > Post to:
> > > 
> > > NavigationURLLoaderImpl::NotifyRequestRedirected
> > > NavigationRequest::OnRequestRedirected
> > > NavigationHandleImpl::WillRedirectRequest
> > > redirect_chain_.push_back(new_url);
> > > 
> > > If client redirects happen later than OnCommit, this should be good.
> > > 
> > > 
> > > Agreed that count is probably sufficient (but needs another CL and review
> from
> > UKM).
> > 
> > Thanks for digging into this! That's interesting - IIRC clamy had told me
this
> > was used to track content initiated rather than normal HTTP redirects.
> 
> Pretty odd because it doesn't seem that way (AFAICT).
> 
> I printed during DidFinishNavigation and under plznavigate and non-plznavigate
I
> see:
> 
> [43747:43747:0530/103518.126366:WARNING:previews_infobar_tab_helper.cc(72)]
> http://reddit.com/r/pics
> [43747:43747:0530/103518.126495:WARNING:previews_infobar_tab_helper.cc(72)]
> https://reddit.com/r/pics
> [43747:43747:0530/103518.126538:WARNING:previews_infobar_tab_helper.cc(72)]
> https://www.reddit.com/r/pics
> [43747:43747:0530/103518.126578:WARNING:previews_infobar_tab_helper.cc(72)]
> https://www.reddit.com/r/pics/
> 
> which are pretty typical server redirects, so I guess you should file a bug
and
> follow up with clamy@ about what she meant.

To clarify (separate action items):
File a bug to add the count in UKM
and
Follow up with clamy@ about the content/ API

[commit-bot: I haz the power, 2017-05-30 18:19:00]

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

[commit-bot: I haz the power, 2017-05-30 18:19:01]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-30 18:26:43]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2017-05-30 18:27:26]

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-05-30 18:31:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496168803906470,
"parent_rev": "70d88f21d9d5fbdd31646dbf14feed8bcae57c46", "commit_rev":
"0bff3c633799c676eadce377337cc24a27795151"}

[commit-bot: I haz the power, 2017-05-30 18:31:56]

Description was changed from

==========
Move UKM PageLoad SourceId generation to PageLoadTracker.

We'd like to enable all observers to record UKM data sharing the same SourceId,
so metrics from different observers can be queried/joined together.

This change migrates SourceId generation from the UKM observer to the
PageLoadTracker, so all observers can share the same SourceId.

BUG=727286
==========

to

==========
Move UKM PageLoad SourceId generation to PageLoadTracker.

We'd like to enable all observers to record UKM data sharing the same SourceId,
so metrics from different observers can be queried/joined together.

This change migrates SourceId generation from the UKM observer to the
PageLoadTracker, so all observers can share the same SourceId.

BUG=727286

Review-Url: https://codereview.chromium.org/2909033002
Cr-Commit-Position: refs/heads/master@{#475606}
Committed:
https://chromium.googlesource.com/chromium/src/+/0bff3c633799c676eadce377337c...
==========

[commit-bot: I haz the power, 2017-05-30 18:31:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0bff3c633799c676eadce377337c...