Add experiment to exercise AffiliationService with dummy data, plus add related UMA histograms.

Add experiment to exercise AffiliationService with dummy data, plus add related UMA histograms.

Add support for an experiment where affiliation information is prefetched for a set of dummy Android facet URIs on start-up. Then, first shortly after start-up, and then periodically once an hour, the AffiliationService is queried for affiliations of a set of related dummy Web facet URIs. It is expected that these requests can be answered from the cache. This allows testing the AffiliationService before it can be used with any real data.

Furthermore, add UMA histograms to measure some basic metrics in general and wrt. the experiment:
  * AffiliationFetcher: Success/failure counts; and HTTP result and network error codes on failure.
  * AffiliationBackend: The number of facets requested in a single network fetch; the time before the first network fetch and between subsequent network fetches, to give an insight into how well the fetch scheduling/throttling logic works.
  * Experiment-specific: The number of times that affiliations for the dummy Web facets were successfully returned, and the number of results returned each time, to give an insight into how high the fill factor of data is in the cache.

BUG=478952
Committed: https://crrev.com/c096020684b111a8ba7cf94f549841afdb0c96d1
Cr-Commit-Position: refs/heads/master@{#326284}

[engedy, 2015-04-20 22:26:01]

engedy@chromium.org changed reviewers:
+ isherman@chromium.org, mkwst@chromium.org

[engedy, 2015-04-20 22:27:54]

@Ilya, could you please take a look at the histogram- and variations-related
parts?

@Mike, could you please review everything?

[Ilya Sherman, 2015-04-21 00:38:05]

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:67:
UMA_HISTOGRAM_COUNTS_100("AffiliationService.DummyData.RequestResultCount",
Would it be appropriate for these histograms to be defined in the "Passwords"
namespace, i.e. "Passwords.AffiliationService.*"?

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:68:
results.size());
Should this be logged both on success and on failure, or only on success?

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliation_backend.cc (right):

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_backend.cc:237:
base::TimeDelta::FromDays(7), 100);
Hmm, do you really need such a large range?

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_utils.cc:326:
kFieldTrialName, "affiliation_requests_for_dummy_facets");
Out of curiousity, what is the advantage of using a param over a direct field
trial to control a simple enabled/disabled state?

https://codereview.chromium.org/1090163004/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1090163004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:266: +    dummy Android facets into the
cache.
Boy, I find this sentence hard to parse.  Is this recorded once per dummy Web
facet, or once for the aggregate?  Maybe you could split it into multiple
sentences for improved clarity?

https://codereview.chromium.org/1090163004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:274: +    recorded for every request.
nit: The histogram descriptions might be easier to read if you split off the
"recorded upon ..." information into a separate sentence.  (Applies to all of
the histograms)

https://codereview.chromium.org/1090163004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:282: +    needs to issue a network
fetch.
nit: "needs" -> "needed"

[Mike West, 2015-04-21 08:53:42]

The mechanics of the change LGTM, but I'll defer to Ilya for the metrics portion
(other than to suggest that it would be good to add a bit more detail to the CL
description about what you're measuring and why).

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:67:
UMA_HISTOGRAM_COUNTS_100("AffiliationService.DummyData.RequestResultCount",
On 2015/04/21 at 00:38:05, Ilya Sherman wrote:
> Would it be appropriate for these histograms to be defined in the "Passwords"
namespace, i.e. "Passwords.AffiliationService.*"?

Sounds good to me too.

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:245:
&AffiliatedMatchHelper::VerifyAffiliationsOfDummyFacetsPrefetched);
How often is this going to trigger?

[engedy, 2015-04-21 18:29:08]

Addressed comments, updated CL description, and add some more histograms, and
tidied up everything.

@Ilya, @Mike, please take another look.

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:67:
UMA_HISTOGRAM_COUNTS_100("AffiliationService.DummyData.RequestResultCount",
On 2015/04/21 08:53:42, Mike West wrote:
> On 2015/04/21 at 00:38:05, Ilya Sherman wrote:
> > Would it be appropriate for these histograms to be defined in the
"Passwords"
> namespace, i.e. "Passwords.AffiliationService.*"?
> 
> Sounds good to me too.

SGTM, I have moved histograms to the PasswordManager.Affiliation* namespace.

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:68:
results.size());
On 2015/04/21 00:38:05, Ilya Sherman wrote:
> Should this be logged both on success and on failure, or only on success?

Yeah, does not make much sense on failure. Added conditional. Done.

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliated_match_helper.cc:245:
&AffiliatedMatchHelper::VerifyAffiliationsOfDummyFacetsPrefetched);
On 2015/04/21 08:53:42, Mike West wrote:
> How often is this going to trigger?

