When network time is unavailable, record the reason in UMA

components/network_time/network_time_tracker.cc

 // Copyright 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 "components/network_time/network_time_tracker.h"
 
 #include <stdint.h>
 #include <string>
 #include <utility>
 
@@ skipping 288 matching lines @@
 void NetworkTimeTracker::WaitForFetchForTesting(uint32_t nonce) {
   query_signer_->OverrideNonceForTesting(kKeyVersion, nonce);
   base::RunLoop().Run();
 }
 
 base::TimeDelta NetworkTimeTracker::GetTimerDelayForTesting() const {
   DCHECK(timer_.IsRunning());
   return timer_.GetCurrentDelay();
 }
 
-bool NetworkTimeTracker::GetNetworkTime(base::Time* network_time,
-                                        base::TimeDelta* uncertainty) const {
+NetworkTimeTracker::NetworkTimeResult NetworkTimeTracker::GetNetworkTime(
+    base::Time* network_time,
+    base::TimeDelta* uncertainty) const {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(network_time);
   if (network_time_at_last_measurement_.is_null()) {
-    return false;
+    return NETWORK_TIME_NO_SYNC;
   }
   DCHECK(!ticks_at_last_measurement_.is_null());
   DCHECK(!time_at_last_measurement_.is_null());
   base::TimeDelta tick_delta =
       tick_clock_->NowTicks() - ticks_at_last_measurement_;
   base::TimeDelta time_delta = clock_->Now() - time_at_last_measurement_;
   if (time_delta.InMilliseconds() < 0) {  // Has wall clock run backward?
     DVLOG(1) << "Discarding network time due to wall clock running backward";
     network_time_at_last_measurement_ = base::Time();
-    return false;
+    return NETWORK_TIME_SYNC_LOST;

[mab, 2016/08/19 01:51:08]

Do you not want to record the new histogram in this case as well?

[estark, 2016/08/19 11:51:08]

I think it would be clearest to have this be yet another histogram.
NetworkTimeTracker.ClockDivergence is the tick delta compared to the time delta,
whereas this is the wall clock compared to itself. Hmm, guess I might as well
add that one as well.

[mab, 2016/08/20 00:11:05]

Your comment makes me wonder if it's worth distinguishing the two cases of
NETWORK_TIME_SYNC_LOST.  You could even distinguish 3 cases, if you wanted:

- wall clock ran backwards
- wall clock is ahead of tick clock
- wall clock is behind tick clock

Though maybe that's redundant enough with the histograms that it's not worth
bothering.

Totally up to you, of course, but it seems like you're aiming for the
instrumentation to end instrumentation, so I thought I'd mention it. :-)

   }
   // Now we know that both |tick_delta| and |time_delta| are positive.
-  base::TimeDelta divergence = (tick_delta - time_delta).magnitude();
-  if (divergence > base::TimeDelta::FromSeconds(kClockDivergenceSeconds)) {
+  base::TimeDelta divergence = tick_delta - time_delta;
+  if (divergence.magnitude() >
+      base::TimeDelta::FromSeconds(kClockDivergenceSeconds)) {
     // Most likely either the machine has suspended, or the wall clock has been
     // reset.
     DVLOG(1) << "Discarding network time due to clocks diverging";
+    UMA_HISTOGRAM_SPARSE_SLOWLY("NetworkTimeTracker.ClockDivergence",
+                                divergence.InSeconds());

[mab, 2016/08/19 01:51:07]

Is this always positive?  It would be nice to get a sign as well as a magnitude,
so we know which clock is ahead of the other.

[estark, 2016/08/19 11:51:08]

This should be signed. My bad for not writing a test to make it obvious. Added
one now.

     network_time_at_last_measurement_ = base::Time();
-    return false;
+    return NETWORK_TIME_SYNC_LOST;
   }
   *network_time = network_time_at_last_measurement_ + tick_delta;
   if (uncertainty) {
     *uncertainty = network_time_uncertainty_ + divergence;
   }
-  return true;
+  return NETWORK_TIME_AVAILABLE;
 }
 
 void NetworkTimeTracker::CheckTime() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // If NetworkTimeTracker is waking up after a backoff, this will reset the
   // timer to its default faster frequency.
   QueueCheckTime(CheckTimeInterval());
 
   if (!ShouldIssueTimeQuery()) {
@@ skipping 112 matching lines @@
 void NetworkTimeTracker::QueueCheckTime(base::TimeDelta delay) {
   timer_.Start(FROM_HERE, delay, this, &NetworkTimeTracker::CheckTime);
 }
 
 bool NetworkTimeTracker::ShouldIssueTimeQuery() {
   // Do not query the time service if not enabled via Variations Service.
   if (!base::FeatureList::IsEnabled(kNetworkTimeServiceQuerying)) {
     return false;
   }
 
-  // If GetNetworkTime() returns false, synchronization has been lost
-  // and a query is needed.
+  // If GetNetworkTime() does not return NETWORK_TIME_AVAILABLE,
+  // synchronization has been lost and a query is needed.
   base::Time network_time;
-  if (!GetNetworkTime(&network_time, nullptr)) {
+  if (GetNetworkTime(&network_time, nullptr) != NETWORK_TIME_AVAILABLE) {
     return true;
   }
 
   // Otherwise, make the decision at random.
   return base::RandDouble() < RandomQueryProbability();
 }
 
 }  // namespace network_time