When network time is unavailable, record the reason in UMA

components/network_time/network_time_tracker.cc

Index: components/network_time/network_time_tracker.cc
diff --git a/components/network_time/network_time_tracker.cc b/components/network_time/network_time_tracker.cc
index 5ce5e24a42cc4b252e7c9af84a7eaf725f352498..d4ab737ed5be0053e4de6d9d91c2db9baf10b91d 100644
--- a/components/network_time/network_time_tracker.cc
+++ b/components/network_time/network_time_tracker.cc
@@ -306,12 +306,13 @@ base::TimeDelta NetworkTimeTracker::GetTimerDelayForTesting() const {
   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());
@@ -321,22 +322,25 @@ bool NetworkTimeTracker::GetNetworkTime(base::Time* network_time,
   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() {
@@ -469,10 +473,10 @@ bool NetworkTimeTracker::ShouldIssueTimeQuery() {
     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;
   }