Certificate Transparency: Collect metrics on age of SCT vs STH

components/certificate_transparency/single_tree_tracker.h

 // Copyright 2016 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.
 
 #ifndef COMPONENTS_CERTIFICATE_TRANSPARENCY_SINGLE_TREE_TRACKER_H_
 #define COMPONENTS_CERTIFICATE_TRANSPARENCY_SINGLE_TREE_TRACKER_H_
 
 #include <map>
 #include <string>
 
 #include "base/memory/ref_counted.h"
 #include "base/time/time.h"
 #include "net/cert/ct_verifier.h"
 #include "net/cert/signed_tree_head.h"
 #include "net/cert/sth_observer.h"
 
 namespace net {
 class CTLogVerifier;
 class X509Certificate;
 
 namespace ct {
 struct SignedCertificateTimestamp;
 }  // namespace ct
 
 }  // namespace net
 
 namespace certificate_transparency {
 
+namespace internal {
+extern const char kCanCheckForInclusionHistogramName[];

[Ryan Sleevi, 2016/07/20 20:34:00]

This does feel unnecessary, and it doesn't sound like Alexei was requiring it,
so I'd like to push back on this a little bit.

While I agree it's coupling a unittest to an implementation detail, my main
concerns are:

Putting it into an internal namespace still preserves global symbol visibility.
That is, it doesn't allow the compiler to help protect you from any mistakes.
While a static class member (private, with the unit test friended) would be
better for this, in that it provides a better guarantee (through
compiler-enforce visibility), the fact remains that if the unittest looks at
this histogram, it still remains that the unittest is testing the implementation
details of the class.

As such, I don't think there's much of a line between embedding the histogram
name itself in the test, without exposing any symbol, as you're still testing
implementation details. Arguably, by duplicating the symbol name, you're making
it clearer the contract of the unit tests expects a particular histogram named a
particular way to be set.

That's my longer justification for why I would say to duplicate in the test.

At the risk of code golfing, which may be an anti-pattern, it does seem the
duplication is the dominant form.

[Eran Messeri, 2016/07/21 17:53:43]

Done, duplicated in the test.

+}
+
 // Tracks the state of an individual Certificate Transparency Log's Merkle Tree.
 // A CT Log constantly issues Signed Tree Heads, for which every older STH must
 // be incorporated into the current/newer STH. As new certificates are logged,
 // new SCTs are produced, and eventually, those SCTs are incorporated into the
 // log and a new STH is produced, with there being an inclusion proof between
 // the SCTs and the new STH, and a consistency proof between the old STH and the
 // new STH.
 // This class receives STHs provided by/observed by the embedder, with the
 // assumption that STHs have been checked for consistency already. As SCTs are
 // observed, their status is checked against the latest STH to ensure they were
@@ skipping 68 matching lines @@
   // whole MerkleTreeLeaf (RFC6962, section 3.4.) as a key. See
   // https://crbug.com/506227#c22 and https://crbug.com/613495
   std::map<base::Time, SCTInclusionStatus> entries_status_;
 
   DISALLOW_COPY_AND_ASSIGN(SingleTreeTracker);
 };
 
 }  // namespace certificate_transparency
 
 #endif  // COMPONENTS_CERTIFICATE_TRANSPARENCY_SINGLE_TREE_TRACKER_H_