Add support for custom subresource filter InstallerAttributes.

Add support for custom subresource filter InstallerAttributes.

BUG=686765
Review-Url: https://codereview.chromium.org/2665473002
Cr-Commit-Position: refs/heads/master@{#448738}
Committed: https://chromium.googlesource.com/chromium/src/+/45a965f8519020883364af56fc28b8164bd5751a

[Bryan McQuade, 2017-01-27 22:15:15]

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

[commit-bot: I haz the power, 2017-01-27 22:16:32]

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-27 22:56:44]

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

[commit-bot: I haz the power, 2017-01-27 22:56:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bryan McQuade, 2017-01-29 22:46:20]

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

[commit-bot: I haz the power, 2017-01-29 22:46:36]

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-29 23:30:35]

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

[commit-bot: I haz the power, 2017-01-29 23:30:36]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-01-30 16:05:44]

bmcquade@chromium.org changed reviewers:
+ engedy@chromium.org, waffles@chromium.org

[Bryan McQuade, 2017-01-30 16:09:04]

Description was changed from

==========
Add support for custom subresource filter InstallerAttributes.
==========

to

==========
Add support for custom subresource filter InstallerAttributes.

BUG=686765
==========

[Bryan McQuade, 2017-01-30 16:09:30]

PTAL. This is a first cut at adding support for custom subresource filtering
configurable by a field trial param. Let me know if this looks right to you, or
if anything needs to change. Thanks!

[engedy, 2017-01-30 17:51:23]

LGTM % comments below.

