Add histograms to record the severity scores for certain SSL errors.

Add histograms to record the severity scores for certain SSL errors.

BUG=403978
Committed: https://crrev.com/235c6c6e9556a7249b45d72dc33cbcaf94f50bf3
Cr-Commit-Position: refs/heads/master@{#294531}

[palmer, 2014-09-09 00:16:21]

palmer@chromium.org changed reviewers:
+ agl@chromium.org, felt@chromium.org, mpearson@chromium.org

[palmer, 2014-09-09 00:16:22]

agl and mpearson for OWNERS

felt for comparison to https://codereview.chromium.org/474103002/, which is
superceded by this CL.

[felt, 2014-09-09 01:11:20]

https://codereview.chromium.org/549363002/diff/20001/chrome/browser/ssl/ssl_e...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/549363002/diff/20001/chrome/browser/ssl/ssl_e...
chrome/browser/ssl/ssl_error_classification.cc:72:
UMA_HISTOGRAM_CUSTOM_COUNTS("interstitial.ssl.severity_score.date_invalid",
should this one be UMA_HISTOGRAM_COUNTS_100 too?

[Mark P, 2014-09-10 20:41:37]

https://codereview.chromium.org/549363002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/549363002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10516: +    |CERT_COMMON_NAME_INVALID|
error. Higher the percentage, higher the
For the first sentence, do you mean:
The likelihood of an CERT_COMMON_NAME_INVALID error being an attack.
?

https://codereview.chromium.org/549363002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10516: +    |CERT_COMMON_NAME_INVALID|
error. Higher the percentage, higher the
nit: higher -> the higher
(two places)

Or omit the last sentence entirely.  Personally, I don't think it adds anything.

[palmer, 2014-09-11 18:03:48]

https://codereview.chromium.org/549363002/diff/20001/chrome/browser/ssl/ssl_e...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/549363002/diff/20001/chrome/browser/ssl/ssl_e...
chrome/browser/ssl/ssl_error_classification.cc:72:
UMA_HISTOGRAM_CUSTOM_COUNTS("interstitial.ssl.severity_score.date_invalid",
> should this one be UMA_HISTOGRAM_COUNTS_100 too?

bool oops_youre_right = static_cast<bool>("totes");

https://codereview.chromium.org/549363002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/549363002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10516: +    |CERT_COMMON_NAME_INVALID|
error. Higher the percentage, higher the
On 2014/09/10 20:41:36, Mark P wrote:
> For the first sentence, do you mean:
> The likelihood of an CERT_COMMON_NAME_INVALID error being an attack.
> ?

Done.

https://codereview.chromium.org/549363002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10516: +    |CERT_COMMON_NAME_INVALID|
error. Higher the percentage, higher the
On 2014/09/10 20:41:36, Mark P wrote:
> nit: higher -> the higher
> (two places)
> 
> Or omit the last sentence entirely.  Personally, I don't think it adds
anything.

Done.

[Mark P, 2014-09-11 18:40:07]

lgtm

[felt, 2014-09-11 19:06:32]

lgtm

[palmer, 2014-09-11 20:48:54]

palmer@chromium.org changed reviewers:
+ rsleevi@chromium.org

[palmer, 2014-09-11 20:49:24]

sleevi or agl: Can I get OWNERS review for chrome/browser/ssl? Thanks!

[Ryan Sleevi, 2014-09-11 20:51:08]

https://codereview.chromium.org/549363002/diff/40001/chrome/browser/ssl/ssl_e...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/549363002/diff/40001/chrome/browser/ssl/ssl_e...
chrome/browser/ssl/ssl_error_classification.cc:76:
SSLErrorInfo::CERT_COMMON_NAME_INVALID) {
These are mutually exclusive conditions, right? Wouldn't an else if be more
appropriate?

[Ryan Sleevi, 2014-09-11 20:56:25]

https://codereview.chromium.org/549363002/diff/40001/chrome/browser/ssl/ssl_e...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/549363002/diff/40001/chrome/browser/ssl/ssl_e...
chrome/browser/ssl/ssl_error_classification.cc:72:
UMA_HISTOGRAM_COUNTS_100("interstitial.ssl.severity_score.date_invalid",
UMA_HISTOGRAM_COUNTS_100 creates 50 buckets for 1-100.

What's the actual range for your severity score? Is there any bound? You
multiply by 100, which makes me think you're trying to get a percentage, but
that's not clear (if so, HISTOGRAM_PERCENTAGE offers an alternative that buckets
0-100)

[palmer, 2014-09-11 20:58:30]

https://codereview.chromium.org/549363002/diff/40001/chrome/browser/ssl/ssl_e...
File chrome/browser/ssl/ssl_error_classification.cc (right):

https://codereview.chromium.org/549363002/diff/40001/chrome/browser/ssl/ssl_e...
chrome/browser/ssl/ssl_error_classification.cc:76:
SSLErrorInfo::CERT_COMMON_NAME_INVALID) {
On 2014/09/11 20:51:07, Ryan Sleevi wrote:
> These are mutually exclusive conditions, right? Wouldn't an else if be more
> appropriate?

Done.

[palmer, 2014-09-11 22:08:50]

On 2014/09/11 20:56:25, Ryan Sleevi wrote:

> What's the actual range for your severity score? Is there any bound?

It's a float between 0.0 – 1.0. The precision is mostly fake; it's not really a
percentage so much as a quantization. As such, it's still sampling at a higher
precision than we need and than we have measured. But it's not likely to produce
incorrect results.

[Ryan Sleevi, 2014-09-11 23:24:44]

On 2014/09/11 22:08:50, Chromium Palmer wrote:
> On 2014/09/11 20:56:25, Ryan Sleevi wrote:
> 
> > What's the actual range for your severity score? Is there any bound?
> 
> It's a float between 0.0 – 1.0. The precision is mostly fake; it's not really
a
> percentage so much as a quantization. As such, it's still sampling at a higher
> precision than we need and than we have measured. But it's not likely to
produce
> incorrect results.

Well, the issue here is you're cutting your precision in half with
UMA_HISTOGRAM_COUNTS_100. It sounds like percentage is what you want, based on
how you're using it (*100). But that is up to you.

LGTM, but only barely.

[palmer, 2014-09-11 23:29:32]

> Well, the issue here is you're cutting your precision in half with
> UMA_HISTOGRAM_COUNTS_100. It sounds like percentage is what you want, based on
> how you're using it (*100). But that is up to you.

Yes; but in the end, there are only going to be like 1 – 5 meaningful values for
these scores anyway. Heuristics; but also, beer.

> LGTM, but only barely.

That's the kind of quantization I can get behind!

[palmer, 2014-09-11 23:30:44]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-09-12 00:15:08]

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

[commit-bot: I haz the power, 2014-09-12 02:44:17]

Committed patchset #4 (id:60001) as 0a0b239ff7c627458e71444ed6be5bae70f2495c

[commit-bot: I haz the power, 2014-09-12 02:47:58]

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