Adds UMA histogram metrics for SafeBrowsing PVer4 methods.

Adds UMA histogram metrics for SafeBrowsing PVer4 methods.

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

[kcarattini, 2016-01-06 05:59:48]

kcarattini@chromium.org changed reviewers:
+ awoz@google.com, nparker@chromium.org, vakh@google.com

[Nathan Parker, 2016-01-07 05:28:29]

https://codereview.chromium.org/1563763002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1563763002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:96:
"SafeBrowsing.V4HttpResponseOrErrorCode";
Should this have "Hash" in it somewhere? (since there will he other V4Http
Responses)

https://codereview.chromium.org/1563763002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:496: // Update or chunk
response.
I always thought it was odd that this OnURLFetchComplete() handles both chunk
responses and hash responses, since they are quite different... Do you think
there is an advantage to having the v4 hash handling in here as well, rather
than in a separate class?  If there's no overlap, it could make both simpler by
creating a V4ProtocolManager or some such.  Just a thought.  You could land this
stuff and consider pull it out later.  Or we could leave it here till all the V3
code goes away (quarters).

https://codereview.chromium.org/1563763002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1563763002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:40213: +    prefix collision).
(PARSE_ERROR, NETWORK_ERROR, HTTP_ERROR, and
Could remove the mention of past changes, since there is not v4 data back then. 
Also mention that this is similar to the GetHashResult, but for v4.

https://codereview.chromium.org/1563763002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:40283: +<histogram
name="SafeBrowsing.V4HttpResponseOrErrorCode"
Again I think this should be "Hash" specific.

[kcarattini, 2016-01-07 06:06:13]

Thanks for the review! PTAL.

https://codereview.chromium.org/1563763002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1563763002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:96:
"SafeBrowsing.V4HttpResponseOrErrorCode";
On 2016/01/07 05:28:29, nparker wrote:
> Should this have "Hash" in it somewhere? (since there will he other V4Http
> Responses)

Done.

https://codereview.chromium.org/1563763002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:496: // Update or chunk
response.
On 2016/01/07 05:28:29, nparker wrote:
> I always thought it was odd that this OnURLFetchComplete() handles both chunk
> responses and hash responses, since they are quite different... Do you think
> there is an advantage to having the v4 hash handling in here as well, rather
> than in a separate class?  If there's no overlap, it could make both simpler
by
> creating a V4ProtocolManager or some such.  Just a thought.  You could land
this
> stuff and consider pull it out later.  Or we could leave it here till all the
V3
> code goes away (quarters).

Added a TODO to consider pulling this out in the future. There is a small amount
of method and type definition reuse that is useful in keeping these together
e.g. the ResultType definition).