https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/subresource_filter_component_installer.cc:122:
std::string installer_tag = variations::GetVariationParamValueByFeature(
Could you please factor out lines 122--124 to a global function in
components/subresource_filter/core/browser/subresource_filter_features.h?

I'm planning some refactoring there, and I'd like to avoid call sites elsewhere
depending on what exact feature/field trial configuration supply these parameter
values.

https://codereview.chromium.org/2665473002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2665473002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:34:
const char kInstallerAttributeTagParameterName[] = "installer_attribute_tag";
Phrasing nit: The term `installer attribute tag` is very generic and not very
informative. What do you think about naming this something along the lines of
`ruleset flavor tag`?

https://codereview.chromium.org/2665473002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2665473002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:5:
#ifndef
COMPONENTS_SUBRESOURCE_FILTER_CORE_BROWSER_SUBRESOURCE_FILTER_FEATURES_H_
Thanks!

[waffles, 2017-01-30 18:58:23]

https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/subresource_filter_component_installer.cc:126:
attributes["tag"] = installer_tag;
For privacy reasons, we prefer for you to not to tag the variation parameter
value directly, as you could then use this Finch experiment to arbitrarily
divide the population of Chrome users into small enough buckets that they could
be tracked.

It is also difficult for non-Googlers who read this source to then determine all
the possible values that the browser may be sending back to Google.

If possible, please specify a small number of fixed strings in the source, and
use the value to choose which of those strings to transmit.

[Bryan McQuade, 2017-01-30 19:18:42]

Thanks! One comment re: picking tag names in advance.

https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/subresource_filter_component_installer.cc:126:
attributes["tag"] = installer_tag;
On 2017/01/30 at 18:58:23, waffles wrote:
> For privacy reasons, we prefer for you to not to tag the variation parameter
value directly, as you could then use this Finch experiment to arbitrarily
divide the population of Chrome users into small enough buckets that they could
be tracked.
> 
> It is also difficult for non-Googlers who read this source to then determine
all the possible values that the browser may be sending back to Google.
> 
> If possible, please specify a small number of fixed strings in the source, and
use the value to choose which of those strings to transmit.

Ah, that's interesting, thanks for pointing this out!

Unfortunately, we don't know the number of possible values here in advance. I
expect that we will need to rev the payload multiple times as we try and learn
during the field trial. We won't target small user populations, but we'll want a
new tag each time we rev, and I'm not at all sure what number of tags to expect.
I'm concerned about picking a fixed amount now given the very high delay
incurred if we end up needing to change it later (e.g. we decide we want to rev
the payload but have used up all identifiers and are already on stable channel).

Is there a way to address this concern with small bucket sizes at field trial
configuration time? I can make sure you or someone representing this concern is
on all relevant field trial code reviews, for instance, if that would help. We
have no plans to run small field trials, so this shouldn't be a problem.

Happy to chat over VC, and then summarize what we discuss on thread, if you
think that's a more efficient way to proceed with the discussion here.

[waffles, 2017-01-30 19:30:24]

On 2017/01/30 19:18:42, Bryan McQuade wrote:
> Thanks! One comment re: picking tag names in advance.
> 
>
https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
> File
chrome/browser/component_updater/subresource_filter_component_installer.cc
> (right):
> 
>
https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
>
chrome/browser/component_updater/subresource_filter_component_installer.cc:126:
> attributes["tag"] = installer_tag;
> On 2017/01/30 at 18:58:23, waffles wrote:
> > For privacy reasons, we prefer for you to not to tag the variation parameter
> value directly, as you could then use this Finch experiment to arbitrarily
> divide the population of Chrome users into small enough buckets that they
could
> be tracked.
> > 
> > It is also difficult for non-Googlers who read this source to then determine
> all the possible values that the browser may be sending back to Google.
> > 
> > If possible, please specify a small number of fixed strings in the source,
and
> use the value to choose which of those strings to transmit.
> 
> Ah, that's interesting, thanks for pointing this out!
> 
> Unfortunately, we don't know the number of possible values here in advance. I
> expect that we will need to rev the payload multiple times as we try and learn
> during the field trial. We won't target small user populations, but we'll want
a
> new tag each time we rev, and I'm not at all sure what number of tags to
expect.
> I'm concerned about picking a fixed amount now given the very high delay
> incurred if we end up needing to change it later (e.g. we decide we want to
rev
> the payload but have used up all identifiers and are already on stable
channel).
> 
> Is there a way to address this concern with small bucket sizes at field trial
> configuration time? I can make sure you or someone representing this concern
is
> on all relevant field trial code reviews, for instance, if that would help. We
> have no plans to run small field trials, so this shouldn't be a problem.
> 
> Happy to chat over VC, and then summarize what we discuss on thread, if you
> think that's a more efficient way to proceed with the discussion here.

Are you certain you need a new tag every time you rev? I assumed your experiment
was something along these lines: you are going to start out with two binaries:
control_v1 (C1) and experiment_v1 (E1), and divide your population into Control
(say 2%), Experiment (2%) and Default (96%). Time passes, and you produce C2
and/or E2. Now you have a choice - are you going to move population from Default
into new groups Control2 and Experiment2, or ship C2 and E2 to your existing
Control and Experiment groups?

If the latter, the component updater need only communicate whether the browser
is in the control or experimental branch; and Omaha will be responsible for
giving the latest version of your control or experimental binary to the browser,
so you should only need two or three tag values.

If, however, you're going to pull new users out of Default to compare both to
each other and to your original Control and Experiment buckets (who are still
running C1 and E1), you will need more tags. But you can only do that so many
times before you exhaust the 96% of users you have in Default, so depending on
the size of your experimental groups the number of possible tags might still be
small.

Does that make any sense? Happy to chat over VC if it helps.

[Bryan McQuade, 2017-01-31 15:00:35]

On 2017/01/30 at 19:30:24, waffles wrote:
> On 2017/01/30 19:18:42, Bryan McQuade wrote:
> > Thanks! One comment re: picking tag names in advance.
> > 
> >
https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
> > File
chrome/browser/component_updater/subresource_filter_component_installer.cc
> > (right):
> > 
> >
https://codereview.chromium.org/2665473002/diff/20001/chrome/browser/componen...
> >
chrome/browser/component_updater/subresource_filter_component_installer.cc:126:
> > attributes["tag"] = installer_tag;
> > On 2017/01/30 at 18:58:23, waffles wrote:
> > > For privacy reasons, we prefer for you to not to tag the variation
parameter
> > value directly, as you could then use this Finch experiment to arbitrarily
> > divide the population of Chrome users into small enough buckets that they
could
> > be tracked.
> > > 
> > > It is also difficult for non-Googlers who read this source to then
determine
> > all the possible values that the browser may be sending back to Google.
> > > 
> > > If possible, please specify a small number of fixed strings in the source,
and
> > use the value to choose which of those strings to transmit.
> > 
> > Ah, that's interesting, thanks for pointing this out!
> > 
> > Unfortunately, we don't know the number of possible values here in advance.
I
> > expect that we will need to rev the payload multiple times as we try and
learn
> > during the field trial. We won't target small user populations, but we'll
want a
> > new tag each time we rev, and I'm not at all sure what number of tags to
expect.
> > I'm concerned about picking a fixed amount now given the very high delay
> > incurred if we end up needing to change it later (e.g. we decide we want to
rev
> > the payload but have used up all identifiers and are already on stable
channel).
> > 
> > Is there a way to address this concern with small bucket sizes at field
trial
> > configuration time? I can make sure you or someone representing this concern
is
> > on all relevant field trial code reviews, for instance, if that would help.
We
> > have no plans to run small field trials, so this shouldn't be a problem.
> > 
> > Happy to chat over VC, and then summarize what we discuss on thread, if you
> > think that's a more efficient way to proceed with the discussion here.
> 
> Are you certain you need a new tag every time you rev? I assumed your
experiment was something along these lines: you are going to start out with two
binaries: control_v1 (C1) and experiment_v1 (E1), and divide your population
into Control (say 2%), Experiment (2%) and Default (96%). Time passes, and you
produce C2 and/or E2. Now you have a choice - are you going to move population
from Default into new groups Control2 and Experiment2, or ship C2 and E2 to your
existing Control and Experiment groups?
> 
> If the latter, the component updater need only communicate whether the browser
is in the control or experimental branch; and Omaha will be responsible for
giving the latest version of your control or experimental binary to the browser,
so you should only need two or three tag values.
> 
> If, however, you're going to pull new users out of Default to compare both to
each other and to your original Control and Experiment buckets (who are still
running C1 and E1), you will need more tags. But you can only do that so many
times before you exhaust the 96% of users you have in Default, so depending on
the size of your experimental groups the number of possible tags might still be
small.
> 
> Does that make any sense? Happy to chat over VC if it helps.

Thanks! The field trial guidelines say to change experiment arm names every time
you change anything about the experiment config ("Change ... trial names any
time you touch study sizing or behavior.) so I'd want to rename groups each time
we push a new payload.

RE: re-using tags with new binary payloads, I'm curious to understand how
caching works end to end and whether we can be certain that re-using a tag for a
new payload would cause the client to update to the new payload with certainty.
I've definitely been burned by revving resources w/o changing URLs or configs in
the past, and am in general very hesitant to make assumptions like this unless
there's a really critical reason to do so.

I'll set up some time to VC. I definitely want to make sure we're addressing
privacy concerns, but I also don't want to make changes that may compromise the
outcome of the experiment. In general, guidance from metrics/field trial teams
is to always rev group names and associated identifiers each time the experiment
changes, to avoid any risks of caching or other subtle issues. Experiments in
particular are very difficult to reason about when things go wrong, so it's
strongly preferred to change names etc to be certain that new experiment arms
are getting the updated group configs, rather than an old cached version with
the same tag, for instance.

[Bryan McQuade, 2017-02-01 18:27:13]

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

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

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

[Bryan McQuade, 2017-02-01 18:29:33]

Thanks! I've addressed comments. Joshua, one question about choice of string
used in the installer attributes.

Joshua I'll book us some time to VC later this week, thanks!

https://codereview.chromium.org/2665473002/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2665473002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/subresource_filter_component_installer.cc:123:
attributes["tag"] = ruleset_flavor;
joshua, do we need to use the string "tag" here? Could we use a different
string, such as "ruleset_flavor" instead?

[waffles, 2017-02-01 18:33:33]

https://codereview.chromium.org/2665473002/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2665473002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/subresource_filter_component_installer.cc:123:
attributes["tag"] = ruleset_flavor;
On 2017/02/01 18:29:33, Bryan McQuade wrote:
> joshua, do we need to use the string "tag" here? Could we use a different
> string, such as "ruleset_flavor" instead?

A different string is fine; you will be able to configure the server to respond
to any string. The server will log the value if and only if the string is "tag",
so if you want to be able to split your Omaha metrics by this attribute you
should use "tag". But if you don't need or use Omaha metrics feel free to pick a
different string.

[Bryan McQuade, 2017-02-01 18:36:29]

https://codereview.chromium.org/2665473002/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2665473002/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/subresource_filter_component_installer.cc:123:
attributes["tag"] = ruleset_flavor;
On 2017/02/01 at 18:33:32, waffles wrote:
> On 2017/02/01 18:29:33, Bryan McQuade wrote:
> > joshua, do we need to use the string "tag" here? Could we use a different
> > string, such as "ruleset_flavor" instead?
> 
> A different string is fine; you will be able to configure the server to
respond to any string. The server will log the value if and only if the string
is "tag", so if you want to be able to split your Omaha metrics by this
attribute you should use "tag". But if you don't need or use Omaha metrics feel
free to pick a different string.

ah, great, thank you! in that case, i'll stick with 'tag' since we get the added
benefits of logging. thanks!

[commit-bot: I haz the power, 2017-02-01 19:10:21]

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

[commit-bot: I haz the power, 2017-02-01 19:10:21]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-02-07 12:06:07]

