[Blob] Added error reporting metrics to invalid message errors.

[Blob] Added error reporting metrics to invalid message errors.

This helps us get to the bottom of where the invalid blob refcount
and url calls are. It's unclear whether these are new or if they've
always occurred, as we used to ignore these messages.

R=kinuko@chromium.org, mpearson@chromium.org
BUG=602443, 607698
Committed: https://crrev.com/a00379794f50019c7047c83e537803b7ba2b11ea
Cr-Commit-Position: refs/heads/master@{#388626}

[dmurph, 2016-04-18 21:30:49]

Description was changed from

==========
[Blob] Added error reporting metrics to invalid message errors.

R=michaeln, mpearson
BUG=602443
==========

to

==========
[Blob] Added error reporting metrics to invalid message errors.

This helps us get to the bottom of where the invalid blob refcount
and url calls are. It's unclear whether these are new or if they've
always occurred, as we used to ignore these messages.

R=kinuko@chromium.org, mpearson@chromium.org
BUG=602443
==========

[dmurph, 2016-04-18 21:30:50]

dmurph@chromium.org changed reviewers:
+ kinuko@chromium.org, mpearson@chromium.org
- michaeln@chromium.org, mpearson@google.com

[dmurph, 2016-04-18 21:32:38]

Hello Kinuko & Mark,

Can you PTAL at these metrics changes? I'm trying to figure out why these bad
messages are happening, and it looks like both of these cases are triggering. So
instead of adding more bad_message.h enum (which I can do, let me know if that
is better), I'm adding UMA metrics for each error case.

One reason for making these separate UMA statistics is if we decide to remove
these bad message crashes -- if we can't figure out why -- we'll still record
these stats.

Thanks!
Daniel

[kinuko, 2016-04-18 23:44:23]

code change lgtm

[Mark P, 2016-04-19 19:34:53]

The decision between using bad_messages and histograms is the following.  Pluses
for bad_messages:
- allows easy comparison between these types of failures and other types of
failures (i.e., not auto-aggregated into the summary invalid rpc messages)
- easy discovery of these additional subtypes of failure types
Pluses for histograms:
- easy to remove later without changing bad_message statistics
- keeps (more useful?) aggregations of error types in bad_messages

--mark

https://codereview.chromium.org/1893293006/diff/1/content/browser/blob_storag...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1893293006/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_dispatcher_host.cc:10: #include
"base/metrics/histogram.h"
Prefer histogram_macros.h

https://codereview.chromium.org/1893293006/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_dispatcher_host.cc:213:
UMA_HISTOGRAM_BOOLEAN("Storage.Blob.InvalidReferenceInc", true);
If you're only emitting true to this histogram, why don't you simply do
Storage.Blob.InvalidReference and have true = Increment and false = Decrement?

https://codereview.chromium.org/1893293006/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_dispatcher_host.cc:255:
UMA_HISTOGRAM_BOOLEAN("Storage.Blob.InvalidURLRegister", true);
analogous comment here

[dmurph, 2016-04-20 21:45:37]

Hi Mark,

I switched to using an enum to cut down on metric names. WDYT?

Thanks,
Daniel

[Mark P, 2016-04-20 21:55:03]

lgtm

https://codereview.chromium.org/1893293006/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1893293006/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_dispatcher_host.cc:28: namespace {
Add some blank lines in/around this new code.

[dmurph, 2016-04-20 22:04:51]

Thanks!

https://codereview.chromium.org/1893293006/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/1893293006/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_dispatcher_host.cc:28: namespace {
On 2016/04/20 at 21:55:02, Mark P wrote:
> Add some blank lines in/around this new code.

Done.

[dmurph, 2016-04-20 22:04:58]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-04-20 22:04:59]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1893293006/#ps40001
(title: "added spaces")

[commit-bot: I haz the power, 2016-04-20 22:05:22]

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

[commit-bot: I haz the power, 2016-04-21 00:22:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:28:34]

Description was changed from

==========
[Blob] Added error reporting metrics to invalid message errors.

This helps us get to the bottom of where the invalid blob refcount
and url calls are. It's unclear whether these are new or if they've
always occurred, as we used to ignore these messages.

R=kinuko@chromium.org, mpearson@chromium.org
BUG=602443
==========

to

==========
[Blob] Added error reporting metrics to invalid message errors.

This helps us get to the bottom of where the invalid blob refcount
and url calls are. It's unclear whether these are new or if they've
always occurred, as we used to ignore these messages.

R=kinuko@chromium.org, mpearson@chromium.org
BUG=602443

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

[commit-bot: I haz the power, 2016-04-22 19:28:36]

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

[dmurph, 2016-04-29 21:21:54]

Description was changed from

==========
[Blob] Added error reporting metrics to invalid message errors.

This helps us get to the bottom of where the invalid blob refcount
and url calls are. It's unclear whether these are new or if they've
always occurred, as we used to ignore these messages.

R=kinuko@chromium.org, mpearson@chromium.org
BUG=602443

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

to

==========
[Blob] Added error reporting metrics to invalid message errors.

This helps us get to the bottom of where the invalid blob refcount
and url calls are. It's unclear whether these are new or if they've
always occurred, as we used to ignore these messages.

R=kinuko@chromium.org, mpearson@chromium.org
BUG=602443, 607698

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