Domain Reliability: Create beacons from ConnectionAttempts.

Domain Reliability: Create beacons from ConnectionAttempts.

This also starts logging CANCELED requests, since they are just as
likely as FAILED/ERR_ABORTED ones to be from what *would've* been
ERR_TIMED_OUT if the user had been more patient. :)

BUG=480565
Committed: https://crrev.com/8feb018c6a97b6d075e7483464cc8fab9994a186
Cr-Commit-Position: refs/heads/master@{#330233}

[Deprecated (see juliatuttle), 2015-04-24 02:34:47]

ttuttle@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Deprecated (see juliatuttle), 2015-04-24 02:34:47]

PTAL when you get a chance; note dependency in CL description.

[Deprecated (see juliatuttle), 2015-04-27 22:26:47]

Patchset #1 (id:1) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:26:52]

Patchset #1 (id:20001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:26:57]

Patchset #1 (id:40001) has been deleted

[Randy Smith (Not in Mondays), 2015-05-01 17:41:19]

High level cop-out question: Is there someone who already understands the net
domain reliability code?  I suspect such a person would polish this off much
more easily than I will.

[Deprecated (see juliatuttle), 2015-05-04 21:06:09]

On 2015/05/01 17:41:19, rdsmith wrote:
> High level cop-out question: Is there someone who already understands the net
> domain reliability code?  I suspect such a person would polish this off much
> more easily than I will.

davidben does, but he's currently chasing a P0 bug.

[Randy Smith (Not in Mondays), 2015-05-05 17:32:59]

On 2015/05/04 21:06:09, ttuttle wrote:
> On 2015/05/01 17:41:19, rdsmith wrote:
> > High level cop-out question: Is there someone who already understands the
net
> > domain reliability code?  I suspect such a person would polish this off much
> > more easily than I will.
> 
> davidben does, but he's currently chasing a P0 bug.

Ok.  I'll put it at the end of my priority list, and if I get to it before he
does, so be it.

[Deprecated (see juliatuttle), 2015-05-11 18:28:05]

ttuttle@chromium.org changed reviewers:
+ davidben@chromium.org

[Deprecated (see juliatuttle), 2015-05-11 18:28:05]

PTAL, davidben.

