Adding cache hit/miss histograms to DiskBasedCertCache.

net/http/disk_based_cert_cache.cc

 // Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/disk_based_cert_cache.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/memory/ref_counted.h"
+#include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/disk_cache.h"
 
 namespace net {
 
 namespace {
 
 // TODO(brandonsalmon): change this number to improve performance.
 const size_t kMemoryCacheMaxSize = 30;
 
 // Used to obtain a unique cache key for a certificate in the form of
 // "cert:<hash>".
-std::string GetCacheKeyToCert(const X509Certificate::OSCertHandle cert_handle) {
+std::string GetCacheKeyForCert(
+    const X509Certificate::OSCertHandle cert_handle) {
   SHA1HashValue fingerprint =
       X509Certificate::CalculateFingerprint(cert_handle);
 
   return "cert:" +
          base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
 }
 
+enum CacheResult {
+  MEMORY_CACHE_HIT = 0,
+  DISK_CACHE_HIT,
+  DISK_CACHE_CORRUPT,
+  CACHE_MISS,
+  CACHE_ERROR,

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

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.

+  CACHE_RESULT_MAX
+};
+
+void RecordCacheResult(CacheResult result) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "DiskBasedCertCache.CertIoCacheResult", result, CACHE_RESULT_MAX);
+}
+
 }  // namespace
 
 // WriteWorkers represent pending Set jobs in the DiskBasedCertCache. Each
 // certificate requested to be cached is assigned a Writeworker on a one-to-one
 // basis. The same certificate should not have multiple WriteWorkers at the same
 // time; instead, add a user callback to the existing WriteWorker.
 class DiskBasedCertCache::WriteWorker {
  public:
   // |backend| is the backend to store |certificate| in, using
   // |key| as the key for the disk_cache::Entry.
@@ skipping 71 matching lines @@
       canceled_(false),
       entry_(NULL),
       state_(STATE_NONE),
       io_buf_len_(0),
       cleanup_callback_(cleanup_callback),
       io_callback_(
           base::Bind(&WriteWorker::OnIOComplete, base::Unretained(this))) {
 }
 
 DiskBasedCertCache::WriteWorker::~WriteWorker() {
-  X509Certificate::FreeOSCertHandle(cert_handle_);
+  if (cert_handle_)
+    X509Certificate::FreeOSCertHandle(cert_handle_);
   if (entry_)
     entry_->Close();
 }
 
 void DiskBasedCertCache::WriteWorker::Start() {
   DCHECK_EQ(STATE_NONE, state_);
   state_ = STATE_CREATE;
   int rv = DoLoop(OK);
 
   if (rv == ERR_IO_PENDING)
@@ skipping 284 matching lines @@
 
   return rv;
 }
 
 int DiskBasedCertCache::ReadWorker::DoOpen() {
   state_ = STATE_OPEN_COMPLETE;
   return backend_->OpenEntry(key_, &entry_, io_callback_);
 }
 
 int DiskBasedCertCache::ReadWorker::DoOpenComplete(int rv) {
-  if (rv < 0)
+  if (rv < 0) {
+    // Errors other than ERR_CACHE_MISS are not recorded as either a hit
+    // or a miss.
+    RecordCacheResult(rv == ERR_CACHE_MISS ? CACHE_MISS : CACHE_ERROR);
     return rv;
+  }
 
   state_ = STATE_READ;
   return OK;
 }
 
 int DiskBasedCertCache::ReadWorker::DoRead() {
   state_ = STATE_READ_COMPLETE;
   io_buf_len_ = entry_->GetDataSize(0 /* index */);
   buffer_ = new IOBuffer(io_buf_len_);
   return entry_->ReadData(
       0 /* index */, 0 /* offset */, buffer_, io_buf_len_, io_callback_);
 }
 
 int DiskBasedCertCache::ReadWorker::DoReadComplete(int rv) {
   if (rv < io_buf_len_)
     return ERR_FAILED;

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

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

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

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.)

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

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.

 
   cert_handle_ = X509Certificate::CreateOSCertHandleFromBytes(buffer_->data(),
                                                               io_buf_len_);
-  if (!cert_handle_)
+  if (!cert_handle_) {
+    RecordCacheResult(DISK_CACHE_CORRUPT);

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

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.

     return ERR_FAILED;
+  }
 
+  RecordCacheResult(DISK_CACHE_HIT);
   return OK;
 }
 
 void DiskBasedCertCache::ReadWorker::Finish(int rv) {
   cleanup_callback_.Run(cert_handle_);
   cleanup_callback_.Reset();
   RunCallbacks();
   delete this;
 }
 
@@ skipping 34 matching lines @@
 }
 
 void DiskBasedCertCache::Get(const std::string& key, const GetCallback& cb) {
   DCHECK(!key.empty());
 
   // If the handle is already in the MRU cache, just return that (via callback).
   // Note, this will also bring the cert_handle to the front of the recency
   // list in the MRU cache.
   MRUCertCache::iterator mru_it = mru_cert_cache_.Get(key);
   if (mru_it != mru_cert_cache_.end()) {
+    RecordCacheResult(MEMORY_CACHE_HIT);
     ++mem_cache_hits_;
     cb.Run(mru_it->second);
     return;
   }
   ++mem_cache_misses_;
 
   ReadWorkerMap::iterator it = read_worker_map_.find(key);
 
   if (it == read_worker_map_.end()) {
     ReadWorker* worker =
         new ReadWorker(backend_,
                        key,
                        base::Bind(&DiskBasedCertCache::FinishedReadOperation,
                                   weak_factory_.GetWeakPtr(),
                                   key));
     read_worker_map_[key] = worker;
     worker->AddCallback(cb);
     worker->Start();
   } else {
     it->second->AddCallback(cb);
   }
 }
 
 void DiskBasedCertCache::Set(const X509Certificate::OSCertHandle cert_handle,
                              const SetCallback& cb) {
   DCHECK(!cb.is_null());
   DCHECK(cert_handle);
-  std::string key = GetCacheKeyToCert(cert_handle);
+  std::string key = GetCacheKeyForCert(cert_handle);
 
   WriteWorkerMap::iterator it = write_worker_map_.find(key);
 
   if (it == write_worker_map_.end()) {
     WriteWorker* worker =
         new WriteWorker(backend_,
                         key,
                         cert_handle,
                         base::Bind(&DiskBasedCertCache::FinishedWriteOperation,
                                    weak_factory_.GetWeakPtr(),
@@ skipping 17 matching lines @@
 
 void DiskBasedCertCache::FinishedWriteOperation(
     const std::string& key,
     X509Certificate::OSCertHandle cert_handle) {
   write_worker_map_.erase(key);
   if (!key.empty())
     mru_cert_cache_.Put(key, X509Certificate::DupOSCertHandle(cert_handle));
 }
 
 }  // namespace net