Add UMA metric for GetHash parsing errors.

Add UMA metric for GetHash parsing errors.

BUG=561867
Committed: https://crrev.com/4c3f131a4c569276b15581ac75e3fc49a7af1de8
Cr-Commit-Position: refs/heads/master@{#371142}

[kcarattini, 2016-01-12 04:41:08]

kcarattini@chromium.org changed reviewers:
+ nparker@chromium.org

[Nathan Parker, 2016-01-15 19:34:43]

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:378: return false;
Why does this this one return and the rest dont?  I'm noticing that some of
these can be reported multiple times per V4 HashReponse (if there are multiple
matches), and some at most once.  This makes it harder to calculate percentages
of partially-failed requests (full-failed requests will return false and the
caller reports a different metric).

We could report a PARTIAL_FAILURE error (once) when ANY of these are reported,
so we can expect that that will add up to the rest of them unless we're getting
multiple errors per response.

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.h (right):

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:182: // Enumerate V4 parsing
failures for histogramming purposes.  DO NOT CHANGE
This could all go in a n anonymous namespace in the .cc file, since noone
outside of that needs access to it.

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:186: PARSE_FROM_STRING_ERROR,
I like to add "= 1.." to each so that you can more easily verify they match the
.xml, and it protects a bit more from accidental reordering.

[kcarattini, 2016-01-20 04:02:05]

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:378: return false;
On 2016/01/15 19:34:42, nparker wrote:
> Why does this this one return and the rest dont?  I'm noticing that some of
> these can be reported multiple times per V4 HashReponse (if there are multiple
> matches), and some at most once.  This makes it harder to calculate
percentages
> of partially-failed requests (full-failed requests will return false and the
> caller reports a different metric).
> 
> We could report a PARTIAL_FAILURE error (once) when ANY of these are reported,
> so we can expect that that will add up to the rest of them unless we're
getting
> multiple errors per response.  

I made them all return false, that way there is only one report per Response and
the Response is meaningless if any of these errors occur. What do you think? It
gets more complicated if we want to have partial errors. I think in a follow up
cl I'll add enforcement of the one-threat-type-per-request decision we made
previously so that it is explicit.

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.h (right):

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:182: // Enumerate V4 parsing
failures for histogramming purposes.  DO NOT CHANGE
On 2016/01/15 19:34:43, nparker wrote:
> This could all go in a n anonymous namespace in the .cc file, since noone
> outside of that needs access to it.

Done.

https://codereview.chromium.org/1579083002/diff/30001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:186: PARSE_FROM_STRING_ERROR,
On 2016/01/15 19:34:42, nparker wrote:
> I like to add "= 1.." to each so that you can more easily verify they match
the
> .xml, and it protects a bit more from accidental reordering.

Done.

[Nathan Parker, 2016-01-20 23:09:58]

lgtm

https://codereview.chromium.org/1579083002/diff/50001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1579083002/diff/50001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40294: +    (PARSE_FROM_STRING_ERROR,
UNEXPECTED_THREAT_ENTRY_TYPE_ERROR,
You don't need to list the enum values here, esp since this may get out of sync
with the list.

[kcarattini, 2016-01-20 23:23:39]

kcarattini@chromium.org changed reviewers:
+ holte@chromium.org

[kcarattini, 2016-01-20 23:23:40]

Thanks, nathan!

holte@ please review histograms.xml

https://codereview.chromium.org/1579083002/diff/50001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1579083002/diff/50001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40294: +    (PARSE_FROM_STRING_ERROR,
UNEXPECTED_THREAT_ENTRY_TYPE_ERROR,
On 2016/01/20 23:09:58, nparker wrote:
> You don't need to list the enum values here, esp since this may get out of
sync
> with the list.

Done.

[Steven Holte, 2016-01-22 21:14:53]

lgtm

[kcarattini, 2016-01-23 21:09:14]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2016-01-23 21:09:15]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1579083002/#ps70001
(title: "Review Comments")

[commit-bot: I haz the power, 2016-01-23 21:10:01]

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

[commit-bot: I haz the power, 2016-01-23 23:18:34]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-01-23 23:19:48]

Description was changed from

==========
Add UMA metric for GetHash parsing errors.

BUG=561867
==========

to

==========
Add UMA metric for GetHash parsing errors.

BUG=561867

Committed: https://crrev.com/4c3f131a4c569276b15581ac75e3fc49a7af1de8
Cr-Commit-Position: refs/heads/master@{#371142}
==========

[commit-bot: I haz the power, 2016-01-23 23:19:49]

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