|
|
DescriptionCreate a synthetic field trial for precache.
For clients in a Precache trial group who've started a precache in the last 15
days, registers them in a matching group in the PrecacheSynthetic15D trial. For
those who've started a precache in the last 1 day, registers in them in the
PrecacheSynthetic1D trial also.
BUG=663987
Review-Url: https://codereview.chromium.org/2596093002
Cr-Commit-Position: refs/heads/master@{#443352}
Committed: https://chromium.googlesource.com/chromium/src/+/9020aee19a0a9cdb845f7cb2bc7f30c799741fdf
Patch Set 1 #
Total comments: 18
Patch Set 2 : Add test, change group names, and pass base::Time by value. #Patch Set 3 : Register synth trial on every URL request. #Patch Set 4 : Rebase. #Patch Set 5 : Use two field trials instead of multigroups. #Patch Set 6 : Add back !group_name.empty() check, plus comment. #Patch Set 7 : Remove unnecessary includes. #Patch Set 8 : Rebase. #
Messages
Total messages: 40 (18 generated)
twifkak@chromium.org changed reviewers: + jamartin@chromium.org, mpearson@google.com
mpearson: LG needed for chrome/browser/metrics. This is based on https://codereview.chromium.org/2562573003/ plus most of the additional changes I'd requested. (It turns out the test I requested is too hard to instrument given the current configuration.) rajendrant and rkaplow are OOO until 2017, so this is my "last, best shot."
Two points: (1) Chrome has no planned releases scheduled until next calendar year. Robert will be back then. I suggest you simply wait until he comes back. (It sounds vaguely like he has more context for this than me; not sure if I'm reading too much into this thread.) (2) You have unresolved comments on your patch set. Please deal with them. Once they're resolved, if you still want me to review rather than Robert, I am willing to if need be. Robert it looks like had the same request about dealing with open feedback before inviting other reviewers or trying to submit. --mark On Wed, Dec 21, 2016 at 3:23 PM, <twifkak@chromium.org> wrote: > Reviewers: jamartin, mpearson > CL: https://codereview.chromium.org/2596093002/ > > Message: > mpearson: LG needed for chrome/browser/metrics. > > This is based on https://codereview.chromium.org/2562573003/ plus most of > the > additional changes I'd requested. (It turns out the test I requested is > too hard > to instrument given the current configuration.) rajendrant and rkaplow are > OOO > until 2017, so this is my "last, best shot." > > Description: > Create a synthetic field trial for precache. > > For clients in a Precache trial group who've started a precache in the > last 15 > days, registers them in a matching group in the PrecacheSynthetic trial. > For > those who've started a precache in the last 1 day, registers in them in a > corresponding "Recent" group also. > > BUG=663987 > > Affected files (+76, -5 lines): > M chrome/browser/metrics/chrome_metrics_service_accessor.h > M chrome/browser/precache/precache_manager_factory.cc > M chrome/browser/precache/precache_util.h > M chrome/browser/precache/precache_util.cc > M components/precache/content/precache_manager.h > M components/precache/content/precache_manager.cc > M components/precache/content/precache_manager_unittest.cc > > > Index: chrome/browser/metrics/chrome_metrics_service_accessor.h > diff --git a/chrome/browser/metrics/chrome_metrics_service_accessor.h > b/chrome/browser/metrics/chrome_metrics_service_accessor.h > index 04df2f9a93a6cd4b4125a4f77b0865b34e5b0d98.. > b11bb9d7ce555e99a52ac22ac7b915c8a382b9fd 100644 > --- a/chrome/browser/metrics/chrome_metrics_service_accessor.h > +++ b/chrome/browser/metrics/chrome_metrics_service_accessor.h > @@ -49,6 +49,10 @@ namespace options { > class BrowserOptionsHandler; > } > > +namespace precache { > +void RegisterPrecacheSyntheticFieldTrial(const base::Time&); > +} > + > namespace prerender { > bool IsOmniboxEnabled(Profile* profile); > } > @@ -107,6 +111,7 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor > { > bool, > const OnMetricsReportingCallbackType&); > friend class options::BrowserOptionsHandler; > + friend void precache::RegisterPrecacheSyntheticFieldTrial(const > base::Time&); > friend bool prerender::IsOmniboxEnabled(Profile* profile); > friend class settings::MetricsReportingHandler; > friend class speech::ChromeSpeechRecognitionManagerDelegate; > Index: chrome/browser/precache/precache_manager_factory.cc > diff --git a/chrome/browser/precache/precache_manager_factory.cc > b/chrome/browser/precache/precache_manager_factory.cc > index c4d533fbb1ac49e1321d5a73f5834b327be1f0e6.. > feec9c9fc5c0a28833f805b9913f4a65582d6e3e 100644 > --- a/chrome/browser/precache/precache_manager_factory.cc > +++ b/chrome/browser/precache/precache_manager_factory.cc > @@ -11,6 +11,7 @@ > #include "chrome/browser/history/history_service_factory.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/precache/precache_util.h" > #include "chrome/browser/profiles/profile.h" > #include "chrome/browser/sync/profile_sync_service_factory.h" > #include "components/keyed_service/content/browser_context_ > dependency_manager.h" > @@ -60,7 +61,8 @@ KeyedService* PrecacheManagerFactory:: > BuildServiceInstanceFor( > ServiceAccessType::IMPLICIT_ACCESS), > DataReductionProxyChromeSettingsFactory::GetForBrowserContext( > browser_context), > - db_path, std::move(precache_database)); > + db_path, std::move(precache_database), > + base::Bind(&precache::RegisterPrecacheSyntheticFieldTrial)); > } > > } // namespace precache > Index: chrome/browser/precache/precache_util.cc > diff --git a/chrome/browser/precache/precache_util.cc > b/chrome/browser/precache/precache_util.cc > index 049155ba5122cea054071d7eb1b8884612c5a18d.. > f9a38aa00d8d3c2e6b5ff186c572fe2204e79c1d 100644 > --- a/chrome/browser/precache/precache_util.cc > +++ b/chrome/browser/precache/precache_util.cc > @@ -4,13 +4,19 @@ > > #include "chrome/browser/precache/precache_util.h" > > +#include <vector> > + > +#include "base/metrics/field_trial.h" > +#include "base/strings/string_util.h" > #include "base/time/time.h" > #include "chrome/browser/browser_process.h" > +#include "chrome/browser/metrics/chrome_metrics_service_accessor.h" > #include "chrome/browser/precache/precache_manager_factory.h" > #include "chrome/browser/profiles/profile.h" > #include "chrome/browser/profiles/profile_manager.h" > #include "components/data_use_measurement/content/content_ > url_request_classifier.h" > #include "components/precache/content/precache_manager.h" > +#include "components/variations/metrics_util.h" > #include "content/public/browser/browser_thread.h" > #include "net/url_request/url_request.h" > #include "url/gurl.h" > @@ -21,6 +27,15 @@ class HttpResponseInfo; > > namespace { > > +const char kPrecacheFieldTrialName[] = "Precache"; > +const char kPrecacheSyntheticFieldTrialName[] = "PrecacheSynthetic"; > + > +// Users who precached in the last |kDaysForPrecacheCandidates| and > +// |kDaysForRecentPrecacheCandidates| days will be placed in separate > trial > +// groups. > +const size_t kDaysForPrecacheCandidates = 15; > +const size_t kDaysForRecentPrecacheCandidates = 1; > + > void UpdatePrecacheMetricsAndStateOnUIThread(const GURL& url, > const GURL& referrer, > base::TimeDelta latency, > @@ -70,4 +85,17 @@ void UpdatePrecacheMetricsAndState(const > net::URLRequest* request, > data_use_measurement::IsUserRequest(*request), profile_id)); > } > > +void RegisterPrecacheSyntheticFieldTrial(const base::Time& > last_precache_time) { > + std::vector<uint32_t> groups; > + base::TimeDelta time_ago = base::Time::Now() - last_precache_time; > + std::string group_name = > + base::FieldTrialList::FindFullName(kPrecacheFieldTrialName); > + if (time_ago <= base::TimeDelta::FromDays(kDaysForPrecacheCandidates)) > + groups.push_back(metrics::HashName(group_name)); > + if (time_ago <= base::TimeDelta::FromDays(kDaysForRecentPrecacheCandidat > es)) > + groups.push_back(metrics::HashName(group_name + "Recent")); > + ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial( > + kPrecacheSyntheticFieldTrialName, groups); > +} > + > } // namespace precache > Index: chrome/browser/precache/precache_util.h > diff --git a/chrome/browser/precache/precache_util.h > b/chrome/browser/precache/precache_util.h > index 80a1fc8f0537df05ce81ed0958ada1d7e83a0bac.. > 32e221204310b1480bce5fcae3acbdb621f79354 100644 > --- a/chrome/browser/precache/precache_util.h > +++ b/chrome/browser/precache/precache_util.h > @@ -5,6 +5,8 @@ > #ifndef CHROME_BROWSER_PRECACHE_PRECACHE_UTIL_H_ > #define CHROME_BROWSER_PRECACHE_PRECACHE_UTIL_H_ > > +#include "base/time/time.h" > + > namespace net { > class URLRequest; > } > @@ -14,6 +16,12 @@ namespace precache { > void UpdatePrecacheMetricsAndState(const net::URLRequest* request, > void* profile_id); > > +// Registers the precache synthetic field trial for users whom the > precache task > +// started in the last |kDaysForPrecacheCandidates| days. The group name > of the > +// synthetic field trial will be the regular precache group. > +// |last_precache_time| is the last time precache task was run. > +void RegisterPrecacheSyntheticFieldTrial(const base::Time& > last_precache_time); > + > } // namespace precache > > #endif // CHROME_BROWSER_PRECACHE_PRECACHE_UTIL_H_ > Index: components/precache/content/precache_manager.cc > diff --git a/components/precache/content/precache_manager.cc > b/components/precache/content/precache_manager.cc > index 204ab492392722db4e52799aebea116da60f1305.. > 4ab90fd75a0d0ffadeb262907cc19014cdbf1f59 100644 > --- a/components/precache/content/precache_manager.cc > +++ b/components/precache/content/precache_manager.cc > @@ -63,17 +63,26 @@ PrecacheManager::PrecacheManager( > const data_reduction_proxy::DataReductionProxySettings* > data_reduction_proxy_settings, > const base::FilePath& db_path, > - std::unique_ptr<PrecacheDatabase> precache_database) > + std::unique_ptr<PrecacheDatabase> precache_database, > + const base::Callback<void(const base::Time&)>& > + register_synthetic_field_trial_callback) > : browser_context_(browser_context), > sync_service_(sync_service), > history_service_(history_service), > data_reduction_proxy_settings_(data_reduction_proxy_settings), > - is_precaching_(false) { > + is_precaching_(false), > + register_synthetic_field_trial_callback_( > + register_synthetic_field_trial_callback) { > precache_database_ = std::move(precache_database); > BrowserThread::PostTask( > BrowserThread::DB, FROM_HERE, > base::Bind(base::IgnoreResult(&PrecacheDatabase::Init), > base::Unretained(precache_database_.get()), db_path)); > + BrowserThread::PostTaskAndReplyWithResult( > + BrowserThread::DB, FROM_HERE, > + base::Bind(&PrecacheDatabase::GetLastPrecacheTimestamp, > + base::Unretained(precache_database_.get())), > + base::Bind(&PrecacheManager::RegisterSyntheticFieldTrial, AsWeakPtr())); > } > > PrecacheManager::~PrecacheManager() { > @@ -84,6 +93,11 @@ PrecacheManager::~PrecacheManager() { > precache_database_.release()); > } > > +void PrecacheManager::RegisterSyntheticFieldTrial( > + const base::Time& last_precache_time) { > + register_synthetic_field_trial_callback_.Run(last_precache_time); > +} > + > bool PrecacheManager::IsInExperimentGroup() const { > // Verify IsPrecachingAllowed() before calling > FieldTrialList::FindFullName(). > // This is because field trials are only assigned when requested. This > allows > Index: components/precache/content/precache_manager.h > diff --git a/components/precache/content/precache_manager.h > b/components/precache/content/precache_manager.h > index ec093b0057a520aa177ac8fd282f8d7cae5fc70d.. > eee04410b416df18cb5c72f65f28b4fc67b255ed 100644 > --- a/components/precache/content/precache_manager.h > +++ b/components/precache/content/precache_manager.h > @@ -81,7 +81,9 @@ class PrecacheManager : public KeyedService, > const data_reduction_proxy::DataReductionProxySettings* > data_reduction_proxy_settings, > const base::FilePath& db_path, > - std::unique_ptr<PrecacheDatabase> precache_database); > + std::unique_ptr<PrecacheDatabase> precache_database, > + const base::Callback<void(const base::Time&)>& > + register_synthetic_field_trial_callback); > ~PrecacheManager() override; > > // Returns true if the client is in the experiment group -- that is, > @@ -153,6 +155,11 @@ class PrecacheManager : public KeyedService, > // From PrecacheFetcher::PrecacheDelegate. > void OnDone() override; > > + // Registers the precache synthetic field trial for users whom the > precache > + // task was run recently. |last_precache_time| is the last time precache > task > + // was run. > + void RegisterSyntheticFieldTrial(const base::Time& last_precache_time); > + > // Callback when fetching unfinished work from storage is done. > void OnGetUnfinishedWorkDone( > std::unique_ptr<PrecacheUnfinishedWork> unfinished_work); > @@ -239,6 +246,10 @@ class PrecacheManager : public KeyedService, > // Work that hasn't yet finished. > std::unique_ptr<PrecacheUnfinishedWork> unfinished_work_; > > + // Callback to register the synthetic field trial. > + const base::Callback<void(const base::Time&)> > + register_synthetic_field_trial_callback_; > + > DISALLOW_COPY_AND_ASSIGN(PrecacheManager); > }; > > Index: components/precache/content/precache_manager_unittest.cc > diff --git a/components/precache/content/precache_manager_unittest.cc > b/components/precache/content/precache_manager_unittest.cc > index a93a2ca8caab07e55e83696db23b33d688e84c9a.. > cb166d362d2f8bc5d1d95dc851a735d76958b79d 100644 > --- a/components/precache/content/precache_manager_unittest.cc > +++ b/components/precache/content/precache_manager_unittest.cc > @@ -130,6 +130,8 @@ class TestPrecacheCompletionCallback { > bool was_on_done_called_; > }; > > +void RegisterSyntheticFieldTrial(const base::Time& last_precache_time) {} > + > class PrecacheManagerUnderTest : public PrecacheManager { > public: > PrecacheManagerUnderTest( > @@ -145,7 +147,8 @@ class PrecacheManagerUnderTest : public > PrecacheManager { > history_service, > data_reduction_proxy_settings, > db_path, > - std::move(precache_database)), > + std::move(precache_database), > + base::Bind(&precache::RegisterSyntheticFieldTrial)), > control_group_(false) {} > bool IsInExperimentGroup() const override { return !control_group_; } > bool IsInControlGroup() const override { return control_group_; } > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Dec 21, 2016 at 3:41 PM, Mark Pearson <mpearson@google.com> wrote: > (1) > Chrome has no planned releases scheduled until next calendar year. > Does that include canaries? > (2) > You have unresolved comments on your patch set. > I wrote those comments. As specified in my email, I resolved all but one, which proved too difficult. Given that it was a "would be nice", I'm okay with letting it go. That said, it sounds like you feel strongly I should simply wait until next year. If that's true, I'm okay with doing so. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM. Some questions and suggestions for minor improvements. https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... File chrome/browser/precache/precache_util.cc (right): https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... chrome/browser/precache/precache_util.cc:37: const size_t kDaysForRecentPrecacheCandidates = 1; (nit) s/kDaysForPrecacheCandidates/kDaysForPrecache/ s/kDaysForRecentPrecacheCandidates/kDaysForPrecacheRecent/ I accept if you want to keep the Candidates suffix but I think there is some merit in having PrecacheRecent matching the group name. Or maybe even better, instead of naming the synthetic groups "Precache" and "PrecacheRecent", which may be confusing since our experiment is called "Precache" already, why not simply call them "1D" and "15D"? Reading the documentation I believe you did this to guarantee that the trial group names are unique. Then, I'd suggest "Precache1D" and "Precache15D". https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... chrome/browser/precache/precache_util.cc:97: ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial( I couldn't find any other caller for this function. Either I didn't look for it hard enough or I this is a bit concerning. https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... File chrome/browser/precache/precache_util.h (right): https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... chrome/browser/precache/precache_util.h:23: void RegisterPrecacheSyntheticFieldTrial(const base::Time& last_precache_time); // All time classes are copyable, assignable, and occupy 64-bits per instance. // As a result, prefer passing them by value: https://codereview.chromium.org/2596093002/diff/1/components/precache/content... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager.cc:81: BrowserThread::PostTaskAndReplyWithResult( There are no race conditions between these two tasks, are there? https://codereview.chromium.org/2596093002/diff/1/components/precache/content... File components/precache/content/precache_manager.h (right): https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager.h:85: const base::Callback<void(const base::Time&)>& Idem. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager.h:161: void RegisterSyntheticFieldTrial(const base::Time& last_precache_time); Idem. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager.h:250: const base::Callback<void(const base::Time&)> Idem. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... File components/precache/content/precache_manager_unittest.cc (right): https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:133: void RegisterSyntheticFieldTrial(const base::Time& last_precache_time) {} Idem. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:133: void RegisterSyntheticFieldTrial(const base::Time& last_precache_time) {} Maybe test that this is eventually called with the right time?
On Wed, Dec 21, 2016 at 3:48 PM, Devin Mullins <twifkak@chromium.org> wrote: > On Wed, Dec 21, 2016 at 3:41 PM, Mark Pearson <mpearson@google.com> wrote: > >> (1) >> Chrome has no planned releases scheduled until next calendar year. >> > > Does that include canaries? > I'm not sure. The calendar doesn't make it clear. You could make the TPM to check. That said, I don't trust data (even from stable) over the holidays as indicating whether a change is good or bad, because user behavior is so different. And I at least have never thought about trying to believe anything canary channel is trying to tell me either... I think you could wait, especially if Rob has more context. --mark > > >> (2) >> You have unresolved comments on your patch set. >> > > I wrote those comments. As specified in my email, I resolved all but one, > which proved too difficult. Given that it was a "would be nice", I'm okay > with letting it go. > > That said, it sounds like you feel strongly I should simply wait until > next year. If that's true, I'm okay with doing so. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
twifkak@chromium.org changed reviewers: + rkaplow@chromium.org - mpearson@google.com
rkaplow: PTAL. This is a fork of https://codereview.chromium.org/2562573003/ with the changes I suggested in that CL. jamartin: PTAL. https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... File chrome/browser/precache/precache_util.cc (right): https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... chrome/browser/precache/precache_util.cc:37: const size_t kDaysForRecentPrecacheCandidates = 1; On 2016/12/21 23:54:33, jamartin wrote: > (nit) > > s/kDaysForPrecacheCandidates/kDaysForPrecache/ > s/kDaysForRecentPrecacheCandidates/kDaysForPrecacheRecent/ > > I accept if you want to keep the Candidates suffix but I think there is some > merit in having PrecacheRecent matching the group name. > > Or maybe even better, instead of naming the synthetic groups "Precache" and > "PrecacheRecent", which may be confusing since our experiment is called > "Precache" already, why not simply call them "1D" and "15D"? > > Reading the documentation I believe you did this to guarantee that the trial > group names are unique. Then, I'd suggest "Precache1D" and "Precache15D". You read my code incorrectly. Keep in mind that there are two names: the trial name and the group name. For succinctness, I'll denote with a slash, as TrialName/GroupName. The function below maps Precache/Control to zero or more of PrecacheSynthetic/Control and PrecacheSynthetic/ControlRecent. Ditto for Precache/Enabled. I renamed Control{,Recent} to Control{15,1}D, per your suggestion. I got rid of the constants, since having a `const int kFifteen = 15` is pretty ridiculous. I also added some comments. https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... chrome/browser/precache/precache_util.cc:97: ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial( On 2016/12/21 23:54:33, jamartin wrote: > I couldn't find any other caller for this function. Either I didn't look for it > hard enough or I this is a bit concerning. It looks like it was implemented in https://chromium.googlesource.com/chromium/src/+/6a1c881f89fe55559248668026b0... and its only caller is in https://chrome-internal.googlesource.com/clank/internal/apps/+/556a4789aeab52.... https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... File chrome/browser/precache/precache_util.h (right): https://codereview.chromium.org/2596093002/diff/1/chrome/browser/precache/pre... chrome/browser/precache/precache_util.h:23: void RegisterPrecacheSyntheticFieldTrial(const base::Time& last_precache_time); On 2016/12/21 23:54:33, jamartin wrote: > // All time classes are copyable, assignable, and occupy 64-bits per instance. > // As a result, prefer passing them by value: Done. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager.cc:81: BrowserThread::PostTaskAndReplyWithResult( On 2016/12/21 23:54:33, jamartin wrote: > There are no race conditions between these two tasks, are there? Assuming you mean the potential race between Init and GetLastPrecacheTimestamp: It turns out there isn't, but it's not obvious. PostTaskAndReplyWithResult posts a nestable post task, which means that it could be started before the previous tasks are finished, according to sequenced_task_runner.h. However, AFAICT, while SequencedTaskRunner is an interface, the actually implementation of the task runner is backed by a base::MessageLoop with NestableTasksAllowed() == false. (Verified with a DCHECK running on my phone.) OTOH, even if there is a race, the return value of GetLastPrecacheTimestamp when !IsDatabaseAccessible() is a "null" base::Time, which would simply result in no synth trials being registered. Certainly not what I want, but not a crash. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... File components/precache/content/precache_manager.h (right): https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager.h:85: const base::Callback<void(const base::Time&)>& On 2016/12/21 23:54:33, jamartin wrote: > Idem. Done. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager.h:161: void RegisterSyntheticFieldTrial(const base::Time& last_precache_time); On 2016/12/21 23:54:33, jamartin wrote: > Idem. Done. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager.h:250: const base::Callback<void(const base::Time&)> On 2016/12/21 23:54:33, jamartin wrote: > Idem. Done. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... File components/precache/content/precache_manager_unittest.cc (right): https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:133: void RegisterSyntheticFieldTrial(const base::Time& last_precache_time) {} On 2016/12/21 23:54:34, jamartin wrote: > Idem. Done. https://codereview.chromium.org/2596093002/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:133: void RegisterSyntheticFieldTrial(const base::Time& last_precache_time) {} On 2016/12/21 23:54:34, jamartin wrote: > Maybe test that this is eventually called with the right time? Done.
overall this lg, I just want to make sure this is going to measure what you want. I'm not that familiar with the whole precache stuff - so it very well could be all good, I'm just double checking. The field trial will be registered once, and then all the UMA metrics will be covered. I'm a bit concerned this will not worth for longer sessions - for example someone starts and the condition applies, but then the user is using Chrome for a while and then perhaps the precache doesn't apply. Or vice versa, during the session of Chrome they should have moved to another group. Basically I just want to make sure all UMA records in a session tagged with one of these trial groups, all subsequent events should be in that group, even if they are days later.
On 2017/01/06 16:00:41, rkaplow wrote: > The field trial will be registered once, and then all the UMA metrics will be > covered. Oy, indeed, thanks for catching this (shamecube). Would it be all right to call ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial from every request (via chrome_network_delegate.cc), or is that too expensive?
On 2017/01/06 20:43:03, twifkak wrote: > On 2017/01/06 16:00:41, rkaplow wrote: > > The field trial will be registered once, and then all the UMA metrics will be > > covered. > > Oy, indeed, thanks for catching this (shamecube). > > Would it be all right to call > ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial from every > request (via chrome_network_delegate.cc), or is that too expensive? Actually something like that would somewhat work. Actually you should be using RegisterSyntheticFieldTrial and not the multigroup version. Say you register Enabled_15 or whatever, then all metrics on that log will be tagged with that group. Then say partway through the next log you register some other group (Enable_1 say). That log will not report any group at all (since it's ambiguous). Then the subsequent log will be reporting Enabled_1 (assuming you don't register a group again). If you don't mind missing the data when a register is called (and the data +/- 30 minutes around it) - it will work. Does that suit your needs?
On 2017/01/06 20:49:45, rkaplow wrote: > On 2017/01/06 20:43:03, twifkak wrote: > > On 2017/01/06 16:00:41, rkaplow wrote: > > > The field trial will be registered once, and then all the UMA metrics will > be > > > covered. > > > > Oy, indeed, thanks for catching this (shamecube). > > > > Would it be all right to call > > ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial from every > > request (via chrome_network_delegate.cc), or is that too expensive? > > Actually something like that would somewhat work. > Actually you should be using RegisterSyntheticFieldTrial and not the multigroup > version. I don't think so. "Enabled15D" means "users who precached <= 15 days ago" and "Enabled1D" means "users who precached <= 1 day ago". So the latter is a subset of the former; I still want those users to show up in Eanbled15D. Otherwise 15D would mean "users who precached (1, 15] days ago" which is not what I want to look at in Finch. (I could look at the union of Enabled15D and Enabled1D and compare that against the union of Control15D and Control1D, but the point of this change is to be able to look at our metrics directly in Finch rather than with a custom pipeline.)
On 2017/01/06 20:49:45, rkaplow wrote: > On 2017/01/06 20:43:03, twifkak wrote: > > On 2017/01/06 16:00:41, rkaplow wrote: > > > The field trial will be registered once, and then all the UMA metrics will > be > > > covered. > > > > Oy, indeed, thanks for catching this (shamecube). > > > > Would it be all right to call > > ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial from every > > request (via chrome_network_delegate.cc), or is that too expensive? > > Actually something like that would somewhat work. Okay, done, PTAL.
The CQ bit was checked by twifkak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by twifkak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/07 01:06:10, twifkak wrote: > On 2017/01/06 20:49:45, rkaplow wrote: > > On 2017/01/06 20:43:03, twifkak wrote: > > > On 2017/01/06 16:00:41, rkaplow wrote: > > > > The field trial will be registered once, and then all the UMA metrics will > > be > > > > covered. > > > > > > Oy, indeed, thanks for catching this (shamecube). > > > > > > Would it be all right to call > > > ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial from > every > > > request (via chrome_network_delegate.cc), or is that too expensive? > > > > Actually something like that would somewhat work. > > Okay, done, PTAL. Another option is to set it up as two separate synthetic studies. It's a bit messy but then you wouldn't have the overlap issue, and you can treat each <N group seperately. You wouldn't be able to compare two groups to each other (versus control) easily, but could do so looking at different tabs. Just mentioning as another option...
On 2017/01/09 21:49:35, rkaplow wrote: > On 2017/01/07 01:06:10, twifkak wrote: > > On 2017/01/06 20:49:45, rkaplow wrote: > > > On 2017/01/06 20:43:03, twifkak wrote: > > > > On 2017/01/06 16:00:41, rkaplow wrote: > > > > > The field trial will be registered once, and then all the UMA metrics > will > > > be > > > > > covered. > > > > > > > > Oy, indeed, thanks for catching this (shamecube). > > > > > > > > Would it be all right to call > > > > ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial from > > every > > > > request (via chrome_network_delegate.cc), or is that too expensive? > > > > > > Actually something like that would somewhat work. > > > > Okay, done, PTAL. > > Another option is to set it up as two separate synthetic studies. It's a bit > messy but then you wouldn't have the overlap issue, and you can treat each <N > group seperately. You wouldn't be able to compare two groups to each other > (versus control) easily, but could do so looking at different tabs. Just > mentioning as another option... True. What's the advantage of this option? Are you looking to remove multigroups as a temporary hack, or is there an inaccuracy to the collection/analysis somehow?
On 2017/01/09 21:54:11, twifkak wrote: > On 2017/01/09 21:49:35, rkaplow wrote: > > On 2017/01/07 01:06:10, twifkak wrote: > > > On 2017/01/06 20:49:45, rkaplow wrote: > > > > On 2017/01/06 20:43:03, twifkak wrote: > > > > > On 2017/01/06 16:00:41, rkaplow wrote: > > > > > > The field trial will be registered once, and then all the UMA metrics > > will > > > > be > > > > > > covered. > > > > > > > > > > Oy, indeed, thanks for catching this (shamecube). > > > > > > > > > > Would it be all right to call > > > > > ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial from > > > every > > > > > request (via chrome_network_delegate.cc), or is that too expensive? > > > > > > > > Actually something like that would somewhat work. > > > > > > Okay, done, PTAL. > > > > Another option is to set it up as two separate synthetic studies. It's a bit > > messy but then you wouldn't have the overlap issue, and you can treat each <N > > group seperately. You wouldn't be able to compare two groups to each other > > (versus control) easily, but could do so looking at different tabs. Just > > mentioning as another option... > > True. What's the advantage of this option? Are you looking to remove multigroups > as a temporary hack, or is there an inaccuracy to the collection/analysis > somehow? rkaplow: Friendly ping.
On 2017/01/11 19:52:43, twifkak wrote: > On 2017/01/09 21:54:11, twifkak wrote: > > On 2017/01/09 21:49:35, rkaplow wrote: > > > On 2017/01/07 01:06:10, twifkak wrote: > > > > On 2017/01/06 20:49:45, rkaplow wrote: > > > > > On 2017/01/06 20:43:03, twifkak wrote: > > > > > > On 2017/01/06 16:00:41, rkaplow wrote: > > > > > > > The field trial will be registered once, and then all the UMA > metrics > > > will > > > > > be > > > > > > > covered. > > > > > > > > > > > > Oy, indeed, thanks for catching this (shamecube). > > > > > > > > > > > > Would it be all right to call > > > > > > ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial > from > > > > every > > > > > > request (via chrome_network_delegate.cc), or is that too expensive? > > > > > > > > > > Actually something like that would somewhat work. > > > > > > > > Okay, done, PTAL. > > > > > > Another option is to set it up as two separate synthetic studies. It's a > bit > > > messy but then you wouldn't have the overlap issue, and you can treat each > <N > > > group seperately. You wouldn't be able to compare two groups to each other > > > (versus control) easily, but could do so looking at different tabs. Just > > > mentioning as another option... > > > > True. What's the advantage of this option? Are you looking to remove > multigroups > > as a temporary hack, or is there an inaccuracy to the collection/analysis > > somehow? > > rkaplow: Friendly ping. Ah sorry I missed the response. Our processing logic don't correctly support the multi-group stuff as in RegisterSyntheticMultiGroupFieldTrial. So I wouldn't suggest using it. So using separate trials will allow you to correctly look at the users in who would fit into multiple groups.
On 2017/01/11 20:00:55, rkaplow wrote: > On 2017/01/11 19:52:43, twifkak wrote: > > On 2017/01/09 21:54:11, twifkak wrote: > > > On 2017/01/09 21:49:35, rkaplow wrote: > > > > On 2017/01/07 01:06:10, twifkak wrote: > > > > > On 2017/01/06 20:49:45, rkaplow wrote: > > > > > > On 2017/01/06 20:43:03, twifkak wrote: > > > > > > > On 2017/01/06 16:00:41, rkaplow wrote: > > > > > > > > The field trial will be registered once, and then all the UMA > > metrics > > > > will > > > > > > be > > > > > > > > covered. > > > > > > > > > > > > > > Oy, indeed, thanks for catching this (shamecube). > > > > > > > > > > > > > > Would it be all right to call > > > > > > > ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial > > from > > > > > every > > > > > > > request (via chrome_network_delegate.cc), or is that too expensive? > > > > > > > > > > > > Actually something like that would somewhat work. > > > > > > > > > > Okay, done, PTAL. > > > > > > > > Another option is to set it up as two separate synthetic studies. It's a > > bit > > > > messy but then you wouldn't have the overlap issue, and you can treat each > > <N > > > > group seperately. You wouldn't be able to compare two groups to each other > > > > (versus control) easily, but could do so looking at different tabs. Just > > > > mentioning as another option... > > > > > > True. What's the advantage of this option? Are you looking to remove > > multigroups > > > as a temporary hack, or is there an inaccuracy to the collection/analysis > > > somehow? > > > > rkaplow: Friendly ping. > > Ah sorry I missed the response. > > Our processing logic don't correctly support the multi-group stuff as in > RegisterSyntheticMultiGroupFieldTrial. So I wouldn't suggest using it. So using > separate trials will allow you to correctly look at the users in who would fit > into multiple groups. Ah, I see. Okay, done, PTAL.
Description was changed from ========== Create a synthetic field trial for precache. For clients in a Precache trial group who've started a precache in the last 15 days, registers them in a matching group in the PrecacheSynthetic trial. For those who've started a precache in the last 1 day, registers in them in a corresponding "Recent" group also. BUG=663987 ========== to ========== Create a synthetic field trial for precache. For clients in a Precache trial group who've started a precache in the last 15 days, registers them in a matching group in the PrecacheSynthetic15D trial. For those who've started a precache in the last 1 day, registers in them in the PrecacheSynthetic1D trial also. BUG=663987 ==========
lgtm lg, sorry my turnaround was pretty slow for the review
The CQ bit was checked by twifkak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamartin@chromium.org Link to the patchset: https://codereview.chromium.org/2596093002/#ps100001 (title: "Add back !group_name.empty() check, plus comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/12 16:03:07, rkaplow wrote: > lgtm > > lg, sorry my turnaround was pretty slow for the review Thanks. FWIW, your median turnaround was pretty fast; you just had a couple outliers, and this particular patch just required a lot of back-and-forth.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by twifkak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamartin@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2596093002/#ps140001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484248632277840, "parent_rev": "7903b42aa872bd49033a43b51268a78191b0bbc0", "commit_rev": "9020aee19a0a9cdb845f7cb2bc7f30c799741fdf"}
Message was sent while issue was closed.
Description was changed from ========== Create a synthetic field trial for precache. For clients in a Precache trial group who've started a precache in the last 15 days, registers them in a matching group in the PrecacheSynthetic15D trial. For those who've started a precache in the last 1 day, registers in them in the PrecacheSynthetic1D trial also. BUG=663987 ========== to ========== Create a synthetic field trial for precache. For clients in a Precache trial group who've started a precache in the last 15 days, registers them in a matching group in the PrecacheSynthetic15D trial. For those who've started a precache in the last 1 day, registers in them in the PrecacheSynthetic1D trial also. BUG=663987 Review-Url: https://codereview.chromium.org/2596093002 Cr-Commit-Position: refs/heads/master@{#443352} Committed: https://chromium.googlesource.com/chromium/src/+/9020aee19a0a9cdb845f7cb2bc7f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9020aee19a0a9cdb845f7cb2bc7f... |