https://codereview.chromium.org/1563763002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1563763002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:40213: +    prefix collision).
(PARSE_ERROR, NETWORK_ERROR, HTTP_ERROR, and
On 2016/01/07 05:28:29, nparker wrote:
> Could remove the mention of past changes, since there is not v4 data back
then. 
> Also mention that this is similar to the GetHashResult, but for v4.

Done.

https://codereview.chromium.org/1563763002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:40283: +<histogram
name="SafeBrowsing.V4HttpResponseOrErrorCode"
On 2016/01/07 05:28:29, nparker wrote:
> Again I think this should be "Hash" specific.

Done.

[vakh (old account. dont use), 2016-01-08 19:41:51]

lgtm

[awoz, 2016-01-11 23:11:45]

lgtm

[Nathan Parker, 2016-01-11 23:28:12]

lgtm

[kcarattini, 2016-01-13 04:25:45]

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

[kcarattini, 2016-01-13 04:25:45]

holte@ please review histograms.xml

Thanks,
Kendra

[Steven Holte, 2016-01-13 21:25:43]

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

https://codereview.chromium.org/1563763002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:507:
RecordGetV4HashResult(GET_HASH_PARSE_ERROR);
It's usually preferable to have non-overlapping enum values, so that you can say
if 30% of the counts in a histogram fall in bucket X then 30% of the operations
that recorded a result had that result.

In this path it looks like you are recording counts in both PARSE_ERROR and
STATUS_200, which will make interpreting those results more complicated.  It
would be cleaner to make these two cases (STATUS_200 w/o error and PARSE_ERROR)

Not sure if BACKOFF_ERROR/MIN_WAIT_DURATION_ERROR can also be recorded.

[kcarattini, 2016-01-13 22:53:21]

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

https://codereview.chromium.org/1563763002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:507:
RecordGetV4HashResult(GET_HASH_PARSE_ERROR);
On 2016/01/13 21:25:43, Steven Holte wrote:
> It's usually preferable to have non-overlapping enum values, so that you can
say
> if 30% of the counts in a histogram fall in bucket X then 30% of the
operations
> that recorded a result had that result.
> 
> In this path it looks like you are recording counts in both PARSE_ERROR and
> STATUS_200, which will make interpreting those results more complicated.  It
> would be cleaner to make these two cases (STATUS_200 w/o error and
PARSE_ERROR)
> 
> Not sure if BACKOFF_ERROR/MIN_WAIT_DURATION_ERROR can also be recorded.

Nathan, please correct me if I'm wrong, but I assume we are interested in the
raw counts of these histogram buckets rather than percentage, and we only look
for large changes in these metrics to indicate something is amiss. Otherwise, as
Steve mentioned, interpreting the results is more complicated. I could remove
the GET_HASH_PARSE_ERROR bucket entirely because I have added a new histogram
for that in a follow up cl anyway, but the other buckets (backoff,
min_wait_duration etc.) would still overlap. I've added the new histograms
buckets in the same way that the existing v3 GetHashResult metrics are recorded
so I am assuming this is the way we want it.

[Steven Holte, 2016-01-13 23:06:17]

On 2016/01/13 22:53:21, kcarattini wrote:
>
https://codereview.chromium.org/1563763002/diff/60001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/protocol_manager.cc (right):
> 
>
https://codereview.chromium.org/1563763002/diff/60001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/protocol_manager.cc:507:
> RecordGetV4HashResult(GET_HASH_PARSE_ERROR);
> On 2016/01/13 21:25:43, Steven Holte wrote:
> > It's usually preferable to have non-overlapping enum values, so that you can
> say
> > if 30% of the counts in a histogram fall in bucket X then 30% of the
> operations
> > that recorded a result had that result.
> > 
> > In this path it looks like you are recording counts in both PARSE_ERROR and
> > STATUS_200, which will make interpreting those results more complicated.  It
> > would be cleaner to make these two cases (STATUS_200 w/o error and
> PARSE_ERROR)
> > 
> > Not sure if BACKOFF_ERROR/MIN_WAIT_DURATION_ERROR can also be recorded.
> 
> Nathan, please correct me if I'm wrong, but I assume we are interested in the
> raw counts of these histogram buckets rather than percentage, and we only look
> for large changes in these metrics to indicate something is amiss. Otherwise,
as
> Steve mentioned, interpreting the results is more complicated. I could remove
> the GET_HASH_PARSE_ERROR bucket entirely because I have added a new histogram
> for that in a follow up cl anyway, but the other buckets (backoff,
> min_wait_duration etc.) would still overlap. I've added the new histograms
> buckets in the same way that the existing v3 GetHashResult metrics are
recorded
> so I am assuming this is the way we want it.

If you know you don't care about bucket percentages, then lgtm.

[Nathan Parker, 2016-01-14 02:04:25]

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

https://codereview.chromium.org/1563763002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:507:
RecordGetV4HashResult(GET_HASH_PARSE_ERROR);
On 2016/01/13 22:53:20, kcarattini wrote:
> On 2016/01/13 21:25:43, Steven Holte wrote:
> > It's usually preferable to have non-overlapping enum values, so that you can
> say
> > if 30% of the counts in a histogram fall in bucket X then 30% of the
> operations
> > that recorded a result had that result.
> > 
> > In this path it looks like you are recording counts in both PARSE_ERROR and
> > STATUS_200, which will make interpreting those results more complicated.  It
> > would be cleaner to make these two cases (STATUS_200 w/o error and
> PARSE_ERROR)
> > 
> > Not sure if BACKOFF_ERROR/MIN_WAIT_DURATION_ERROR can also be recorded.
> 
> Nathan, please correct me if I'm wrong, but I assume we are interested in the
> raw counts of these histogram buckets rather than percentage, and we only look
> for large changes in these metrics to indicate something is amiss. Otherwise,
as
> Steve mentioned, interpreting the results is more complicated. I could remove
> the GET_HASH_PARSE_ERROR bucket entirely because I have added a new histogram
> for that in a follow up cl anyway, but the other buckets (backoff,
> min_wait_duration etc.) would still overlap. I've added the new histograms
> buckets in the same way that the existing v3 GetHashResult metrics are
recorded
> so I am assuming this is the way we want it. 

I agree with Steven but if it's already overlapping, there's not real harm in
adding more overlapping.  As long as this is described in the .xml text, then
this is fine.

[kcarattini, 2016-01-14 03:07:24]

Thanks, I added a comment. We can always add histograms with mutually exclusive
buckets if we want to in the future.

[kcarattini, 2016-01-14 03:07:39]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2016-01-14 03:07:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, awoz@google.com, vakh@google.com, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/1563763002/#ps80001
(title: "Add comment")

[commit-bot: I haz the power, 2016-01-14 03:09:34]

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

[commit-bot: I haz the power, 2016-01-14 05:15:04]

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

[commit-bot: I haz the power, 2016-01-14 05:15:05]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)

[kcarattini, 2016-01-14 05:41:20]

The CQ bit was checked by kcarattini@chromium.org

[commit-bot: I haz the power, 2016-01-14 05:42:05]

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

[commit-bot: I haz the power, 2016-01-14 06:34:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-14 06:36:51]

Description was changed from

==========
Adds UMA histogram metrics for SafeBrowsing PVer4 methods.

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

to

==========
Adds UMA histogram metrics for SafeBrowsing PVer4 methods.

BUG=561867

Committed: https://crrev.com/b0746c34eaa9b36fe6b7a20c4fa7fa213a71697e
Cr-Commit-Position: refs/heads/master@{#369350}
==========

[commit-bot: I haz the power, 2016-01-14 06:36:52]

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