Fix up certificate error reporting histograms

Fix up certificate error reporting histograms

This CL adds a failure histogram for certificate error reporting,
similar to the histograms we have for Expect CT and HPKP reports.

It also negates the net error codes for Expect CT and HPKP histograms,
to match up with the histogram enums. (See bug 616599.)

BUG=616599, 596624
Committed: https://crrev.com/a79bbb7b735bfbc7b84518bdd6f166a2fb39f7c1
Cr-Commit-Position: refs/heads/master@{#406604}

[estark, 2016-07-01 21:45:48]

estark@chromium.org changed reviewers:
+ eroman@chromium.org, felt@chromium.org, mpearson@chromium.org

[estark, 2016-07-01 21:45:49]

Can you please review...
felt: components/certificate_reporting and chrome/browser/ssl
eroman: net/http/transport_security_state
mpearson: histograms.xml
Thanks!

[estark, 2016-07-01 21:46:47]

https://codereview.chromium.org/2120683002/diff/1/components/certificate_repo...
File components/certificate_reporting/DEPS (right):

https://codereview.chromium.org/2120683002/diff/1/components/certificate_repo...
components/certificate_reporting/DEPS:2: "+content/public/test",
I'm not 100% sure if this is allowed? I see other examples of it in components
though (e.g.
https://cs.chromium.org/chromium/src/components/safe_browsing_db/DEPS).

[felt, 2016-07-01 22:03:13]

https://codereview.chromium.org/2120683002/diff/1/components/certificate_repo...
File components/certificate_reporting/DEPS (right):

https://codereview.chromium.org/2120683002/diff/1/components/certificate_repo...
components/certificate_reporting/DEPS:2: "+content/public/test",
On 2016/07/01 21:46:47, estark wrote:
> I'm not 100% sure if this is allowed? I see other examples of it in components
> though (e.g.
> https://cs.chromium.org/chromium/src/components/safe_browsing_db/DEPS).

A component can depend on content if it is not used by iOS, assuming the docs
are still up to date:
https://www.chromium.org/developers/design-documents/cookbook

[felt, 2016-07-01 22:05:58]

https://codereview.chromium.org/2120683002/diff/1/chrome/browser/ssl/chrome_e...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/2120683002/diff/1/chrome/browser/ssl/chrome_e...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:110:
UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.ExpectCTReportFailure", -net_error);
Will this change the histogram? Do you need to define a new histogram to avoid
messed up results in the transition?

[estark, 2016-07-01 22:08:53]

https://codereview.chromium.org/2120683002/diff/1/chrome/browser/ssl/chrome_e...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/2120683002/diff/1/chrome/browser/ssl/chrome_e...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:110:
UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.ExpectCTReportFailure", -net_error);
On 2016/07/01 22:05:58, felt wrote:
> Will this change the histogram? Do you need to define a new histogram to avoid
> messed up results in the transition?

It will change it, but I think it's okay. I'm pretty sure I'm the only one
looking at these, and it just means that for a while we'll have to add two
numbers together (the negative version and the positive version) to get the full
count for a given error code. Maybe I should add a note to the histograms.xml
description?

[felt, 2016-07-01 22:13:48]

https://codereview.chromium.org/2120683002/diff/1/chrome/browser/ssl/chrome_e...
File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):

https://codereview.chromium.org/2120683002/diff/1/chrome/browser/ssl/chrome_e...
chrome/browser/ssl/chrome_expect_ct_reporter.cc:110:
UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.ExpectCTReportFailure", -net_error);
On 2016/07/01 22:08:53, estark wrote:
> On 2016/07/01 22:05:58, felt wrote:
> > Will this change the histogram? Do you need to define a new histogram to
avoid
> > messed up results in the transition?
> 
> It will change it, but I think it's okay. I'm pretty sure I'm the only one
> looking at these, and it just means that for a while we'll have to add two
> numbers together (the negative version and the positive version) to get the
full
> count for a given error code. Maybe I should add a note to the histograms.xml
> description?

If you're the only one looking at it, probably fine, but please do add a note.

[estark, 2016-07-01 22:18:38]

On 2016/07/01 22:13:48, felt wrote:
>
https://codereview.chromium.org/2120683002/diff/1/chrome/browser/ssl/chrome_e...
> File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right):
> 
>
https://codereview.chromium.org/2120683002/diff/1/chrome/browser/ssl/chrome_e...
> chrome/browser/ssl/chrome_expect_ct_reporter.cc:110:
> UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.ExpectCTReportFailure", -net_error);
> On 2016/07/01 22:08:53, estark wrote:
> > On 2016/07/01 22:05:58, felt wrote:
> > > Will this change the histogram? Do you need to define a new histogram to
> avoid
> > > messed up results in the transition?
> > 
> > It will change it, but I think it's okay. I'm pretty sure I'm the only one
> > looking at these, and it just means that for a while we'll have to add two
> > numbers together (the negative version and the positive version) to get the
> full
> > count for a given error code. Maybe I should add a note to the
histograms.xml
> > description?
> 
> If you're the only one looking at it, probably fine, but please do add a note.

Done.

Added the content test dep to BUILD.gn too, which will hopefully please the
trybots. This component isn't used on iOS.

[eroman, 2016-07-01 22:28:25]

These histograms (with negative errors) are in M52 right?

I definitely think you want to rename them instead -- perhaps by appending a
"2", and mark the original names as deprecated.

There isn't really any downside to the uglier name IMO.

[estark, 2016-07-01 22:44:03]

On 2016/07/01 22:28:25, eroman wrote:
> These histograms (with negative errors) are in M52 right?
> 
> I definitely think you want to rename them instead -- perhaps by appending a
> "2", and mark the original names as deprecated.
> 
> There isn't really any downside to the uglier name IMO.

Happy to do that, but I'm curious why? To me it seems about the same (a
deprecated set of values in one histogram versus two histograms, one deprecated
and one not-deprecated, with disjoint values).