Right, this is why I should not be sending out CLs after midnight. Every hour, I
just shortened it for testing. :-/

I have changed it to 1 minute after start, and then once every hour.

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliation_backend.cc (right):

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_backend.cc:237:
base::TimeDelta::FromDays(7), 100);
On 2015/04/21 00:38:05, Ilya Sherman wrote:
> Hmm, do you really need such a large range?

I need quite a large range, but on second though, not this much indeed. Reduced
to 1 sec ... 3 days and to 50 buckets.

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_utils.cc:326:
kFieldTrialName, "affiliation_requests_for_dummy_facets");
On 2015/04/21 00:38:05, Ilya Sherman wrote:
> Out of curiousity, what is the advantage of using a param over a direct field
> trial to control a simple enabled/disabled state?

We will never get to initializing this sub-feature unless the main feature is
enabled. So if they are controlled by two separate field trials, then the
sub-feature is only enabled for the intersection of users. That seemed not so
nice in terms of easy of analyzing the results.

Also, it seemed clearer to just have one field trial, with multiple experiment
groups, with each one having some set of sub-features enabled. WDYT?

https://codereview.chromium.org/1090163004/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1090163004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:266: +    dummy Android facets into the
cache.
On 2015/04/21 00:38:05, Ilya Sherman wrote:
> Boy, I find this sentence hard to parse.  Is this recorded once per dummy Web
> facet, or once for the aggregate?  Maybe you could split it into multiple
> sentences for improved clarity?

Done.

https://codereview.chromium.org/1090163004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:274: +    recorded for every request.
On 2015/04/21 00:38:05, Ilya Sherman wrote:
> nit: The histogram descriptions might be easier to read if you split off the
> "recorded upon ..." information into a separate sentence.  (Applies to all of
> the histograms)

Done.

https://codereview.chromium.org/1090163004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:282: +    needs to issue a network
fetch.
On 2015/04/21 00:38:05, Ilya Sherman wrote:
> nit: "needs" -> "needed"

Done.

[Ilya Sherman, 2015-04-21 20:11:27]

metrics LGTM % a couple of optional nits:

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1090163004/diff/1/components/password_manager...
components/password_manager/core/browser/affiliation_utils.cc:326:
kFieldTrialName, "affiliation_requests_for_dummy_facets");
On 2015/04/21 18:29:07, engedy wrote:
> On 2015/04/21 00:38:05, Ilya Sherman wrote:
> > Out of curiousity, what is the advantage of using a param over a direct
field
> > trial to control a simple enabled/disabled state?
> 
> We will never get to initializing this sub-feature unless the main feature is
> enabled. So if they are controlled by two separate field trials, then the
> sub-feature is only enabled for the intersection of users. That seemed not so
> nice in terms of easy of analyzing the results.
> 
> Also, it seemed clearer to just have one field trial, with multiple experiment
> groups, with each one having some set of sub-features enabled. WDYT?

Got it.  Yeah, that sounds reasonable.  Thanks for the explanation =)

https://codereview.chromium.org/1090163004/diff/40001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1090163004/diff/40001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:68:
"PasswordManager.AffiliationDummyData.Startup_RequestSuccess", success);
Optional nit: It seems a little weird to mix "." and "_" as separators.  You can
keep this as-is if you want to, but I'd recommend using "." throughout for
consistency.  Note that if you're choosing "_" to work with the field_trial
mechanism, you can actually specify the separator in the histograms.xml entry,
as separator="."

https://codereview.chromium.org/1090163004/diff/40001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:76:
"PasswordManager.AffiliationDummyData.Periodic_RequestSuccess",
Optional nit: If you're going to want to compare Startup_RequestSuccess vs.
Periodic_RequestSuccess, and likewise for the other pair, it might be more
helpful to name them as "RequestSuccess_Startup", etc., so that the most closely
comparable histograms are sorted alphabetically together.

[engedy, 2015-04-21 20:23:16]

https://codereview.chromium.org/1090163004/diff/40001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1090163004/diff/40001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:68:
"PasswordManager.AffiliationDummyData.Startup_RequestSuccess", success);
On 2015/04/21 20:11:27, Ilya Sherman wrote:
> Optional nit: It seems a little weird to mix "." and "_" as separators.  You
can
> keep this as-is if you want to, but I'd recommend using "." throughout for
> consistency.  Note that if you're choosing "_" to work with the field_trial
> mechanism, you can actually specify the separator in the histograms.xml entry,
> as separator="."

