Adding cache hit/miss histograms to DiskBasedCertCache.

Adding cache hit/miss histograms to DiskBasedCertCache. There are separate
histograms for the in-memory MRU cache and the disk cache.

This cl is closely related to https://codereview.chromium.org/356953003/, which implements similar histograms in the http_cache.

and a follow up to https://codereview.chromium.org/361513003/

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282584

[brandonsalmon, 2014-07-08 23:57:10]

I added a few histograms to deal with cache hits/misses. 

Note: I made a new cl for this because the git branch I'm using for the other cl
isn't up to date with the addition of the MRU cache to disk_based_cert_cache.cc.

[Ryan Sleevi, 2014-07-09 00:10:10]

jar: Should we prefix these histograms with "Net." or not? net/ seems
inconsistent in this regard.

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

https://codereview.chromium.org/378063002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:3502: +<histogram
name="DiskBasedCertCache.DiskCacheHits">
Rather than describing in prose what true/false (really, 0/1) mean, you can use
enum="SomeEnumName" to describe what the values are.

See BooleanEnabled or BooleanSuccess for examples of this.

https://codereview.chromium.org/378063002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:3506: +    disk using
DiskBasedCertCache. True for hits, false for misses.
Measure whether a certificate was found cached on disk by the DiskBasedCertCache
?

https://codereview.chromium.org/378063002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:3514: +    True for hits, false for
misses.
Measure whether a certificate was found in net::DiskBasedCertCache's in-memory
MRU cache ?

[wtc, 2014-07-09 14:55:36]

Patch set 1 LGTM.

I skimmed through histograms.xml, but I checked the code in
disk_based_cert_cache.cc carefully.

https://codereview.chromium.org/378063002/diff/1/net/http/disk_based_cert_cac...
File net/http/disk_based_cert_cache.cc (right):

https://codereview.chromium.org/378063002/diff/1/net/http/disk_based_cert_cac...
net/http/disk_based_cert_cache.cc:445: return rv;

Nit: I would structure this as follows:

  if (rv < 0) {
    // Errors other than cache miss are not recorded as either a hit
    // or a miss.
    if (rv == ERR_CACHE_MISS)
      RecordDiskCacheHits(false);
    return rv;
  }

so that we don't do the rv == ERR_CACHE_MISS test when rv is OK. This is a very
minor issue, so please pick the form that you think is more readable.

https://codereview.chromium.org/378063002/diff/1/net/http/disk_based_cert_cac...
net/http/disk_based_cert_cache.cc:446: RecordDiskCacheHits(true);

This means a corrupted disk cache entry is still reported as a cache hit. I
think this is reasonable.

[brandonsalmon, 2014-07-09 18:17:06]

I added an enum to histograms.xml to use for my histograms. Not sure why a
generic cache hit/miss enumeration didn't yet exist.

[Ryan Sleevi, 2014-07-09 19:00:52]

lgtm

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

https://codereview.chromium.org/378063002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:35338: +<enum name="CacheHit"
type="int">
Jar can confirm, but I believe the preference is to name these Boolean-prefix
style

(BooleanHit?)

More of a nit though.

[wtc, 2014-07-09 21:07:37]

Patch set 3 LGTM.

https://codereview.chromium.org/378063002/diff/60001/net/http/disk_based_cert...
File net/http/disk_based_cert_cache.cc (right):

https://codereview.chromium.org/378063002/diff/60001/net/http/disk_based_cert...
net/http/disk_based_cert_cache.cc:443: // Errors other than CACHE_MISS are not
recorded as either a hit

Nit: I prefer either the original "cache miss" or the full error code name
"ERR_CACHE_MISS".

[jar (doing other things), 2014-07-09 22:22:49]

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

https://codereview.chromium.org/378063002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:3510: +<histogram
name="DiskBasedCertCache.MemCacheHit" enum="CacheHit">
nit: 
Suggest one histogram, tot replace these two, with three enumerated values:
MemoryHit
DiskHit
CacheMiss

[jar (doing other things), 2014-07-10 00:04:58]

LGTM % nits below. Please add them before hitting the CQ.

https://chromiumcodereview.appspot.com/378063002/diff/80001/net/http/disk_bas...
File net/http/disk_based_cert_cache.cc (right):

https://chromiumcodereview.appspot.com/378063002/diff/80001/net/http/disk_bas...
net/http/disk_based_cert_cache.cc:37: enum CacheResult { MEMORY_CACHE_HIT = 0,
DISK_CACHE_HIT, CACHE_MISS };
nit: Add one more enum:
CACHE_RESULT_MAX

Use that in the UMA_HISTOGRAM_ENUMERATION.

https://chromiumcodereview.appspot.com/378063002/diff/80001/net/http/disk_bas...
net/http/disk_based_cert_cache.cc:41: "DiskBasedCertCache.CertIoCacheResult",
result, CACHE_MISS);
nit: Replace CACHE_MISS With CACHE_RESULT_MAX

https://chromiumcodereview.appspot.com/378063002/diff/80001/net/http/disk_bas...
net/http/disk_based_cert_cache.cc:442: // Errors other than ERR_CACHE_MISS are
not recorded as either a hit
Consider adding one more enum, which captures these error cases.  It is nice to
not lose data... even when you predict it will be non-existant... sometimes we
learn stuff ;-).