[davidben, 2015-05-15 18:36:50]

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:27: int
URLRequestStatusToNetError(const net::URLRequestStatus& status) {
[Not your fault, but we really need to get rid of this nonsense sometime...]

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:42: 
This function is confusing since it intentionally leaves half of |beacon| as-is
while filling in other parts. This deserves documentation as to which is which.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:49: &beacon->status)) {
Mind doing a git cl format pass? I'm not sure that's the right indentation.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:65: if
(!net::ParseIPLiteralToNumber(host_port_pair.host(), &ip_address_number))
Are you sure this is correct? When an IPv6 address is written on its own, it's
hex and colons. When in the host of a URL, it has square brackets around it.
ParseIPLiteralToNumber seems to expect there *not* be square brackets. Which
format does net::HostPortPair supply?

https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util....

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:291: return true;
This is a change from the current behavior. Before, it looks like this was just
keying on network_accessed? Also it wouldn't record CANCELED. Why the change?
(And can that be documented in the commit message?)

Recording CANCELED seems weird. We might cancel a request internally for things.
Or maybe the user just closed a tab. That's not a network error. They just
happened to close a tab while some XHR was in progress.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:338: url_request_attempt_is_duplicate =
true;
When does this happen? This seems like the ConnectionAttempt API isn't terribly
well-defined, since this is a really loose check.

[Deprecated (see juliatuttle), 2015-05-15 19:08:45]

PTAL.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:27: int
URLRequestStatusToNetError(const net::URLRequestStatus& status) {
On 2015/05/15 18:36:50, David Benjamin wrote:
> [Not your fault, but we really need to get rid of this nonsense sometime...]

Yup.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:42: 
On 2015/05/15 18:36:50, David Benjamin wrote:
> This function is confusing since it intentionally leaves half of |beacon|
as-is
> while filling in other parts. This deserves documentation as to which is
which.

Done.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:49: &beacon->status)) {
On 2015/05/15 18:36:50, David Benjamin wrote:
> Mind doing a git cl format pass? I'm not sure that's the right indentation.

Done.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:65: if
(!net::ParseIPLiteralToNumber(host_port_pair.host(), &ip_address_number))
On 2015/05/15 18:36:50, David Benjamin wrote:
> Are you sure this is correct? When an IPv6 address is written on its own, it's
> hex and colons. When in the host of a URL, it has square brackets around it.
> ParseIPLiteralToNumber seems to expect there *not* be square brackets. Which
> format does net::HostPortPair supply?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util....

Yep, it's correct. HostPortPair eventually uses url::AppendIPv6Address, which
doesn't include brackets.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:291: return true;
On 2015/05/15 18:36:50, David Benjamin wrote:
> This is a change from the current behavior. Before, it looks like this was
just
> keying on network_accessed?

I moved a bunch of the logic into this function -- we were checking is_upload
and LOAD_DO_NOT_SEND_COOKIES before as well.

> Also it wouldn't record CANCELED. Why the change?
> (And can that be documented in the commit message?)

IIRC, I talked to Matt and he thought we should be treating CANCELED the same as
ERR_ABORTED.

> Recording CANCELED seems weird. We might cancel a request internally for
things.
> Or maybe the user just closed a tab. That's not a network error. They just
> happened to close a tab while some XHR was in progress.

Currently, we're reporting it, since users often close tabs before a hanging
connection actually turns into a timeout error.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:338: url_request_attempt_is_duplicate =
true;
On 2015/05/15 18:36:50, David Benjamin wrote:
> When does this happen? This seems like the ConnectionAttempt API isn't
terribly
> well-defined, since this is a really loose check.

It is well-defined, but it doesn't cover some errors that can occur *after*
connecting, like a content length mismatch. This check makes sure that we don't
report a failed connection attempt twice, but we do report other errors that
don't count as connection attempts.

[davidben, 2015-05-15 19:46:18]

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:65: if
(!net::ParseIPLiteralToNumber(host_port_pair.host(), &ip_address_number))
On 2015/05/15 19:08:45, ttuttle wrote:
> On 2015/05/15 18:36:50, David Benjamin wrote:
> > Are you sure this is correct? When an IPv6 address is written on its own,
it's
> > hex and colons. When in the host of a URL, it has square brackets around it.
> > ParseIPLiteralToNumber seems to expect there *not* be square brackets. Which
> > format does net::HostPortPair supply?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util....
> 
> Yep, it's correct. HostPortPair eventually uses url::AppendIPv6Address, which
> doesn't include brackets.

Acknowledged.

https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:291: return true;
On 2015/05/15 19:08:45, ttuttle wrote:
> On 2015/05/15 18:36:50, David Benjamin wrote:
> > This is a change from the current behavior. Before, it looks like this was
> just
> > keying on network_accessed?
> 
> I moved a bunch of the logic into this function -- we were checking is_upload
> and LOAD_DO_NOT_SEND_COOKIES before as well.

Right, and that one didn't change in behavior. They're still ANDs. Switching
from false to true means you have an OR now.

> > Also it wouldn't record CANCELED. Why the change?
> > (And can that be documented in the commit message?)
> 
> IIRC, I talked to Matt and he thought we should be treating CANCELED the same
as
> ERR_ABORTED.

Yeah, they should be the same. But ERR_ABORTED means we cancelled the request,
not that the network failed on us.
 
> > Recording CANCELED seems weird. We might cancel a request internally for
> things.
> > Or maybe the user just closed a tab. That's not a network error. They just
> > happened to close a tab while some XHR was in progress.
> 
> Currently, we're reporting it, since users often close tabs before a hanging
> connection actually turns into a timeout error.

(Would that not still have network_accessed true? Not sure, actually. Maybe no
since the transaction doesn't complete Start.)

I don't believe we're currently reporting it. The old code required
AccessedNetwork to be true, which doesn't allow for CANCELED.

I don't hugely mind the change in behavior, though I think it's puzzling. Just
it should be documented as a change in behavior in the commit message so it's
clear that's what you intended. (I'm not clear whether you did or no right now.)

[Deprecated (see juliatuttle), 2015-05-15 21:35:50]

On 2015/05/15 19:46:18, David Benjamin wrote:
>
https://codereview.chromium.org/1095553004/diff/160001/components/domain_reli...
> components/domain_reliability/monitor.cc:291: return true;
> On 2015/05/15 19:08:45, ttuttle wrote:
> > On 2015/05/15 18:36:50, David Benjamin wrote:
> > > This is a change from the current behavior. Before, it looks like this was
> > just
> > > keying on network_accessed?
> > 
> > I moved a bunch of the logic into this function -- we were checking
is_upload
> > and LOAD_DO_NOT_SEND_COOKIES before as well.
> 
> Right, and that one didn't change in behavior. They're still ANDs. Switching
> from false to true means you have an OR now.

Oh, yes. Now documented. (We do want to collect errors in the whitelist even if
network_accessed is false, as we're specifically listing network errors.)

> > > Also it wouldn't record CANCELED. Why the change?
> > > (And can that be documented in the commit message?)
> > 
> > IIRC, I talked to Matt and he thought we should be treating CANCELED the
same
> as
> > ERR_ABORTED.
> 
> Yeah, they should be the same. But ERR_ABORTED means we cancelled the request,
> not that the network failed on us.
>  
> > > Recording CANCELED seems weird. We might cancel a request internally for
> > things.
> > > Or maybe the user just closed a tab. That's not a network error. They just
> > > happened to close a tab while some XHR was in progress.
> > 
> > Currently, we're reporting it, since users often close tabs before a hanging
> > connection actually turns into a timeout error.
> 
> (Would that not still have network_accessed true? Not sure, actually. Maybe no
> since the transaction doesn't complete Start.)
> 
> I don't believe we're currently reporting it. The old code required
> AccessedNetwork to be true, which doesn't allow for CANCELED.
> 
> I don't hugely mind the change in behavior, though I think it's puzzling. Just
> it should be documented as a change in behavior in the commit message so it's
> clear that's what you intended. (I'm not clear whether you did or no right
now.)

Alright, documented in the commit message.

[davidben, 2015-05-15 21:36:33]

lgtm

[Deprecated (see juliatuttle), 2015-05-15 21:39:32]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2015-05-15 21:42:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1095553004/240001

[commit-bot: I haz the power, 2015-05-15 23:40:46]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2015-05-18 11:29:11]

Patchset 10 (id:??) landed as
https://crrev.com/8feb018c6a97b6d075e7483464cc8fab9994a186
Cr-Commit-Position: refs/heads/master@{#330233}