Domain Reliability: Histogram net error codes from uploads

Domain Reliability: Histogram net error codes from uploads

BUG=

Committed: https://crrev.com/128179a85d0225f6eff4f177daae5ce3ec797f3b
Cr-Commit-Position: refs/heads/master@{#302351}

[Deprecated (see juliatuttle), 2014-10-30 21:05:27]

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

[Deprecated (see juliatuttle), 2014-10-30 21:05:27]

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

[Deprecated (see juliatuttle), 2014-10-30 21:05:28]

PTAL, davidben.

[davidben, 2014-10-30 22:26:55]

lgtm with one comment

mmenke: FYI. There's another URLRequestStatus::error silliness below. We should
probably resolve that in our copious spare time.

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
File components/domain_reliability/uploader.cc (right):

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
components/domain_reliability/uploader.cc:103: net_error = net::OK;
+mmenke: FYI, here's another one of these. We really should go through and make
it so that you can query status.error() directly or something.

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
components/domain_reliability/uploader.cc:113: -net_error);
Should this be

UMA_HISTOGRAM_CUSTOM_ENUMERATION("DomainReliability.UploadNetError",
                                 -net_error,
                                 net::GetAllErrorCodesForUma());

We're not very consistent about this. Perhaps ask the histograms.xml reviewer
which is preferred?

[mmenke, 2014-10-30 22:42:59]

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

[mmenke, 2014-10-30 22:42:59]

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
File components/domain_reliability/uploader.cc (right):

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
components/domain_reliability/uploader.cc:103: net_error = net::OK;
On 2014/10/30 22:26:55, David Benjamin wrote:
> +mmenke: FYI, here's another one of these. We really should go through and
make
> it so that you can query status.error() directly or something.

I'd like to get rid of the double-value status entirely, and just make it a
NetError.

Sadly, to be sure this works properly, this code should probably check for:

status.status() == net::URLRequestStatus::FAILED && status.error() !=
net::ERR_ABORTED

[Deprecated (see juliatuttle), 2014-10-31 18:54:01]

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
File components/domain_reliability/uploader.cc (right):

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
components/domain_reliability/uploader.cc:103: net_error = net::OK;
On 2014/10/30 22:42:59, mmenke wrote:
> On 2014/10/30 22:26:55, David Benjamin wrote:
> > +mmenke: FYI, here's another one of these. We really should go through and
> make
> > it so that you can query status.error() directly or something.
> 
> I'd like to get rid of the double-value status entirely, and just make it a
> NetError.
> 
> Sadly, to be sure this works properly, this code should probably check for:
> 
> status.status() == net::URLRequestStatus::FAILED && status.error() !=
> net::ERR_ABORTED

Why does ERR_ABORTED not count?

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
components/domain_reliability/uploader.cc:113: -net_error);
On 2014/10/30 22:26:55, David Benjamin wrote:
> Should this be
> 
> UMA_HISTOGRAM_CUSTOM_ENUMERATION("DomainReliability.UploadNetError",
>                                  -net_error,
>                                  net::GetAllErrorCodesForUma());
> 
> We're not very consistent about this. Perhaps ask the histograms.xml reviewer
> which is preferred?

This is rare enough that I think a sparse one will save a lot of RAM.

[mmenke, 2014-10-31 18:55:55]

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
File components/domain_reliability/uploader.cc (right):

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
components/domain_reliability/uploader.cc:103: net_error = net::OK;
On 2014/10/31 18:54:01, ttuttle wrote:
> On 2014/10/30 22:42:59, mmenke wrote:
> > On 2014/10/30 22:26:55, David Benjamin wrote:
> > > +mmenke: FYI, here's another one of these. We really should go through and
> > make
> > > it so that you can query status.error() directly or something.
> > 
> > I'd like to get rid of the double-value status entirely, and just make it a
> > NetError.
> > 
> > Sadly, to be sure this works properly, this code should probably check for:
> > 
> > status.status() == net::URLRequestStatus::FAILED && status.error() !=
> > net::ERR_ABORTED
> 
> Why does ERR_ABORTED not count?

The same reason you're not counting net::URLRequestStatus:CANCELED.  IF you want
to count cancelled requests, should count those, too.  Don't ask why we have two
ways to indicate a request was cancelled.

[Deprecated (see juliatuttle), 2014-10-31 19:14:38]

On 2014/10/31 18:55:55, mmenke wrote:
>
https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
> File components/domain_reliability/uploader.cc (right):
> 
>
https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
> components/domain_reliability/uploader.cc:103: net_error = net::OK;
> On 2014/10/31 18:54:01, ttuttle wrote:
> > On 2014/10/30 22:42:59, mmenke wrote:
> > > On 2014/10/30 22:26:55, David Benjamin wrote:
> > > > +mmenke: FYI, here's another one of these. We really should go through
and
> > > make
> > > > it so that you can query status.error() directly or something.
> > > 
> > > I'd like to get rid of the double-value status entirely, and just make it
a
> > > NetError.
> > > 
> > > Sadly, to be sure this works properly, this code should probably check
for:
> > > 
> > > status.status() == net::URLRequestStatus::FAILED && status.error() !=
> > > net::ERR_ABORTED
> > 
> > Why does ERR_ABORTED not count?
> 
> The same reason you're not counting net::URLRequestStatus:CANCELED.  IF you
want
> to count cancelled requests, should count those, too.  Don't ask why we have
two
> ways to indicate a request was cancelled.