https://chromiumcodereview.appspot.com/378063002/diff/80001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/378063002/diff/80001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:35332: +  <int value="2"
label="CACHE_MISS"/>
again... add the error case here please.

(you don't need the max)

[brandonsalmon, 2014-07-10 17:46:21]

I added one more possibility to the CacheResult enum, so that it is possible to
differentiate corrupted cache hits and successful cache hits.

[wtc, 2014-07-10 18:21:38]

Patch set 6 LGTM.

https://chromiumcodereview.appspot.com/378063002/diff/120001/net/http/disk_ba...
File net/http/disk_based_cert_cache.cc (right):

https://chromiumcodereview.appspot.com/378063002/diff/120001/net/http/disk_ba...
net/http/disk_based_cert_cache.cc:42: CACHE_ERROR,

Nit: without looking at the source code, the meaning of CACHE_ERROR is not
clear. This should be documented or renamed in a more self-documenting way, such
as DISK_CACHE_OPEN_ENTRY_ERROR or DISK_CACHE_OPEN_ENTRY_FAILED.

https://chromiumcodereview.appspot.com/378063002/diff/120001/net/http/disk_ba...
net/http/disk_based_cert_cache.cc:469: return ERR_FAILED;

Nit: you missed this error return (entry_->ReadData() failed or returned fewer
bytes than expected).

https://chromiumcodereview.appspot.com/378063002/diff/120001/net/http/disk_ba...
net/http/disk_based_cert_cache.cc:474: RecordCacheResult(DISK_CACHE_CORRUPT);

This enum value should be named so it's clear only an entry is corrupted, for
example, DISK_CACHE_ENTRY_CORRUPT or DISK_CACHE_ENTRY_BAD_DATA.

https://chromiumcodereview.appspot.com/378063002/diff/120001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/378063002/diff/120001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:3502: +<histogram
name="DiskBasedCertCache.CertIoCacheResult" enum="CacheResult">

Nit: what does the "Io" mean in the histogram name?

https://chromiumcodereview.appspot.com/378063002/diff/120001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:3505: +    Measures the frequency of
cache hits in the DiskBasedCertCache.

This summary needs updating. This histogram is not just measuring cache hits".

[brandonsalmon, 2014-07-10 18:33:38]

https://chromiumcodereview.appspot.com/378063002/diff/120001/net/http/disk_ba...
File net/http/disk_based_cert_cache.cc (right):

https://chromiumcodereview.appspot.com/378063002/diff/120001/net/http/disk_ba...
net/http/disk_based_cert_cache.cc:469: return ERR_FAILED;
On 2014/07/10 18:21:38, wtc wrote:
> 
> Nit: you missed this error return (entry_->ReadData() failed or returned fewer
> bytes than expected).

Do you think that this is also cache entry corruption? It might be better to
label it a CACHE_ERROR. (but then it would not make sense to rename CACHE_ERROR
to DISK_CACHE_OPEN_ENTRY_ERROR like you suggested.)

[brandonsalmon, 2014-07-10 18:42:39]

https://chromiumcodereview.appspot.com/378063002/diff/120001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/378063002/diff/120001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:3502: +<histogram
name="DiskBasedCertCache.CertIoCacheResult" enum="CacheResult">
On 2014/07/10 18:21:38, wtc wrote:
> 
> Nit: what does the "Io" mean in the histogram name?

Oops missed this comment. I did this because Jim recommended it was best to have
things grouped by prefixes (so they would be grouped nicely when they show up in
alphabetical order). It might make more sense after seeing how I redid the
histograms in the other cl, https://codereview.chromium.org/356953003/.

[wtc, 2014-07-10 18:49:36]

https://chromiumcodereview.appspot.com/378063002/diff/120001/net/http/disk_ba...
File net/http/disk_based_cert_cache.cc (right):

https://chromiumcodereview.appspot.com/378063002/diff/120001/net/http/disk_ba...
net/http/disk_based_cert_cache.cc:469: return ERR_FAILED;

On 2014/07/10 18:33:38, brandonsalmon wrote:
>
> Do you think that this is also cache entry corruption?

I don't know. You may want to read the entry_->ReadData() source code.

You can decide the granularity of error reporting. A specific error code for
each error return, CACHE_ERROR for all errors except cache misses, or something
between these two extremes, are all fine by me. These errors aren't why you are
adding this histogram.

[brandonsalmon, 2014-07-10 19:24:08]

I added another error case and edited the description of the cache result
histogram.

[brandonsalmon, 2014-07-10 21:31:28]

The CQ bit was checked by brandonsalmon@chromium.org

[commit-bot: I haz the power, 2014-07-10 21:32:14]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/378063002/1...

[commit-bot: I haz the power, 2014-07-11 02:10:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-11 02:13:27]

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

[commit-bot: I haz the power, 2014-07-11 02:13:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[brandonsalmon, 2014-07-11 02:26:42]

After talking with Ryan I believe the issue with the patch was a merge conflict.
I have attempted to remedy that merge conflict and will try uploading again.

[brandonsalmon, 2014-07-11 02:29:46]

The CQ bit was checked by brandonsalmon@chromium.org

[commit-bot: I haz the power, 2014-07-11 02:31:53]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/378063002/1...

[commit-bot: I haz the power, 2014-07-11 06:09:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-11 07:45:13]

Change committed as 282584