Description was changed from

==========
Add support for custom subresource filter InstallerAttributes.

BUG=686765
==========

to

==========
Add support for custom subresource filter InstallerAttributes.

BUG=686765
==========

[Bryan McQuade, 2017-02-07 19:56:25]

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

[commit-bot: I haz the power, 2017-02-07 19:57:18]

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

[Bryan McQuade, 2017-02-07 20:36:43]

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

[commit-bot: I haz the power, 2017-02-07 20:37:24]

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

[Bryan McQuade, 2017-02-07 20:38:49]

Joshua and I got to sync on the outstanding issue w/ number of potential tags.
Thanks Joshua for taking the time to meet!

Joshua helped me to understand that there weren't any caching risks with tag
re-use, so I've updated this change to limit us to 4 tags: a, b, c, d. We
most likely only need 2, but I wanted to leave a little more room in case we 
want to run a third or fourth variant of the experiment concurrently.

Joshua, PTAL when you have a chance, thanks!

[waffles, 2017-02-07 20:41:49]

component_updater lgtm

[commit-bot: I haz the power, 2017-02-07 21:27:57]

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

[commit-bot: I haz the power, 2017-02-07 21:27:58]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-07 21:46:19]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-02-07 21:46:19]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/2665473002/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2017-02-07 21:47:12]

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-02-07 22:16:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486503979173140,
"parent_rev": "255e749e57448c43f5f9f49ba0bf842cd92a6b69", "commit_rev":
"45a965f8519020883364af56fc28b8164bd5751a"}

[commit-bot: I haz the power, 2017-02-07 22:16:57]

Description was changed from

==========
Add support for custom subresource filter InstallerAttributes.

BUG=686765
==========

to

==========
Add support for custom subresource filter InstallerAttributes.

BUG=686765

Review-Url: https://codereview.chromium.org/2665473002
Cr-Commit-Position: refs/heads/master@{#448738}
Committed:
https://chromium.googlesource.com/chromium/src/+/45a965f8519020883364af56fc28...
==========

[commit-bot: I haz the power, 2017-02-07 22:16:58]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/45a965f8519020883364af56fc28...