From my perspective, a cancelled request is not successful. I'm trying to figure
out whether the data was  collected, and in this case, it (possibly) wasn't.

[mmenke, 2014-10-31 19:17:44]

On 2014/10/31 19:14:38, ttuttle wrote:
> On 2014/10/31 18:55:55, mmenke wrote:
> >
>
https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
> > File components/domain_reliability/uploader.cc (right):
> > 
> >
>
https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
> > components/domain_reliability/uploader.cc:103: net_error = net::OK;
> > On 2014/10/31 18:54:01, ttuttle wrote:
> > > On 2014/10/30 22:42:59, mmenke wrote:
> > > > On 2014/10/30 22:26:55, David Benjamin wrote:
> > > > > +mmenke: FYI, here's another one of these. We really should go through
> and
> > > > make
> > > > > it so that you can query status.error() directly or something.
> > > > 
> > > > I'd like to get rid of the double-value status entirely, and just make
it
> a
> > > > NetError.
> > > > 
> > > > Sadly, to be sure this works properly, this code should probably check
> for:
> > > > 
> > > > status.status() == net::URLRequestStatus::FAILED && status.error() !=
> > > > net::ERR_ABORTED
> > > 
> > > Why does ERR_ABORTED not count?
> > 
> > The same reason you're not counting net::URLRequestStatus:CANCELED.  IF you
> want
> > to count cancelled requests, should count those, too.  Don't ask why we have
> two
> > ways to indicate a request was cancelled.
> 
> From my perspective, a cancelled request is not successful. I'm trying to
figure
> out whether the data was  collected, and in this case, it (possibly) wasn't.

I'm fine with counting cancelled requests, I had just assumed you were
deliberately not counting them - either way, should either both cancelled status
codes, or neither.

[Deprecated (see juliatuttle), 2014-10-31 21:25:39]

PTAL, mmenke.

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
File components/domain_reliability/uploader.cc (right):

https://codereview.chromium.org/694573003/diff/20001/components/domain_reliab...
components/domain_reliability/uploader.cc:103: net_error = net::OK;
On 2014/10/31 18:55:55, mmenke wrote:
> On 2014/10/31 18:54:01, ttuttle wrote:
> > On 2014/10/30 22:42:59, mmenke wrote:
> > > On 2014/10/30 22:26:55, David Benjamin wrote:
> > > > +mmenke: FYI, here's another one of these. We really should go through
and
> > > make
> > > > it so that you can query status.error() directly or something.
> > > 
> > > I'd like to get rid of the double-value status entirely, and just make it
a
> > > NetError.
> > > 
> > > Sadly, to be sure this works properly, this code should probably check
for:
> > > 
> > > status.status() == net::URLRequestStatus::FAILED && status.error() !=
> > > net::ERR_ABORTED
> > 
> > Why does ERR_ABORTED not count?
> 
> The same reason you're not counting net::URLRequestStatus:CANCELED.  IF you
want
> to count cancelled requests, should count those, too.  Don't ask why we have
two
> ways to indicate a request was cancelled.

Alright, I'm counting CANCELED as ERR_ABORTED.

[Deprecated (see juliatuttle), 2014-10-31 21:26:42]

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

[Deprecated (see juliatuttle), 2014-10-31 21:26:42]

ttuttle@chromium.org changed required reviewers:
+ asvitkine@chromium.org

[Deprecated (see juliatuttle), 2014-10-31 21:26:42]

asvitkine, PTAL at histograms.xml

[Alexei Svitkine (slow), 2014-10-31 21:35:38]

Lgtm with a suggestion

https://codereview.chromium.org/694573003/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/694573003/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5222: <histogram
name="DomainReliability.UploadResponseCode">
We have an enum entry for http codes, can you set it on the histogram?

[Deprecated (see juliatuttle), 2014-10-31 22:00:26]

Thanks!

https://codereview.chromium.org/694573003/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/694573003/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5222: <histogram
name="DomainReliability.UploadResponseCode">
On 2014/10/31 21:35:38, Alexei Svitkine wrote:
> We have an enum entry for http codes, can you set it on the histogram?

Done.

[Deprecated (see juliatuttle), 2014-10-31 22:02:05]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-10-31 22:03:23]

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

[commit-bot: I haz the power, 2014-10-31 23:25:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-31 23:26:26]

Patchset 4 (id:??) landed as
https://crrev.com/128179a85d0225f6eff4f177daae5ce3ec797f3b
Cr-Commit-Position: refs/heads/master@{#302351}