I have changed the separator here, and added the attribute into histograms.xml. 

Not sure I understand what you mean by "if you're choosing "_" to work with the
field_trial mechanism" -- could you please elaborate on that?

https://codereview.chromium.org/1090163004/diff/40001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:76:
"PasswordManager.AffiliationDummyData.Periodic_RequestSuccess",
On 2015/04/21 20:11:27, Ilya Sherman wrote:
> Optional nit: If you're going to want to compare Startup_RequestSuccess vs.
> Periodic_RequestSuccess, and likewise for the other pair, it might be more
> helpful to name them as "RequestSuccess_Startup", etc., so that the most
closely
> comparable histograms are sorted alphabetically together.

Done. Also, changed "Startup" to "OnStartup" to be less confusing as a suffix.

[Ilya Sherman, 2015-04-21 21:46:53]

(Still LGTM)

https://codereview.chromium.org/1090163004/diff/40001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1090163004/diff/40001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:68:
"PasswordManager.AffiliationDummyData.Startup_RequestSuccess", success);
On 2015/04/21 20:23:16, engedy wrote:
> On 2015/04/21 20:11:27, Ilya Sherman wrote:
> > Optional nit: It seems a little weird to mix "." and "_" as separators.  You
> can
> > keep this as-is if you want to, but I'd recommend using "." throughout for
> > consistency.  Note that if you're choosing "_" to work with the field_trial
> > mechanism, you can actually specify the separator in the histograms.xml
entry,
> > as separator="."
> 
> I have changed the separator here, and added the attribute into
histograms.xml. 
> 
> Not sure I understand what you mean by "if you're choosing "_" to work with
the
> field_trial mechanism" -- could you please elaborate on that?

Sorry, I meant "histogram_suffixes" -- it used to be called "field_trials", and
I slipped into the old (and now very confusing) jargon.

[engedy, 2015-04-22 08:29:47]

Mike, do you want to take another look at this? There have been a couple of
changes.

[Mike West, 2015-04-22 09:59:21]

Still LGTM % nits.

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:63: bool
still_early_after_startup,
Perhaps naming this "OnStartup" would be easier to understand?

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:273:
&early_verification_timer_, base::TimeDelta::FromMinutes(1), true);
Nit: I know Chromium does this everywhere, but enums are so much more
descriptive than booleans. ON_STARTUP vs PERIODIC would be a significant
improvement over `true` and `false`.

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.h (right):

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.h:135: //
indicate if it |is_still_early_after_startup|.
What does "still early" mean in this context?

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.h:144: bool
is_still_early_after_startup);
Nit: `is_first_check`?

[engedy, 2015-04-22 11:59:30]

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:63: bool
still_early_after_startup,
On 2015/04/22 09:59:21, Mike West wrote:
> Perhaps naming this "OnStartup" would be easier to understand?

Done.

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:273:
&early_verification_timer_, base::TimeDelta::FromMinutes(1), true);
On 2015/04/22 09:59:21, Mike West wrote:
> Nit: I know Chromium does this everywhere, but enums are so much more
> descriptive than booleans. ON_STARTUP vs PERIODIC would be a significant
> improvement over `true` and `false`.

Done.

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.h (right):

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.h:135: //
indicate if it |is_still_early_after_startup|.
On 2015/04/22 09:59:21, Mike West wrote:
> What does "still early" mean in this context?

Replaced with enum.

https://codereview.chromium.org/1090163004/diff/60001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.h:144: bool
is_still_early_after_startup);
On 2015/04/22 09:59:21, Mike West wrote:
> Nit: `is_first_check`?

Replaced with enum.

[engedy, 2015-04-22 11:59:54]

The CQ bit was checked by engedy@chromium.org

[engedy, 2015-04-22 11:59:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1090163004/#ps100001
(title: "Comment phrasing.")

[commit-bot: I haz the power, 2015-04-22 12:00:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1090163004/100001

[commit-bot: I haz the power, 2015-04-22 13:23:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-22 13:24:31]

Patchset 6 (id:??) landed as
https://crrev.com/c096020684b111a8ba7cf94f549841afdb0c96d1
Cr-Commit-Position: refs/heads/master@{#326284}

[oystein (OOO til 10th of July), 2015-04-22 21:09:10]

oysteine@chromium.org changed reviewers:
+ oysteine@chromium.org

[oystein (OOO til 10th of July), 2015-04-22 21:09:11]

Reverted in https://codereview.chromium.org/1065773006 due to crbug.com/479841