[eroman, 2016-07-01 22:56:29]

The advantage is having cleaner data.

When you look at a timeline view of these histograms, you are generally
aggregating across multiple versions of Chrome. A version with the old data
would pollute that display.

This pollution may persist for a while, since old versions of Chrome have a way
of continuing to contribute data many releases in.

Yes it is not a big deal, the person looking and interpreting the data could
figure it out, or adjust the query to exclude earlier milestones. But that
person may not be you :)

Whereas if the histogram is just renamed, you don't haver to worry about these
particular artifacts when reviewing the data, since it necessarily come from
milestones after 52. And if you look at the older version the description
explains it is deprecated and why.

At any rate, I will let a histograms OWNER speak more authoritatively. But I
don't see a good reason why we shouldn't just change the histogram name since it
is effectively measuring something different.

[eroman, 2016-07-01 23:00:40]

//net LGTM, modulo the outstanding question about naming.

[estark, 2016-07-01 23:24:53]

On 2016/07/01 22:56:29, eroman wrote:
> The advantage is having cleaner data.
> 
> When you look at a timeline view of these histograms, you are generally
> aggregating across multiple versions of Chrome. A version with the old data
> would pollute that display.
> 
> This pollution may persist for a while, since old versions of Chrome have a
way
> of continuing to contribute data many releases in.
> 
> Yes it is not a big deal, the person looking and interpreting the data could
> figure it out, or adjust the query to exclude earlier milestones. But that
> person may not be you :)
> 
> Whereas if the histogram is just renamed, you don't haver to worry about these
> particular artifacts when reviewing the data, since it necessarily come from
> milestones after 52. And if you look at the older version the description
> explains it is deprecated and why.
> 
> At any rate, I will let a histograms OWNER speak more authoritatively. But I
> don't see a good reason why we shouldn't just change the histogram name since
it
> is effectively measuring something different.

Thanks for the explanation. Yeah, I think it would be good if mpearson could
weigh in as a histograms owner. What you describe as "pollution" seems like
valuable data to me. :) I'm imagining trying to decide if there were more
ERR_CONNECTION_REFUSED errors this month than last month. I might pull up the
Net.PublicKeyPinReportSendingFailure2 histogram and compare the
ERR_CONNECTION_REFUSED buckets from this month and last month and conclude that
there were more this month, but forget to check the
Net.PublicKeyPinReportSendingFailure histogram for the data from older clients.
At least if it's all in one histogram I'll hopefully see the negative values
and/or the histogram description and remember that they need to be added
together.

[Mark P, 2016-07-08 17:50:58]

I share eroman@'s concerns (details below).

--mark

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

https://codereview.chromium.org/2120683002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29146: +    and later, the error code is
positive.
I don't like this solution.

Here's why: after this change, the net errors with positive values will be
displayed as human-readable strings in the dashboard. To a quick glance, this
makes the negative values look like bugs / coding errors, not regular /
intended-to-be-reported errors.  If a user wants to answer a question such as
what percent of errors are timeouts, the easiest way is to look at the percent
by the TIMED_OUT line, ignoring the unlabeled values.  But, with this solution,
to do it correctly one would have to notice the warning message / not ignore
out-of-hand the negative values, remember or look up the net error code for
TIMED_OUT, find that negative value, add it to the positive row labeled
TIMED_OUT and then calculate the percent.  You expect a lot from people.

Furthermore, if you use something like the timeline view to graph errors over
time, it doesn't support this type of bucket adding.  And it does sound like
you're wanting to look at data over time: "I'm imagining trying to decide if
there were more ERR_CONNECTION_REFUSED errors this month than last month."

I'd prefer either of these two strategies:
- rename the bad histograms, marking the old ones was obsolete
- add to the histograms.xml NetErrorCodes enum all neterrors as negative values
as well as positive.  You're not likely to be the last person to make this
mistake, and this will prevent needing these corrections in the future.  This
will require updating
  tools/metrics/histograms/update_net_error_codes.py
  and probably also necessitate some discussion with another member of the UMA
