From 4aa25269cda1175d6cbea5cc93892572d53eb4b4 Mon Sep 17 00:00:00 2001 From: manukh Date: Fri, 7 May 2021 20:45:58 +0000 Subject: [PATCH] [memories] Use `base::FeatureParam`s and add persist params. - Replace the param getters & char literals with `base::FeatureParam`s. - Add "MemoriesMaxDaysToCluster" and "MemoriesMaxDaysToCluster" params to be used in a followup CL persisting clusters. - Renames the "MemoriesStoreVisitsInHistoryDb" param to "MemoriesPersistContextAnnotationsInHistoryDb" to more precisely reflect what's being persisted and allow us to later add a similar param for content annotations. - Rename the helper `RemoteModelEndpointForDebugging()` to `RemoteModelEndpoint()` to be consistent with its param name & value. - Rename the helper `ExperimentNameForRemoteModelEndpoint()` to param `kRemoteModelEndpointExperimentName` to be consistent with its param value. - Remove the helper `DebugLoggingEnabled()` and always enable its effects, since it was effectively always enabled. Bug: 1184879, 1171352 Change-Id: I755c81e34a69b1f1becbd2964b15aabcc3673b51 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2869831 Commit-Queue: manuk hovanesian Reviewed-by: Tommy Li Cr-Commit-Position: refs/heads/master@{#880543} --- .../ui/webui/memories/memories_handler.cc | 2 +- .../core/memories_features.cc | 45 +++++++++---------- .../history_clusters/core/memories_features.h | 45 ++++++++++++++----- .../core/memories_remote_model_helper.cc | 4 +- .../history_clusters/core/memories_service.cc | 18 +++----- .../history_clusters/core/memories_service.h | 7 ++- .../core/memories_service_unittest.cc | 4 +- 7 files changed, 68 insertions(+), 57 deletions(-) diff --git a/chrome/browser/ui/webui/memories/memories_handler.cc b/chrome/browser/ui/webui/memories/memories_handler.cc index 4fc0f664786e36..444438cff97de8 100644 --- a/chrome/browser/ui/webui/memories/memories_handler.cc +++ b/chrome/browser/ui/webui/memories/memories_handler.cc @@ -91,7 +91,7 @@ void MemoriesHandler::QueryMemories( auto result_callback = base::BindOnce(&MemoriesHandler::OnMemoriesQueryResult, weak_ptr_factory_.GetWeakPtr(), std::move(result_mojom)); - if (history_clusters::RemoteModelEndpointForDebugging().is_valid()) { + if (history_clusters::RemoteModelEndpoint().is_valid()) { // Cancel pending queries, if any. query_task_tracker_.TryCancelAll(); auto* memory_service = diff --git a/components/history_clusters/core/memories_features.cc b/components/history_clusters/core/memories_features.cc index 044a56334b0759..8824f1a3ad7d12 100644 --- a/components/history_clusters/core/memories_features.cc +++ b/components/history_clusters/core/memories_features.cc @@ -8,41 +8,36 @@ namespace history_clusters { -GURL RemoteModelEndpointForDebugging() { - return GURL(base::GetFieldTrialParamValueByFeature( - kRemoteModelForDebugging, "MemoriesRemoteModelEndpoint")); -} +namespace { -std::string ExperimentNameForRemoteModelEndpoint() { - return base::GetFieldTrialParamValueByFeature( - kRemoteModelForDebugging, "MemoriesRemoteModelEndpointExperimentName"); -} +const base::FeatureParam kRemoteModelEndpoint{ + &kRemoteModelForDebugging, "MemoriesRemoteModelEndpoint", ""}; -bool DebugLoggingEnabled() { - // Do debug logging if the kDebug feature is enabled. Additionally, remote - // endpoint for debugging users are implied to be in debug mode. - return base::FeatureList::IsEnabled(kDebug) || - RemoteModelEndpointForDebugging().is_valid(); -} +} // namespace -bool StoreVisitsInHistoryDb() { - return base::GetFieldTrialParamByFeatureAsBool( - kMemories, "MemoriesStoreVisitsInHistoryDb", false); +GURL RemoteModelEndpoint() { + return GURL(kRemoteModelEndpoint.Get()); } -int MaxVisitsToCluster() { - return base::GetFieldTrialParamByFeatureAsInt( - kMemories, "MemoriesMaxVisitsToCluster", 10); -} +const base::FeatureParam kRemoteModelEndpointExperimentName{ + &kRemoteModelForDebugging, "MemoriesRemoteModelEndpointExperimentName", ""}; + +const base::FeatureParam kPersistContextAnnotationsInHistoryDb{ + &kMemories, "MemoriesPersistContextAnnotationsInHistoryDb", false}; + +const base::FeatureParam kMaxVisitsToCluster{ + &kMemories, "MemoriesMaxVisitsToCluster", 10}; + +const base::FeatureParam kMaxDaysToCluster{&kMemories, + "MemoriesMaxDaysToCluster", 9}; + +const base::FeatureParam kPersistClustersInHistoryDb{ + &kMemories, "MemoriesPersistClustersInHistoryDb", false}; -// Enables the Chrome Memories history clustering feature. const base::Feature kMemories{"Memories", base::FEATURE_DISABLED_BY_DEFAULT}; -// Enables debug info; e.g. shows visit metadata on chrome://history entries. const base::Feature kDebug{"MemoriesDebug", base::FEATURE_DISABLED_BY_DEFAULT}; -// Enables using a remote model endpoint for Memories clustering for debugging -// purposes. This should not be ever enabled in production. const base::Feature kRemoteModelForDebugging{"MemoriesRemoteModelForDebugging", base::FEATURE_DISABLED_BY_DEFAULT}; diff --git a/components/history_clusters/core/memories_features.h b/components/history_clusters/core/memories_features.h index 4f2f0ba99f30e5..9b82e781a6f360 100644 --- a/components/history_clusters/core/memories_features.h +++ b/components/history_clusters/core/memories_features.h @@ -6,34 +6,57 @@ #define COMPONENTS_HISTORY_CLUSTERS_CORE_MEMORIES_FEATURES_H_ #include "base/feature_list.h" +#include "base/metrics/field_trial_params.h" #include "url/gurl.h" namespace history_clusters { +// Params & helpers functions + // Returns the remote model debug endpoint used to cluster visits into memories. // Returns an empty GURL() when the remote model debug endpoint is disabled. -GURL RemoteModelEndpointForDebugging(); +GURL RemoteModelEndpoint(); // Returns the experiment name to pass through to the remote model debug // endpoint to control how the visits get clustered. Returns an empty string if // this client should just use be returned the default clustering or if the // remote model debug endpoint is disabled. -std::string ExperimentNameForRemoteModelEndpoint(); - -// Returns true if debug logs should be generated and shown in WebUI inspector. -bool DebugLoggingEnabled(); +extern const base::FeatureParam kRemoteModelEndpointExperimentName; -// If enabled, completed visits are persisted to the history DB and read back -// when clustering. If disabled, completed visits are kept in-memory and used -// these in-memory visits are used when clustering. -bool StoreVisitsInHistoryDb(); +// If enabled, completed visits context annotations are persisted to the history +// DB and read back when clustering. If disabled, completed visit context +// annotations are kept in-memory and used these in-memory visits are used when +// clustering. +extern const base::FeatureParam kPersistContextAnnotationsInHistoryDb; // The max number of visits to use for each clustering iteration. When using the -// remote model, this limits the number of visits sent. -int MaxVisitsToCluster(); +// remote model, this limits the number of visits sent. Only applies when using +// persisted visit context annotations; i.e. +// `kPersistContextAnnotationsInHistoryDb` is true. +extern const base::FeatureParam kMaxVisitsToCluster; + +// The recency threshold controlling which visits will be clustered. This isn't +// the only factor; i.e. visits older than `MaxDaysToCluster()` may still be +// clustered. Only applies when using persisted visit context annotations; i.e. +// `kPersistContextAnnotationsInHistoryDb` is true. +extern const base::FeatureParam kMaxDaysToCluster; + +// If enabled, updating clusters will persist the results to the history DB and +// accessing clusters will retrieve them from the history DB. If disabled, +// updating clusters is a no-op and accessing clusters will generate and return +// new clusters without persisting them. +extern const base::FeatureParam kPersistClustersInHistoryDb; +// Features + +// Enables the Chrome Memories history clustering feature. extern const base::Feature kMemories; + +// Enables debug info; e.g. shows visit metadata on chrome://history entries. extern const base::Feature kDebug; + +// Enables using a remote model endpoint for Memories clustering for debugging +// purposes. This should not be ever enabled in production. extern const base::Feature kRemoteModelForDebugging; } // namespace history_clusters diff --git a/components/history_clusters/core/memories_remote_model_helper.cc b/components/history_clusters/core/memories_remote_model_helper.cc index 36ea28bc44f8c5..44626adc66faa7 100644 --- a/components/history_clusters/core/memories_remote_model_helper.cc +++ b/components/history_clusters/core/memories_remote_model_helper.cc @@ -28,7 +28,7 @@ proto::GetClustersRequest CreateRequestProto( const std::vector& visits, base::Optional debug_logger) { proto::GetClustersRequest request; - request.set_experiment_name(ExperimentNameForRemoteModelEndpoint()); + request.set_experiment_name(kRemoteModelEndpointExperimentName.Get()); base::ListValue debug_visits_list; for (auto& visit : visits) { @@ -143,7 +143,7 @@ MemoriesRemoteModelHelper::~MemoriesRemoteModelHelper() = default; void MemoriesRemoteModelHelper::GetMemories( MemoriesCallback callback, const std::vector& visits) { - const GURL endpoint(RemoteModelEndpointForDebugging()); + const GURL endpoint(RemoteModelEndpoint()); if (!endpoint.is_valid() || visits.empty()) { std::move(callback).Run({}); return; diff --git a/components/history_clusters/core/memories_service.cc b/components/history_clusters/core/memories_service.cc index 4867fa49a262ea..e3b1958e97f1c5 100644 --- a/components/history_clusters/core/memories_service.cc +++ b/components/history_clusters/core/memories_service.cc @@ -114,14 +114,10 @@ MemoriesService::MemoriesService( history::HistoryService* history_service, scoped_refptr url_loader_factory) : history_service_(history_service) { - base::Optional debug_logger; - if (DebugLoggingEnabled()) { - debug_logger = base::BindRepeating(&MemoriesService::NotifyDebugMessage, - weak_ptr_factory_.GetWeakPtr()); - } - remote_model_helper_ = std::make_unique( - url_loader_factory, debug_logger); + url_loader_factory, + base::BindRepeating(&MemoriesService::NotifyDebugMessage, + weak_ptr_factory_.GetWeakPtr())); remote_model_helper_weak_factory_ = std::make_unique>( remote_model_helper_.get()); @@ -140,8 +136,6 @@ void MemoriesService::RemoveObserver(Observer* obs) { } void MemoriesService::NotifyDebugMessage(const std::string& message) const { - DCHECK(DebugLoggingEnabled()) << "Callers must ensure logging is enabled."; - for (Observer& obs : observers_) { obs.OnMemoriesDebugMessage(message); } @@ -175,7 +169,7 @@ void MemoriesService::CompleteVisitContextAnnotationsIfReady(int64_t nav_id) { (visit_context_annotations.status.ukm_page_end_signals || !visit_context_annotations.status.expect_ukm_page_end_signals)) { if (base::FeatureList::IsEnabled(kMemories)) { - if (StoreVisitsInHistoryDb()) + if (kPersistContextAnnotationsInHistoryDb.Get()) history_service_->AddAnnotatedVisit( {visit_context_annotations.visit_row.visit_id, visit_context_annotations.context_annotations, @@ -211,9 +205,9 @@ void MemoriesService::QueryMemories( std::move(query_params))) .Then(std::move(callback))); - if (StoreVisitsInHistoryDb()) { + if (kPersistContextAnnotationsInHistoryDb.Get()) { history_service_->GetAnnotatedVisits( - MaxVisitsToCluster(), + kMaxVisitsToCluster.Get(), base::BindOnce( // This echo callback is necessary to copy the |AnnotatedVisit| // refs. diff --git a/components/history_clusters/core/memories_service.h b/components/history_clusters/core/memories_service.h index 71b15bec082ea4..a3d3d0d42fc004 100644 --- a/components/history_clusters/core/memories_service.h +++ b/components/history_clusters/core/memories_service.h @@ -56,8 +56,7 @@ class MemoriesService : public KeyedService { void AddObserver(Observer* obs); void RemoveObserver(Observer* obs); - // Notifies the observers of a debug message being available. Caller is - // responsible for checking that logging is enabled before calling this. + // Notifies the observers of a debug message being available. void NotifyDebugMessage(const std::string& message) const; // TODO(manukh) |MemoriesService| should be responsible for constructing the @@ -97,8 +96,8 @@ class MemoriesService : public KeyedService { friend class MemoriesServiceTestApi; // If the Memories flag is enabled, this contains all the visits in-memory - // during the Profile lifetime. If the "MemoriesStoreVisitsInHistoryDb" param - // is true, this will be empty. + // during the Profile lifetime. If the `kPersistContextAnnotationsInHistoryDb` + // param is true, this will be empty. // TODO(tommycli): Hide this better behind a new debug flag. std::vector visits_; diff --git a/components/history_clusters/core/memories_service_unittest.cc b/components/history_clusters/core/memories_service_unittest.cc index e765590b41a8ae..d0ccf982ae7afd 100644 --- a/components/history_clusters/core/memories_service_unittest.cc +++ b/components/history_clusters/core/memories_service_unittest.cc @@ -548,7 +548,7 @@ TEST_F(MemoriesServiceTest, QueryMemoriesWithHistoryDb) { { { kMemories, - {{"MemoriesStoreVisitsInHistoryDb", "true"}}, + {{"MemoriesPersistContextAnnotationsInHistoryDb", "true"}}, }, { kRemoteModelForDebugging, @@ -617,7 +617,7 @@ TEST_F(MemoriesServiceTest, QueryMemoriesWithHistoryDbWithPendingRequest) { { { kMemories, - {{"MemoriesStoreVisitsInHistoryDb", "true"}}, + {{"MemoriesPersistContextAnnotationsInHistoryDb", "true"}}, }, { kRemoteModelForDebugging,