Navigation start to commit trace

Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with carlosk@, this change also
reverts the earlier traces which only covered PlzNavigate and adds the new
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

This patch adds the async begin and end traces in the constructor and
destructor of NavigationHandleImpl respectively. That would cover all of
the scenarios as detailed in the design doc
(https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHCmq9CpqQzDwK6MROaw/edit?usp=sharing).
The start time of Navigation is passed in the
constructor and that would be used as the trace start timestamp. For
failed/aborted provisional loads, the argument net error code will give the error
code.

In this patch I am adding them in the existing "navigation" category only and
later they might be extended for new categories like "loading".

Committed: https://crrev.com/9ab4458be13a0d542768a033fb81286b47615261
Cr-Commit-Position: refs/heads/master@{#389881}

[shivanisha, 2016-03-22 16:59:06]

Description was changed from

==========
Navigation start to commit trace

BUG=part of 585134
==========

to

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[shivanisha, 2016-03-22 18:43:57]

Description was changed from

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

A context of the code change here:

This is a part of the design for 585134 which includes a larger set of traces
and also a new tracing category called loading. As part of that design we also
noticed overlapping traces that existed earlier for navigation start to commit.
As discussed with Carlos, this change reverts the earlier traces as they were
only covering PlzNavigate and adds the new traces that cover the following
scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

The design is to add the async begin and end traces in the constructor and
destructor of NavigationHandleImpl respectively. That would cover all of the
scenarios as detailed in the design doc (
https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the constructor and that would be used
as the trace start timestamp. For failed/aborted provisional loads, the argument
net error code will give the error code.
==========

[shivanisha, 2016-03-22 18:44:13]

Description was changed from

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

A context of the code change here:

This is a part of the design for 585134 which includes a larger set of traces
and also a new tracing category called loading. As part of that design we also
noticed overlapping traces that existed earlier for navigation start to commit.
As discussed with Carlos, this change reverts the earlier traces as they were
only covering PlzNavigate and adds the new traces that cover the following
scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

The design is to add the async begin and end traces in the constructor and
destructor of NavigationHandleImpl respectively. That would cover all of the
scenarios as detailed in the design doc (
https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the constructor and that would be used
as the trace start timestamp. For failed/aborted provisional loads, the argument
net error code will give the error code.
==========

to

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces
and also a new tracing category called loading. As part of that design we also
noticed overlapping traces that existed earlier for navigation start to commit.
As discussed with Carlos, this change reverts the earlier traces as they were
only covering PlzNavigate and adds the new traces that cover the following
scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

The design is to add the async begin and end traces in the constructor and
destructor of NavigationHandleImpl respectively. That would cover all of the
scenarios as detailed in the design doc (
https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the constructor and that would be used
as the trace start timestamp. For failed/aborted provisional loads, the argument
net error code will give the error code.
==========

[shivanisha, 2016-03-22 18:47:50]

Description was changed from

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces
and also a new tracing category called loading. As part of that design we also
noticed overlapping traces that existed earlier for navigation start to commit.
As discussed with Carlos, this change reverts the earlier traces as they were
only covering PlzNavigate and adds the new traces that cover the following
scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

The design is to add the async begin and end traces in the constructor and
destructor of NavigationHandleImpl respectively. That would cover all of the
scenarios as detailed in the design doc (
https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the constructor and that would be used
as the trace start timestamp. For failed/aborted provisional loads, the argument
net error code will give the error code.
==========

to

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with Carlos, this change reverts
the earlier traces as they were only covering PlzNavigate and adds the new
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

The design is to add the async begin and end traces in the constructor and
destructor of NavigationHandleImpl respectively. That would cover all of the
scenarios as detailed in the design doc (
https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the constructor and that would be used
as the trace start timestamp. For failed/aborted provisional loads, the argument
net error code will give the error code.

In this patch I am adding them in the existing "navigation" category only and
later they might be extended for new categories like "loading"
==========

[shivanisha, 2016-03-22 18:51:37]

Description was changed from

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with Carlos, this change reverts
the earlier traces as they were only covering PlzNavigate and adds the new
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

The design is to add the async begin and end traces in the constructor and
destructor of NavigationHandleImpl respectively. That would cover all of the
scenarios as detailed in the design doc (
https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the constructor and that would be used
as the trace start timestamp. For failed/aborted provisional loads, the argument
net error code will give the error code.

In this patch I am adding them in the existing "navigation" category only and
later they might be extended for new categories like "loading"
==========

to

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with carlosk@, this change also
reverts the earlier traces which only covered PlzNavigate and adds the new
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

This patch adds the async begin and end traces in the constructor and destructor
of NavigationHandleImpl respectively. That would cover all of the scenarios as
detailed in the design doc (
https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the constructor and that would be used
as the trace start timestamp. For failed/aborted provisional loads, the argument
net error code will give the error code.

In this patch I am adding them in the existing "navigation" category only and
later they might be extended for new categories like "loading"
==========

[shivanisha, 2016-03-22 18:52:54]

shivanisha@chromium.org changed reviewers:
+ bmcquade@chromium.org, carlosk@chromium.org, clamy@chromium.org

[clamy, 2016-03-23 12:12:16]

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
Are you trying to index the navigation using the url? The url may change between
the start and the commit of a navigation if it encounters a server redirect. I
suggest you use the FrameTreeNode id instead, which is guaranteed to be constant
over the whole navigation.

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:92: "URL", url_.spec(),
"Net Error Code",
You may also want to add a boolean to check whether the navigation resulted in a
commit (page or error page), or whether it was aborted before commit.

[Bryan McQuade, 2016-03-23 12:17:33]

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
On 2016/03/23 at 12:12:16, clamy wrote:
> Are you trying to index the navigation using the url? The url may change
between the start and the commit of a navigation if it encounters a server
redirect. I suggest you use the FrameTreeNode id instead, which is guaranteed to
be constant over the whole navigation.

That's a good point. We shouldn't assume the URL is fixed for the duration of
the navigation. I do think it's also valuable to record the URL that the
navigation was initiated with, and then perhaps separately in a different trace
record the redirected URL and/or the committed URL. Perhaps we should use the
argument name provisional_url rather than url here to reflect that this is the
initial URL during the navigation and may change?

[shivanisha, 2016-03-23 13:56:09]

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
On 2016/03/23 at 12:12:16, clamy wrote:
> Are you trying to index the navigation using the url? The url may change
between the start and the commit of a navigation if it encounters a server
redirect. I suggest you use the FrameTreeNode id instead, which is guaranteed to
be constant over the whole navigation.

the navigation handle pointer is the index here and not the url . The URL is
just an argument. Since the Navigation handle pointer remains the same
throughout the navigation, this should be fine. Having said that, if
FrameTreeNode id is also visible to network level traces (that might be added in
a future enhancement) then may be I can index these traces with FrameTreeNode id
so that it is easier to link with those future traces. I will look into this.

Agree with Bryan's suggestion of changing the URL argument to provisional_url

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:92: "URL", url_.spec(),
"Net Error Code",
On 2016/03/23 at 12:12:16, clamy wrote:
> You may also want to add a boolean to check whether the navigation resulted in
a commit (page or error page), or whether it was aborted before commit.

Sure, that sounds good

[carlosk, 2016-03-23 14:40:34]

Thanks.

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
On 2016/03/23 13:56:09, shivanisha wrote:
> On 2016/03/23 at 12:12:16, clamy wrote:
> > Are you trying to index the navigation using the url? The url may change
> between the start and the commit of a navigation if it encounters a server
> redirect. I suggest you use the FrameTreeNode id instead, which is guaranteed
to
> be constant over the whole navigation.
> 
> the navigation handle pointer is the index here and not the url . The URL is
> just an argument. Since the Navigation handle pointer remains the same
> throughout the navigation, this should be fine. Having said that, if
> FrameTreeNode id is also visible to network level traces (that might be added
in
> a future enhancement) then may be I can index these traces with FrameTreeNode
id
> so that it is easier to link with those future traces. I will look into this.
>
> Agree with Bryan's suggestion of changing the URL argument to provisional_url

Mind that there can be 2 simultaneously ongoing navigations happening on the
same frame (one must be same site and the other cross-site) and so you can have
mixed up traces. Granted this might be an considered a non-supported exception
case but it can happen.

I tried to avoid it in my implementation but it does indeed make things simpler
to match async traces inserted in different parts of the code, when one does not
have a pointer to the NavigationHandle.

FYI in regards to redirects and changes to the URL I'd like to point you to the
doc I wrote when I was creating those traces for our benchmark: 
https://docs.google.com/document/d/1dyJejYZouBwyL6lCd5IJ3nU74lQKtCPra0M5tXrbI...
It describes what each trace would record and one of the things I tried to make
sure was that in the end all URL changes were tracked.

[shivanisha, 2016-03-23 18:07:08]

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
On 2016/03/23 at 14:40:34, carlosk wrote:
> Mind that there can be 2 simultaneously ongoing navigations happening on the
same frame (one must be same site and the other cross-site) and so you can have
mixed up traces. Granted this might be an considered a non-supported exception
case but it can happen.
> 
> I tried to avoid it in my implementation but it does indeed make things
simpler to match async traces inserted in different parts of the code, when one
does not have a pointer to the NavigationHandle.
> 
> FYI in regards to redirects and changes to the URL I'd like to point you to
the doc I wrote when I was creating those traces for our benchmark: 
>
https://docs.google.com/document/d/1dyJejYZouBwyL6lCd5IJ3nU74lQKtCPra0M5tXrbI...
> It describes what each trace would record and one of the things I tried to
make sure was that in the end all URL changes were tracked.

That's a good point that if we use frameTreeNode id then it can lead to mixed
traces between two navigations in the same frame. So , leaving it as it is be
indexed by Navigation Handle pointer.

I understand from the design doc that the redirect delays for PlzNavigate are
being traced currently. As you mention in the design doc at a later stage it
could be extended for both navigation flows.
(Mentioning here for completeness, my change will show Navigation Start to
Commit time which includes any redirect delays as well.)

[clamy, 2016-03-24 10:36:32]

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
On 2016/03/23 18:07:08, shivanisha wrote:
> On 2016/03/23 at 14:40:34, carlosk wrote:
> > Mind that there can be 2 simultaneously ongoing navigations happening on the
> same frame (one must be same site and the other cross-site) and so you can
have
> mixed up traces. Granted this might be an considered a non-supported exception
> case but it can happen.
> > 
> > I tried to avoid it in my implementation but it does indeed make things
> simpler to match async traces inserted in different parts of the code, when
one
> does not have a pointer to the NavigationHandle.
> > 
> > FYI in regards to redirects and changes to the URL I'd like to point you to
> the doc I wrote when I was creating those traces for our benchmark: 
> >
>
https://docs.google.com/document/d/1dyJejYZouBwyL6lCd5IJ3nU74lQKtCPra0M5tXrbI...
> > It describes what each trace would record and one of the things I tried to
> make sure was that in the end all URL changes were tracked.
> 
> That's a good point that if we use frameTreeNode id then it can lead to mixed
> traces between two navigations in the same frame. So , leaving it as it is be
> indexed by Navigation Handle pointer.
> 
> I understand from the design doc that the redirect delays for PlzNavigate are
> being traced currently. As you mention in the design doc at a later stage it
> could be extended for both navigation flows.
> (Mentioning here for completeness, my change will show Navigation Start to
> Commit time which includes any redirect delays as well.)

Indexing on the NavigationHandle is good (I was worried you were doing it on the
url, which would have been a problem).

You can track the time to redirects in both navigation paths by putting traces
in NavigationHandleImpl::WillRedirectRequest.

[shivanisha, 2016-03-29 19:49:32]

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
On 2016/03/23 at 13:56:09, shivanisha wrote:
> On 2016/03/23 at 12:12:16, clamy wrote:
> > Are you trying to index the navigation using the url? The url may change
between the start and the commit of a navigation if it encounters a server
redirect. I suggest you use the FrameTreeNode id instead, which is guaranteed to
be constant over the whole navigation.
> 
> the navigation handle pointer is the index here and not the url . The URL is
just an argument. Since the Navigation handle pointer remains the same
throughout the navigation, this should be fine. Having said that, if
FrameTreeNode id is also visible to network level traces (that might be added in
a future enhancement) then may be I can index these traces with FrameTreeNode id
so that it is easier to link with those future traces. I will look into this.
> 
> Agree with Bryan's suggestion of changing the URL argument to provisional_url

Actually since we are logging the URL in the end trace as well, it will have the
updated value of the URL in case there was a redirect so leaving the name as URL

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:92: "URL", url_.spec(),
"Net Error Code",
On 2016/03/23 at 13:56:09, shivanisha wrote:
> On 2016/03/23 at 12:12:16, clamy wrote:
> > You may also want to add a boolean to check whether the navigation resulted
in a commit (page or error page), or whether it was aborted before commit.
> 
> Sure, that sounds good

Actually, it seems we can assume that net error code of 0 will imply a committed
page. May be then we do not need the boolean?

[carlosk, 2016-03-31 10:10:29]

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
On 2016/03/29 19:49:32, shivanisha wrote:
> On 2016/03/23 at 13:56:09, shivanisha wrote:
> > On 2016/03/23 at 12:12:16, clamy wrote:
> > > Are you trying to index the navigation using the url? The url may change
> between the start and the commit of a navigation if it encounters a server
> redirect. I suggest you use the FrameTreeNode id instead, which is guaranteed
to
> be constant over the whole navigation.
> > 
> > the navigation handle pointer is the index here and not the url . The URL is
> just an argument. Since the Navigation handle pointer remains the same
> throughout the navigation, this should be fine. Having said that, if
> FrameTreeNode id is also visible to network level traces (that might be added
in
> a future enhancement) then may be I can index these traces with FrameTreeNode
id
> so that it is easier to link with those future traces. I will look into this.
> > 
> > Agree with Bryan's suggestion of changing the URL argument to
provisional_url
> 
> Actually since we are logging the URL in the end trace as well, it will have
the
> updated value of the URL in case there was a redirect so leaving the name as
URL

IIUC there's no much use in doing that. The END url recording will (I think)
override the former and as the trace itself will not exist without the END step,
the URL recording in BEGIN would be always overwritten.

My suggestion is to record both but with different labels.

[shivanisha, 2016-03-31 13:18:12]

On 2016/03/31 at 10:10:29, carlosk wrote:
>
https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/navigation_handle_impl.cc (right):
> 
>
https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
> On 2016/03/29 19:49:32, shivanisha wrote:
> > On 2016/03/23 at 13:56:09, shivanisha wrote:
> > > On 2016/03/23 at 12:12:16, clamy wrote:
> > > > Are you trying to index the navigation using the url? The url may change
> > between the start and the commit of a navigation if it encounters a server
> > redirect. I suggest you use the FrameTreeNode id instead, which is
guaranteed to
> > be constant over the whole navigation.
> > > 
> > > the navigation handle pointer is the index here and not the url . The URL
is
> > just an argument. Since the Navigation handle pointer remains the same
> > throughout the navigation, this should be fine. Having said that, if
> > FrameTreeNode id is also visible to network level traces (that might be
added in
> > a future enhancement) then may be I can index these traces with
FrameTreeNode id
> > so that it is easier to link with those future traces. I will look into
this.
> > > 
> > > Agree with Bryan's suggestion of changing the URL argument to
provisional_url
> > 
> > Actually since we are logging the URL in the end trace as well, it will have
the
> > updated value of the URL in case there was a redirect so leaving the name as
URL
> 
> IIUC there's no much use in doing that. The END url recording will (I think)
override the former and as the trace itself will not exist without the END step,
the URL recording in BEGIN would be always overwritten.
> 
> My suggestion is to record both but with different labels.

I have the understanding that the json file will record both start url and end
url even if they have the same label but the UI will only show the end URL. But
having two different labels sounds good to me as it will give more clarity. Will
update.

[shivanisha, 2016-04-12 14:10:03]

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1825533002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:76:
navigation_start.ToInternalValue(), "URL", url_.spec());
On 2016/03/31 at 10:10:29, carlosk wrote:
> On 2016/03/29 19:49:32, shivanisha wrote:
> > On 2016/03/23 at 13:56:09, shivanisha wrote:
> > > On 2016/03/23 at 12:12:16, clamy wrote:
> > > > Are you trying to index the navigation using the url? The url may change
> > between the start and the commit of a navigation if it encounters a server
> > redirect. I suggest you use the FrameTreeNode id instead, which is
guaranteed to
> > be constant over the whole navigation.
> > > 
> > > the navigation handle pointer is the index here and not the url . The URL
is
> > just an argument. Since the Navigation handle pointer remains the same
> > throughout the navigation, this should be fine. Having said that, if
> > FrameTreeNode id is also visible to network level traces (that might be
added in
> > a future enhancement) then may be I can index these traces with
FrameTreeNode id
> > so that it is easier to link with those future traces. I will look into
this.
> > > 
> > > Agree with Bryan's suggestion of changing the URL argument to
provisional_url
> > 
> > Actually since we are logging the URL in the end trace as well, it will have
the
> > updated value of the URL in case there was a redirect so leaving the name as
URL
> 
> IIUC there's no much use in doing that. The END url recording will (I think)
override the former and as the trace itself will not exist without the END step,
the URL recording in BEGIN would be always overwritten.
> 
> My suggestion is to record both but with different labels.

Applying 2 different labels is making things clear. Here is an example of a
redirected navigation. Note I have named the beginning url as "Initial URL" and
committed url as "URL":

Title	Navigation StartToCommit
Category	navigation
Start	14,126.555 ms
Wall Duration	549.542 ms
▶Args	
Initial URL	"http://mit.edu/"
URL	"http://web.mit.edu/"
Net Error Code	0

[shivanisha, 2016-04-19 23:44:06]

Description was changed from

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with carlosk@, this change also
reverts the earlier traces which only covered PlzNavigate and adds the new
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

This patch adds the async begin and end traces in the constructor and destructor
of NavigationHandleImpl respectively. That would cover all of the scenarios as
detailed in the design doc (
https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the constructor and that would be used
as the trace start timestamp. For failed/aborted provisional loads, the argument
net error code will give the error code.

In this patch I am adding them in the existing "navigation" category only and
later they might be extended for new categories like "loading"
==========

to

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with carlosk@, this change also
reverts the earlier traces which only covered PlzNavigate and adds the new 
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

This patch adds the async begin and end traces in the constructor and 
destructor of NavigationHandleImpl respectively. That would cover all of 
the scenarios as detailed in the design doc 
(https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the
constructor and that would be used as the trace start timestamp. For 
failed/aborted provisional loads, the argument net error code will give the
error
code.

In this patch I am adding them in the existing "navigation" category only and 
later they might be extended for new categories like "loading".
==========

[shivanisha, 2016-04-19 23:44:22]

Description was changed from

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with carlosk@, this change also
reverts the earlier traces which only covered PlzNavigate and adds the new 
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

This patch adds the async begin and end traces in the constructor and 
destructor of NavigationHandleImpl respectively. That would cover all of 
the scenarios as detailed in the design doc 
(https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the
constructor and that would be used as the trace start timestamp. For 
failed/aborted provisional loads, the argument net error code will give the
error
code.

In this patch I am adding them in the existing "navigation" category only and 
later they might be extended for new categories like "loading".
==========

to

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with carlosk@, this change also
reverts the earlier traces which only covered PlzNavigate and adds the new 
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

This patch adds the async begin and end traces in the constructor and 
destructor of NavigationHandleImpl respectively. That would cover all of 
the scenarios as detailed in the design doc 
(https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).

The start time of Navigation is passed in the
constructor and that would be used as the trace start timestamp. For 
failed/aborted provisional loads, the argument net error code will give the
error
code.

In this patch I am adding them in the existing "navigation" category only and 
later they might be extended for new categories like "loading".
==========

[shivanisha, 2016-04-20 00:46:51]

Hi Camille and Carlos,

Did you get a chance to look at the latest patch? This patch has all of the
feedback addressed.

We also discussed redirect slices in the discussion on this CL. I have not
included the redirect
slices in this CL yet because redirect slices could be either in Navigation
layer or in net/URLRequest
layer. I intend to start a separate mail thread discussing the two approaches
with you and possibly 
also including people working on the tracing team to make sure we are not adding
redundant 
information in traces.

What do you think?

Thanks,
Shivani

[shivanisha, 2016-04-20 00:55:39]

(resending the message correcting the alignment issues that happened in the
previous message)

Hi Camille and Carlos,

Did you get a chance to look at the latest patch? This patch has all of the
feedback addressed.

We also discussed redirect slices in the discussion on this CL. I have not
included the redirect slices in this CL yet because redirect slices could be
either in Navigation layer or in net/URLRequest layer. I intend to start a
separate mail thread discussing the two approaches with you and possibly also
including people working on the tracing team to make sure we are not adding
redundant information in traces.

What do you think?

Thanks,
Shivani

[shivanisha, 2016-04-20 01:09:40]

Adding more context to my previous message, we would like to get feedback for
redirect traces from the tracing team since they are currently brainstorming on
tracing architecture (for network traces among other things) and getting their
feedback will help to know that what they are thinking is in line with what we
will be adding.

If we mutually think we need to investigate more for redirect traces then we can
scope this CL for navigation start to commit and address redirects in a separate
CL.

[carlosk, 2016-04-20 10:02:57]

You have my LGTM for this change and I agree redirects could/should be addressed
in another one. Camille is currently OOO but should be back by the end of this
week.

I'm not sure if in #12 you were asking a question or not about what you read in
my doc about redirect tracing. But just to make it clear: as we finally
interrupted the development of our benchmark I have no current plans to continue
implementing and expanding those traces (that's why I was considering reverting
them). You can do whatever you need for your goals and go on replacing them.

[shivanisha, 2016-04-26 13:56:17]

Camille, PTAL, Thanks!

[clamy, 2016-04-26 14:53:48]

Thanks! Lgtm.

[shivanisha, 2016-04-26 15:00:44]

On 2016/04/26 at 14:53:48, clamy wrote:
> Thanks! Lgtm.

Thanks everyone.

[shivanisha, 2016-04-26 15:01:30]

The CQ bit was checked by shivanisha@chromium.org

[commit-bot: I haz the power, 2016-04-26 15:01:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825533002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825533002/20001

[commit-bot: I haz the power, 2016-04-26 16:53:16]

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

[commit-bot: I haz the power, 2016-04-26 16:53:17]

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

[shivanisha, 2016-04-26 20:54:36]

The CQ bit was checked by shivanisha@chromium.org

[commit-bot: I haz the power, 2016-04-26 20:55:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825533002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825533002/20001

[commit-bot: I haz the power, 2016-04-26 21:05:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-26 21:08:01]

Description was changed from

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with carlosk@, this change also
reverts the earlier traces which only covered PlzNavigate and adds the new 
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

This patch adds the async begin and end traces in the constructor and 
destructor of NavigationHandleImpl respectively. That would cover all of 
the scenarios as detailed in the design doc 
(https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).

The start time of Navigation is passed in the
constructor and that would be used as the trace start timestamp. For 
failed/aborted provisional loads, the argument net error code will give the
error
code.

In this patch I am adding them in the existing "navigation" category only and 
later they might be extended for new categories like "loading".
==========

to

==========
Navigation start to commit trace

BUG=part of 585134
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

This is a part of the design for 585134 which includes a larger set of traces.
As part of that design we also noticed overlapping traces that existed earlier
for navigation start to commit. As discussed with carlosk@, this change also
reverts the earlier traces which only covered PlzNavigate and adds the new
traces that cover the following scenarios:
- plzNavigate and traditional flows
- committed and aborted/failed/canceled loads

This patch adds the async begin and end traces in the constructor and
destructor of NavigationHandleImpl respectively. That would cover all of
the scenarios as detailed in the design doc
(https://docs.google.com/a/chromium.org/document/d/1yM2khCxLC5AV3X4y7cyuCNCFHC...).
The start time of Navigation is passed in the
constructor and that would be used as the trace start timestamp. For
failed/aborted provisional loads, the argument net error code will give the
error
code.

In this patch I am adding them in the existing "navigation" category only and
later they might be extended for new categories like "loading".

Committed: https://crrev.com/9ab4458be13a0d542768a033fb81286b47615261
Cr-Commit-Position: refs/heads/master@{#389881}
==========

[commit-bot: I haz the power, 2016-04-26 21:08:03]

Patchset 2 (id:??) landed as
https://crrev.com/9ab4458be13a0d542768a033fb81286b47615261
Cr-Commit-Position: refs/heads/master@{#389881}