UMA for the Stale Previews timestamp

chrome/browser/previews/previews_infobar_delegate.cc

Index: chrome/browser/previews/previews_infobar_delegate.cc
diff --git a/chrome/browser/previews/previews_infobar_delegate.cc b/chrome/browser/previews/previews_infobar_delegate.cc
index ec43a5275189bea82969c9002f03e323ff549273..bf01a88a6558342b149da7a455436944b50593de 100644
--- a/chrome/browser/previews/previews_infobar_delegate.cc
+++ b/chrome/browser/previews/previews_infobar_delegate.cc
@@ -7,6 +7,7 @@
 #include "base/feature_list.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
@@ -52,6 +53,11 @@ void RecordPreviewsInfoBarAction(
       ->Add(static_cast<int32_t>(action));
 }
 
+void RecordStaleness(PreviewsInfoBarDelegate::PreviewsInfoBarTimestamp value) {
+  UMA_HISTOGRAM_ENUMERATION("Previews.InfoBarTimestamp", value,
+                            PreviewsInfoBarDelegate::TIMESTAMP_INDEX_BOUNDARY);
+}
+
 // Sends opt out information to the pingback service based on a key value in the
 // infobar tab helper. Sending this information in the case of a navigation that
 // should not send a pingback (or is not a server preview) will not alter the
@@ -256,8 +262,10 @@ base::string16 PreviewsInfoBarDelegate::GetTimestampText() const {
       previews::features::kStalePreviewsTimestamp, kMaxStalenessParamName,
       kMaxStalenessParamDefaultValue);
 
-  if (min_staleness_in_minutes <= 0 || max_staleness_in_minutes <= 0)
+  if (min_staleness_in_minutes <= 0 || max_staleness_in_minutes <= 0) {
+    RecordStaleness(TIMESTAMP_NOT_SHOWN_PARAMS_NOT_AVAILABLE);

[tbansal1, 2017/06/22 21:12:16]

Can this be changed to:
DCHECK(min_staleness_in_minutes > 0 && max_staleness_in_minutes > 0);

because if the experiment is enabled, then the config should really also be
providing the experiment params.

[megjablon, 2017/06/22 23:40:11]

Added NOTREACHED(); since we should still return on non-debug builds.

     return base::string16();
+  }
 
   base::Time network_time;
   if (g_browser_process->network_time_tracker()->GetNetworkTime(&network_time,
@@ -268,12 +276,22 @@ base::string16 PreviewsInfoBarDelegate::GetTimestampText() const {
     network_time = base::Time::Now();

[tbansal1, 2017/06/22 21:12:16]

Record separate UMA here? I guess if the time tracker has low availability, then
we need to come up with a custom solution?

[megjablon, 2017/06/22 23:40:11]

I can add that separately if we decide we need it. I'm still going to
investigate the time tracker more later, but want to get all this code in before
feature freeze.

   }
 
+  if (network_time < previews_freshness_) {
+    RecordStaleness(TIMESTAMP_NOT_SHOWN_STALENESS_NEGATIVE);
+    return base::string16();
+  }
+
   int staleness_in_minutes = (network_time - previews_freshness_).InMinutes();
-  // TODO(megjablon): record metrics for out of bounds staleness.
-  if (staleness_in_minutes < min_staleness_in_minutes)
+  if (staleness_in_minutes < min_staleness_in_minutes) {
+    RecordStaleness(TIMESTAMP_NOT_SHOWN_PREVIEW_NOT_STALE);
     return base::string16();
-  if (staleness_in_minutes > max_staleness_in_minutes)
+  }
+  if (staleness_in_minutes > max_staleness_in_minutes) {
+    RecordStaleness(TIMESTAMP_NOT_SHOWN_STALENESS_GREATER_THAN_MAX);
     return base::string16();
+  }
+
+  RecordStaleness(TIMESTAMP_SHOWN);

[tbansal1, 2017/06/22 21:12:16]

Do we want to record exactly which text (among three options) was shown?

[megjablon, 2017/06/22 23:40:11]

We can, but if we just care about the number of stale previews being seen and
how old they are, we can get those numbers from the server.

 
   if (staleness_in_minutes < 60) {
     return l10n_util::GetStringFUTF16(