UMA for the Stale Previews timestamp

chrome/browser/previews/previews_infobar_delegate.cc

 // 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.
 
 #include "chrome/browser/previews/previews_infobar_delegate.h"
 
 #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"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/android/android_theme_resources.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h"
 #include "chrome/browser/previews/previews_infobar_tab_helper.h"
@@ skipping 25 matching lines @@
   int32_t max_limit =
       static_cast<int32_t>(PreviewsInfoBarDelegate::INFOBAR_INDEX_BOUNDARY);
   base::LinearHistogram::FactoryGet(
       base::StringPrintf("Previews.InfoBarAction.%s",
                          GetStringNameForType(previews_type).c_str()),
       1, max_limit, max_limit + 1,
       base::HistogramBase::kUmaTargetedHistogramFlag)
       ->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
 // pingback.
 void ReportPingbackInformation(content::WebContents* web_contents) {
   auto* data_reduction_proxy_settings =
       DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
           web_contents->GetBrowserContext());
   PreviewsInfoBarTabHelper* infobar_tab_helper =
       PreviewsInfoBarTabHelper::FromWebContents(web_contents);
@@ skipping 184 matching lines @@
     return base::string16();
   }
 
   int min_staleness_in_minutes = base::GetFieldTrialParamByFeatureAsInt(
       previews::features::kStalePreviewsTimestamp, kMinStalenessParamName,
       kMinStalenessParamDefaultValue);
   int max_staleness_in_minutes = base::GetFieldTrialParamByFeatureAsInt(
       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,
                                                                 nullptr) !=
       network_time::NetworkTimeTracker::NETWORK_TIME_AVAILABLE) {
     // When network time has not been initialized yet, simply rely on the
     // machine's current time.
     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(
         IDS_PREVIEWS_INFOBAR_TIMESTAMP_MINUTES,
         base::IntToString16(staleness_in_minutes));
   } else if (staleness_in_minutes < 120) {
     return l10n_util::GetStringUTF16(IDS_PREVIEWS_INFOBAR_TIMESTAMP_ONE_HOUR);
   } else {
     return l10n_util::GetStringFUTF16(
         IDS_PREVIEWS_INFOBAR_TIMESTAMP_HOURS,
         base::IntToString16(staleness_in_minutes / 60));
   }
 }