Log UMA metrics with NTP tile data age/staleness
Most notably, we want to capture this metric for server-side suggestions
in order to detect regressions in upcoming UI changes (Chrome Home).
Bug: 763946
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I4cb7e6cea6026ad33182f70d7434ab59f7fcb0af
Reviewed-on: https://chromium-review.googlesource.com/708736
Reviewed-by: Alexei Svitkine <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Reviewed-by: Marc Treib <[email protected]>
Reviewed-by: Sylvain Defresne <[email protected]>
Reviewed-by: Peter Conn <[email protected]>
Commit-Queue: Mikel Astiz <[email protected]>
Cr-Commit-Position: refs/heads/master@{#508391}diff --git a/components/ntp_tiles/metrics.cc b/components/ntp_tiles/metrics.cc
index 0723c3de..f292b97 100644
--- a/components/ntp_tiles/metrics.cc
+++ b/components/ntp_tiles/metrics.cc
@@ -6,7 +6,7 @@
#include <string>
-#include "base/metrics/histogram.h"
+#include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h"
#include "base/metrics/sparse_histogram.h"
#include "base/strings/stringprintf.h"
@@ -37,17 +37,10 @@
const char kTileTypeSuffixThumbnail[] = "Thumbnail";
const char kTileTypeSuffixThumbnailFailed[] = "ThumbnailFailed";
-// Log an event for a given |histogram| at a given element |position|. This
-// routine exists because regular histogram macros are cached thus can't be used
-// if the name of the histogram will change at a given call site.
-void LogHistogramEvent(const std::string& histogram,
- int position,
- int num_sites) {
- base::HistogramBase* counter = base::LinearHistogram::FactoryGet(
- histogram, 1, num_sites, num_sites + 1,
- base::Histogram::kUmaTargetedHistogramFlag);
- if (counter)
- counter->Add(position);
+void LogUmaHistogramAge(const std::string& name, const base::TimeDelta& value) {
+ // Log the value in number of seconds.
+ base::UmaHistogramCustomCounts(name, value.InSeconds(), 5,
+ base::TimeDelta::FromDays(14).InSeconds(), 20);
}
std::string GetSourceHistogramName(TileSource source) {
@@ -100,19 +93,28 @@
impression.index, kMaxNumTiles);
std::string source_name = GetSourceHistogramName(impression.source);
- std::string impression_histogram = base::StringPrintf(
- "NewTabPage.SuggestionsImpression.%s", source_name.c_str());
- LogHistogramEvent(impression_histogram, impression.index, kMaxNumTiles);
+ base::UmaHistogramExactLinear(
+ base::StringPrintf("NewTabPage.SuggestionsImpression.%s",
+ source_name.c_str()),
+ impression.index, kMaxNumTiles);
+
+ if (!impression.data_generation_time.is_null()) {
+ const base::TimeDelta age =
+ base::Time::Now() - impression.data_generation_time;
+ LogUmaHistogramAge("NewTabPage.SuggestionsImpressionAge", age);
+ LogUmaHistogramAge(
+ base::StringPrintf("NewTabPage.SuggestionsImpressionAge.%s",
+ source_name.c_str()),
+ age);
+ }
UMA_HISTOGRAM_ENUMERATION("NewTabPage.TileTitle",
static_cast<int>(impression.title_source),
kLastTitleSource + 1);
- std::string title_source_histogram =
+ base::UmaHistogramExactLinear(
base::StringPrintf("NewTabPage.TileTitle.%s",
- GetSourceHistogramName(impression.source).c_str());
- LogHistogramEvent(title_source_histogram,
- static_cast<int>(impression.title_source),
- kLastTitleSource + 1);
+ GetSourceHistogramName(impression.source).c_str()),
+ static_cast<int>(impression.title_source), kLastTitleSource + 1);
if (impression.visual_type > LAST_RECORDED_TILE_TYPE) {
return;
@@ -121,10 +123,9 @@
UMA_HISTOGRAM_ENUMERATION("NewTabPage.TileType", impression.visual_type,
LAST_RECORDED_TILE_TYPE + 1);
- std::string tile_type_histogram =
- base::StringPrintf("NewTabPage.TileType.%s", source_name.c_str());
- LogHistogramEvent(tile_type_histogram, impression.visual_type,
- LAST_RECORDED_TILE_TYPE + 1);
+ base::UmaHistogramExactLinear(
+ base::StringPrintf("NewTabPage.TileType.%s", source_name.c_str()),
+ impression.visual_type, LAST_RECORDED_TILE_TYPE + 1);
const char* tile_type_suffix = GetTileTypeSuffix(impression.visual_type);
if (tile_type_suffix) {
@@ -136,10 +137,10 @@
impression.url_for_rappor);
}
- std::string icon_impression_histogram = base::StringPrintf(
- "NewTabPage.SuggestionsImpression.%s", tile_type_suffix);
- LogHistogramEvent(icon_impression_histogram, impression.index,
- kMaxNumTiles);
+ base::UmaHistogramExactLinear(
+ base::StringPrintf("NewTabPage.SuggestionsImpression.%s",
+ tile_type_suffix),
+ impression.index, kMaxNumTiles);
}
}
@@ -147,37 +148,44 @@
UMA_HISTOGRAM_ENUMERATION("NewTabPage.MostVisited", impression.index,
kMaxNumTiles);
- std::string histogram =
- base::StringPrintf("NewTabPage.MostVisited.%s",
- GetSourceHistogramName(impression.source).c_str());
- LogHistogramEvent(histogram, impression.index, kMaxNumTiles);
+ std::string source_name = GetSourceHistogramName(impression.source);
+ base::UmaHistogramExactLinear(
+ base::StringPrintf("NewTabPage.MostVisited.%s", source_name.c_str()),
+ impression.index, kMaxNumTiles);
+
+ if (!impression.data_generation_time.is_null()) {
+ const base::TimeDelta age =
+ base::Time::Now() - impression.data_generation_time;
+ LogUmaHistogramAge("NewTabPage.MostVisitedAge", age);
+ LogUmaHistogramAge(
+ base::StringPrintf("NewTabPage.MostVisitedAge.%s", source_name.c_str()),
+ age);
+ }
const char* tile_type_suffix = GetTileTypeSuffix(impression.visual_type);
if (tile_type_suffix) {
- std::string tile_type_histogram =
- base::StringPrintf("NewTabPage.MostVisited.%s", tile_type_suffix);
- LogHistogramEvent(tile_type_histogram, impression.index, kMaxNumTiles);
+ base::UmaHistogramExactLinear(
+ base::StringPrintf("NewTabPage.MostVisited.%s", tile_type_suffix),
+ impression.index, kMaxNumTiles);
}
UMA_HISTOGRAM_ENUMERATION("NewTabPage.TileTitleClicked",
static_cast<int>(impression.title_source),
kLastTitleSource + 1);
- std::string name_histogram =
+ base::UmaHistogramExactLinear(
base::StringPrintf("NewTabPage.TileTitleClicked.%s",
- GetSourceHistogramName(impression.source).c_str());
- LogHistogramEvent(name_histogram, static_cast<int>(impression.title_source),
- kLastTitleSource + 1);
+ GetSourceHistogramName(impression.source).c_str()),
+ static_cast<int>(impression.title_source), kLastTitleSource + 1);
if (impression.visual_type <= LAST_RECORDED_TILE_TYPE) {
UMA_HISTOGRAM_ENUMERATION("NewTabPage.TileTypeClicked",
impression.visual_type,
LAST_RECORDED_TILE_TYPE + 1);
- std::string type_histogram =
+ base::UmaHistogramExactLinear(
base::StringPrintf("NewTabPage.TileTypeClicked.%s",
- GetSourceHistogramName(impression.source).c_str());
- LogHistogramEvent(type_histogram, impression.visual_type,
- LAST_RECORDED_TILE_TYPE + 1);
+ GetSourceHistogramName(impression.source).c_str()),
+ impression.visual_type, LAST_RECORDED_TILE_TYPE + 1);
}
}
diff --git a/components/ntp_tiles/metrics_unittest.cc b/components/ntp_tiles/metrics_unittest.cc
index cafd8e84..6fc5cb6 100644
--- a/components/ntp_tiles/metrics_unittest.cc
+++ b/components/ntp_tiles/metrics_unittest.cc
@@ -31,15 +31,14 @@
using testing::ElementsAre;
using testing::IsEmpty;
+MATCHER_P3(IsBucketBetween, lower_bound, upper_bound, count, "") {
+ return arg.min >= lower_bound && arg.min <= upper_bound && arg.count == count;
+}
+
// Builder for instances of NTPTileImpression that uses sensible defaults.
class Builder {
public:
- Builder()
- : impression_(/*index=*/0,
- TileSource::TOP_SITES,
- TileTitleSource::UNKNOWN,
- TileVisualType::UNKNOWN_TILE_TYPE,
- GURL()) {}
+ Builder() {}
Builder& WithIndex(int index) {
impression_.index = index;
@@ -57,6 +56,10 @@
impression_.visual_type = visual_type;
return *this;
}
+ Builder& WithDataGenerationTime(base::Time data_generation_time) {
+ impression_.data_generation_time = data_generation_time;
+ return *this;
+ }
Builder& WithUrl(const GURL& url) {
impression_.url_for_rappor = url;
return *this;
@@ -291,6 +294,31 @@
base::Bucket(kInferredTitleSource, /*count=*/1)));
}
+TEST(RecordTileImpressionTest, ShouldRecordAge) {
+ const base::TimeDelta kSuggestionAge = base::TimeDelta::FromMinutes(1);
+ const base::TimeDelta kBucketTolerance = base::TimeDelta::FromSeconds(20);
+ base::HistogramTester histogram_tester;
+ RecordTileImpression(
+ Builder()
+ .WithSource(TileSource::SUGGESTIONS_SERVICE)
+ .WithDataGenerationTime(base::Time::Now() - kSuggestionAge)
+ .Build(),
+ /*rappor_service=*/nullptr);
+
+ EXPECT_THAT(
+ histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpressionAge"),
+ ElementsAre(
+ IsBucketBetween((kSuggestionAge - kBucketTolerance).InSeconds(),
+ (kSuggestionAge + kBucketTolerance).InSeconds(),
+ /*count=*/1)));
+ EXPECT_THAT(histogram_tester.GetAllSamples(
+ "NewTabPage.SuggestionsImpressionAge.server"),
+ ElementsAre(IsBucketBetween(
+ (kSuggestionAge - kBucketTolerance).InSeconds(),
+ (kSuggestionAge + kBucketTolerance).InSeconds(),
+ /*count=*/1)));
+}
+
TEST(RecordTileClickTest, ShouldRecordUmaForIcon) {
base::HistogramTester histogram_tester;
RecordTileClick(Builder()
@@ -505,6 +533,29 @@
base::Bucket(kTitleTagTitleSource, /*count=*/2)));
}
+TEST(RecordTileClickTest, ShouldRecordClickAge) {
+ const base::TimeDelta kSuggestionAge = base::TimeDelta::FromMinutes(1);
+ const base::TimeDelta kBucketTolerance = base::TimeDelta::FromSeconds(20);
+ base::HistogramTester histogram_tester;
+ RecordTileClick(
+ Builder()
+ .WithSource(TileSource::SUGGESTIONS_SERVICE)
+ .WithDataGenerationTime(base::Time::Now() - kSuggestionAge)
+ .Build());
+
+ EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.MostVisitedAge"),
+ ElementsAre(IsBucketBetween(
+ (kSuggestionAge - kBucketTolerance).InSeconds(),
+ (kSuggestionAge + kBucketTolerance).InSeconds(),
+ /*count=*/1)));
+ EXPECT_THAT(
+ histogram_tester.GetAllSamples("NewTabPage.MostVisitedAge.server"),
+ ElementsAre(
+ IsBucketBetween((kSuggestionAge - kBucketTolerance).InSeconds(),
+ (kSuggestionAge + kBucketTolerance).InSeconds(),
+ /*count=*/1)));
+}
+
} // namespace
} // namespace metrics
} // namespace ntp_tiles
diff --git a/components/ntp_tiles/most_visited_sites.cc b/components/ntp_tiles/most_visited_sites.cc
index a6fd4753..8a564fc 100644
--- a/components/ntp_tiles/most_visited_sites.cc
+++ b/components/ntp_tiles/most_visited_sites.cc
@@ -294,7 +294,8 @@
// MostVisitedURL.title is either the title or the URL which is treated
// exactly as the title. Differentiating here is not worth the overhead.
tile.title_source = TileTitleSource::TITLE_TAG;
-
+ // TODO(crbug.com/773278): Populate |data_generation_time| here in order to
+ // log UMA metrics of age.
tiles.push_back(std::move(tile));
}
@@ -331,6 +332,10 @@
if (num_sites_ < num_tiles)
num_tiles = num_sites_;
+ const base::Time profile_timestamp =
+ base::Time::UnixEpoch() +
+ base::TimeDelta::FromMicroseconds(suggestions_profile.timestamp());
+
NTPTilesVector tiles;
for (size_t i = 0; i < num_tiles; ++i) {
const ChromeSuggestion& suggestion_pb = suggestions_profile.suggestions(i);
@@ -347,6 +352,8 @@
tile.whitelist_icon_path = GetWhitelistLargeIconPath(url);
tile.thumbnail_url = GURL(suggestion_pb.thumbnail());
tile.favicon_url = GURL(suggestion_pb.favicon_url());
+ tile.data_generation_time = profile_timestamp;
+
if (AreNtpMostLikelyFaviconsFromServerEnabled()) {
icon_cacher_->StartFetchMostLikely(
url, base::Bind(&MostVisitedSites::OnIconMadeAvailable,
diff --git a/components/ntp_tiles/ntp_tile.h b/components/ntp_tiles/ntp_tile.h
index 4dc012c1..ea6d863 100644
--- a/components/ntp_tiles/ntp_tile.h
+++ b/components/ntp_tiles/ntp_tile.h
@@ -10,6 +10,7 @@
#include "base/files/file_path.h"
#include "base/macros.h"
#include "base/strings/string16.h"
+#include "base/time/time.h"
#include "components/ntp_tiles/tile_source.h"
#include "components/ntp_tiles/tile_title_source.h"
#include "url/gurl.h"
@@ -34,6 +35,10 @@
// This won't be empty, but might 404 etc.
GURL favicon_url;
+ // Timestamp representing when the tile was originally generated (produced by
+ // a ranking algorithm).
+ base::Time data_generation_time;
+
NTPTile();
NTPTile(const NTPTile&);
~NTPTile();
diff --git a/components/ntp_tiles/ntp_tile_impression.cc b/components/ntp_tiles/ntp_tile_impression.cc
index 20194940..325bb21 100644
--- a/components/ntp_tiles/ntp_tile_impression.cc
+++ b/components/ntp_tiles/ntp_tile_impression.cc
@@ -11,17 +11,20 @@
/*source=*/TileSource::TOP_SITES,
/*title_source=*/TileTitleSource::UNKNOWN,
/*visual_type=*/TileVisualType::UNKNOWN_TILE_TYPE,
+ /*data_generation_time=*/base::Time(),
/*url_for_rappor=*/GURL()) {}
NTPTileImpression::NTPTileImpression(int index,
TileSource source,
TileTitleSource title_source,
TileVisualType visual_type,
+ base::Time data_generation_time,
const GURL& url_for_rappor)
: index(index),
source(source),
title_source(title_source),
visual_type(visual_type),
+ data_generation_time(data_generation_time),
url_for_rappor(url_for_rappor) {}
NTPTileImpression::~NTPTileImpression() {}
diff --git a/components/ntp_tiles/ntp_tile_impression.h b/components/ntp_tiles/ntp_tile_impression.h
index 2384ccc8..586151b 100644
--- a/components/ntp_tiles/ntp_tile_impression.h
+++ b/components/ntp_tiles/ntp_tile_impression.h
@@ -5,6 +5,7 @@
#ifndef COMPONENTS_NTP_TILES_NTP_TILE_IMPRESSION_H_
#define COMPONENTS_NTP_TILES_NTP_TILE_IMPRESSION_H_
+#include "base/time/time.h"
#include "components/ntp_tiles/tile_source.h"
#include "components/ntp_tiles/tile_title_source.h"
#include "components/ntp_tiles/tile_visual_type.h"
@@ -19,6 +20,7 @@
TileSource source,
TileTitleSource title_source,
TileVisualType visual_type,
+ base::Time data_generation_time,
const GURL& url_for_rappor);
~NTPTileImpression();
@@ -27,6 +29,10 @@
TileSource source;
TileTitleSource title_source;
TileVisualType visual_type;
+ // The timestamp representing when the tile data (e.g. URL) was generated
+ // originally, regardless of the impression timestamp or the time when it
+ // was fetched (for server-side suggestions).
+ base::Time data_generation_time;
// URL the tile points to, used to report Rappor metrics only (might be empty
// and is hence ignored, e.g. on desktop).
GURL url_for_rappor;
diff --git a/components/suggestions/blacklist_store.cc b/components/suggestions/blacklist_store.cc
index 915fc28..a7d08b1a 100644
--- a/components/suggestions/blacklist_store.cc
+++ b/components/suggestions/blacklist_store.cc
@@ -197,6 +197,7 @@
// Populate the filtered suggestions.
SuggestionsProfile filtered_profile;
+ filtered_profile.set_timestamp(profile->timestamp());
for (int i = 0; i < profile->suggestions_size(); ++i) {
if (blacklist_set.find(profile->suggestions(i).url()) ==
blacklist_set.end()) {
diff --git a/components/suggestions/blacklist_store_unittest.cc b/components/suggestions/blacklist_store_unittest.cc
index 14caf0f6..bb10e7d 100644
--- a/components/suggestions/blacklist_store_unittest.cc
+++ b/components/suggestions/blacklist_store_unittest.cc
@@ -32,6 +32,7 @@
ChromeSuggestion* suggestion = suggestions.add_suggestions();
suggestion->set_url(*it);
}
+ suggestions.set_timestamp(123);
return suggestions;
}
diff --git a/components/suggestions/suggestions_store.cc b/components/suggestions/suggestions_store.cc
index 09a1eca..002dedd 100644
--- a/components/suggestions/suggestions_store.cc
+++ b/components/suggestions/suggestions_store.cc
@@ -76,6 +76,7 @@
SuggestionsProfile filtered_suggestions;
int64_t now_usec =
(this->clock_->Now() - base::Time::UnixEpoch()).ToInternalValue();
+ filtered_suggestions.set_timestamp(suggestions->timestamp());
for (int i = 0; i < suggestions->suggestions_size(); ++i) {
ChromeSuggestion* suggestion = suggestions->mutable_suggestions(i);
diff --git a/components/suggestions/suggestions_store_unittest.cc b/components/suggestions/suggestions_store_unittest.cc
index 0a991d05..b39ad74 100644
--- a/components/suggestions/suggestions_store_unittest.cc
+++ b/components/suggestions/suggestions_store_unittest.cc
@@ -37,6 +37,7 @@
SuggestionsProfile CreateTestSuggestions() {
SuggestionsProfile suggestions;
+ suggestions.set_timestamp(123);
ChromeSuggestion* suggestion = suggestions.add_suggestions();
suggestion->set_url(kTestTitle);
suggestion->set_title(kTestUrl);
