Create a synthetic field trial for precache.

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/+/9020aee19a0a9cdb845f7cb2bc7f30c799741fdf

[twifkak, 2016-12-21 23:23:48]

twifkak@chromium.org changed reviewers:
+ jamartin@chromium.org, mpearson@google.com

[twifkak, 2016-12-21 23:23:49]

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."

[chromium-reviews, 2016-12-21 23:41:24]

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.

[twifkak, 2016-12-21 23:49:07]

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.

[jamartin, 2016-12-21 23:54:34]

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?

[chromium-reviews, 2016-12-21 23:58:21]

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, 2017-01-05 23:53:13]

twifkak@chromium.org changed reviewers:
+ rkaplow@chromium.org
- mpearson@google.com

[twifkak, 2017-01-05 23:53:14]

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.

[rkaplow, 2017-01-06 16:00:41]

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.

[twifkak, 2017-01-06 20:43:03]

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?

[rkaplow, 2017-01-06 20:49:45]

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?

[twifkak, 2017-01-06 20:59:16]

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.)

[twifkak, 2017-01-07 01:06:10]

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.

[twifkak, 2017-01-07 01:06:29]

The CQ bit was checked by twifkak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 01:07:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 01:09:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 01:09:04]

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...)

[twifkak, 2017-01-07 01:43:57]

The CQ bit was checked by twifkak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 01:44:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 02:27:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 02:27:37]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-01-09 21:49:35]

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...

[twifkak, 2017-01-09 21:54:11]

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?

[twifkak, 2017-01-11 19:52:43]

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.

[rkaplow, 2017-01-11 20:00:55]

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.

[twifkak, 2017-01-11 21:23:54]

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.

[twifkak, 2017-01-11 22:30:00]

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
==========

[rkaplow, 2017-01-12 16:03:07]

lgtm

lg, sorry my turnaround was pretty slow for the review

[twifkak, 2017-01-12 18:23:45]

The CQ bit was checked by twifkak@chromium.org

[twifkak, 2017-01-12 18:23:46]

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.")

[commit-bot: I haz the power, 2017-01-12 18:24:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[twifkak, 2017-01-12 18:25:11]

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.

[commit-bot: I haz the power, 2017-01-12 18:27:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 18:27:18]

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...)

[twifkak, 2017-01-12 19:17:12]

The CQ bit was checked by twifkak@chromium.org

[twifkak, 2017-01-12 19:17:12]

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.")

[commit-bot: I haz the power, 2017-01-12 19:18:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 20:48:20]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484248632277840,
"parent_rev": "7903b42aa872bd49033a43b51268a78191b0bbc0", "commit_rev":
"9020aee19a0a9cdb845f7cb2bc7f30c799741fdf"}

[commit-bot: I haz the power, 2017-01-12 20:48:51]

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...
==========

[commit-bot: I haz the power, 2017-01-12 20:48:57]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/9020aee19a0a9cdb845f7cb2bc7f...