team to make strategy is sound.

estark@ wrote: "I'm pretty sure I'm the only one looking at these"
which may be true now, but may change in the future, and users do get stuck on
old versions indefinitely so these negative values will not go away entirely.

[estark, 2016-07-12 18:30:12]

Ok, fair enough. Though, Mark, you suggested renaming the old histograms -- is
there a way to do that? In the latest patch set I've named the new histograms
with a "2" suffix and marked the old ones as obsolete. Please let me know if
there's a way to instead rename the old ones. Thanks.

[Mark P, 2016-07-12 18:33:33]

histograms lgtm

On 2016/07/12 18:30:12, estark wrote:
> Ok, fair enough. Though, Mark, you suggested renaming the old histograms -- is
> there a way to do that? In the latest patch set I've named the new histograms
> with a "2" suffix and marked the old ones as obsolete. Please let me know if
> there's a way to instead rename the old ones. Thanks.

You did it correctly.  (That's what I meant.)

--mark

[estark, 2016-07-12 21:53:33]

felt: lgty?

Adding a //content/test:test_support dependency in the component's unit tests
was breaking the ios/chrome/browser build, which depends on this component. It's
actually an unused dependency, though -- we don't do cert reporting on iOS -- so
I just removed it.

(If it were a used dependency, not sure what the right way to fix it would be...
maybe making it a layered component.)

[felt, 2016-07-12 21:54:49]

On 2016/07/12 21:53:33, estark wrote:
> felt: lgty?
> 
> Adding a //content/test:test_support dependency in the component's unit tests
> was breaking the ios/chrome/browser build, which depends on this component.
It's
> actually an unused dependency, though -- we don't do cert reporting on iOS --
so
> I just removed it.
> 
> (If it were a used dependency, not sure what the right way to fix it would
be...
> maybe making it a layered component.)

Yes, a layered component is the solution for that.

[felt, 2016-07-12 21:55:47]

lgtm

[estark, 2016-07-12 21:57:20]

estark@chromium.org changed reviewers:
+ sdefresne@chromium.org

[estark, 2016-07-12 21:57:21]

sdefresne, can you please review components/BUILD.gn and
ios/chrome/browser/DEPS?

[estark, 2016-07-15 20:16:03]

friendly ping to sdefresne

[sdefresne, 2016-07-18 13:26:40]

components/BUILD.gn and ios/chrome/browser/DEPS lgtm

[estark, 2016-07-18 15:56:11]

The CQ bit was checked by estark@chromium.org

[estark, 2016-07-18 15:56:11]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2120683002/#ps140001
(title: "remove unused ios certificate_reporting dependency")

[commit-bot: I haz the power, 2016-07-18 15:56:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 16:07:39]

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

[commit-bot: I haz the power, 2016-07-18 16:07:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-07-18 16:11:01]

estark@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[estark, 2016-07-18 16:11:02]

phajdan.jr could you please approve for adding content/public/test to
components/certificate_reporting/DEPS?

[Paweł Hajdan Jr., 2016-07-20 15:23:06]

LGTM

[estark, 2016-07-20 16:32:54]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-07-20 16:33:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 17:43:47]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-20 17:44:16]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 17:47:01]

Description was changed from

==========
Fix up certificate error reporting histograms

This CL adds a failure histogram for certificate error reporting,
similar to the histograms we have for Expect CT and HPKP reports.

It also negates the net error codes for Expect CT and HPKP histograms,
to match up with the histogram enums. (See bug 616599.)

BUG=616599,596624
==========

to

==========
Fix up certificate error reporting histograms

This CL adds a failure histogram for certificate error reporting,
similar to the histograms we have for Expect CT and HPKP reports.

It also negates the net error codes for Expect CT and HPKP histograms,
to match up with the histogram enums. (See bug 616599.)

BUG=616599,596624

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

[commit-bot: I haz the power, 2016-07-20 17:47:03]

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

[jam, 2016-07-21 00:47:01]

On 2016/07/18 16:11:02, estark wrote:
> phajdan.jr could you please approve for adding content/public/test to
> components/certificate_reporting/DEPS?

drive-by: in general if a component or directory doesn't use content, it's best
to avoid depending on it for tests. In this case, you can use a
base::MessageLoopForIO instead of the browser test bundle.

[estark, 2016-07-21 01:16:25]

On 2016/07/21 00:47:01, jam wrote:
> On 2016/07/18 16:11:02, estark wrote:
> > phajdan.jr could you please approve for adding content/public/test to
> > components/certificate_reporting/DEPS?
> 
> drive-by: in general if a component or directory doesn't use content, it's
best
> to avoid depending on it for tests. In this case, you can use a
> base::MessageLoopForIO instead of the browser test bundle.

Thanks! I'll submit a fix.