Modify rappor parameters.

Modify rappor parameters.

Change the default parameters for ETLD+1 Rappor metrics.
Allow number of cohorts to be set per metric.
Modify metric names to distinguish from reports with old parameters.

BUG=370599
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273100

[Alexei Svitkine (slow), 2014-05-20 06:59:49]

https://codereview.chromium.org/264123004/diff/20001/components/rappor/byte_v...
File components/rappor/byte_vector_utils.cc (right):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/byte_v...
components/rappor/byte_vector_utils.cc:83: for (size_t i = 0, len = lhs.size();
i < len; ++i) {
Nit: Remove len and just do i < lhs.size(). (Compiler should be inlining
lhs.size() in already.)

https://codereview.chromium.org/264123004/diff/20001/components/rappor/proto/...
File components/rappor/proto/rappor_metric.proto (right):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/proto/...
components/rappor/proto/rappor_metric.proto:23: // generated randomly by the
client and is currently in the range [0,512).
Hmm, should we mention when this changed? (i.e. Chrome versions)

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
File components/rappor/rappor_parameters.h (right):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
components/rappor/rappor_parameters.h:27: static const int kMaxCohorts = 512;
I'm not sure that this syntax is supported by all our compilers. You might need
to set the value in the .cc file instead.

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
File components/rappor/rappor_pref_names.cc (left):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
components/rappor/rappor_pref_names.cc:11: const char kRapporCohort[] =
"rappor.cohort";
Keep the old pref and add code to Clear it, so that clients' prefs won't be
littered with old prefs. Then, after a few releases, remove code related to it.

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
File components/rappor/rappor_service.cc (right):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
components/rappor/rappor_service.cc:13: #include "base/sys_info.h"
What's this for?

[Steven Holte, 2014-05-20 18:28:03]

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
File components/rappor/rappor_pref_names.cc (left):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
components/rappor/rappor_pref_names.cc:11: const char kRapporCohort[] =
"rappor.cohort";
On 2014/05/20 06:59:49, Alexei Svitkine wrote:
> Keep the old pref and add code to Clear it, so that clients' prefs won't be
> littered with old prefs. Then, after a few releases, remove code related to
it.

Is there a reliable pattern for doing a one time Clear of data, so that I can
ensure this only gets re-rolled once?

[Steven Holte, 2014-05-20 22:53:53]

https://codereview.chromium.org/264123004/diff/20001/components/rappor/byte_v...
File components/rappor/byte_vector_utils.cc (right):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/byte_v...
components/rappor/byte_vector_utils.cc:83: for (size_t i = 0, len = lhs.size();
i < len; ++i) {
On 2014/05/20 06:59:49, Alexei Svitkine wrote:
> Nit: Remove len and just do i < lhs.size(). (Compiler should be inlining
> lhs.size() in already.)

Done.

https://codereview.chromium.org/264123004/diff/20001/components/rappor/proto/...
File components/rappor/proto/rappor_metric.proto (right):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/proto/...
components/rappor/proto/rappor_metric.proto:23: // generated randomly by the
client and is currently in the range [0,512).
On 2014/05/20 06:59:49, Alexei Svitkine wrote:
> Hmm, should we mention when this changed? (i.e. Chrome versions)

Not sure how to predict the specific chrome version this lands in.  Is M37
specific enough?

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
File components/rappor/rappor_parameters.h (right):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
components/rappor/rappor_parameters.h:27: static const int kMaxCohorts = 512;
On 2014/05/20 06:59:49, Alexei Svitkine wrote:
> I'm not sure that this syntax is supported by all our compilers. You might
need
> to set the value in the .cc file instead.

Searching turned up the STATIC_CONST_MEMBER_DEFINITION macro which has the
following comment:
// The C++ standard requires that static const members have an out-of-class
// definition (in a single compilation unit), but MSVC chokes on this (when
// language extensions, which are required, are enabled). (You're only likely to
// notice the need for a definition if you take the address of the member or,
// more commonly, pass it to a function that takes it as a reference argument --
// probably an STL function.) This macro makes MSVC do the right thing. See
// http://msdn.microsoft.com/en-us/library/34h23df8(v=vs.100).aspx for more
// information. Use like:
//
// In .h file:
//   struct Foo {
//     static const int kBar = 5;
//   };
//
// In .cc file:
//   STATIC_CONST_MEMBER_DEFINITION const int Foo::kBar;

So I've added the definition to the .cc, but left the value here so it can be
inlined by the compiler.

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
File components/rappor/rappor_service.cc (right):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
components/rappor/rappor_service.cc:13: #include "base/sys_info.h"
On 2014/05/20 06:59:49, Alexei Svitkine wrote:
> What's this for?

Not used in this revision.  Removed.

[Alexei Svitkine (slow), 2014-05-21 09:13:36]

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
File components/rappor/rappor_pref_names.cc (left):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
components/rappor/rappor_pref_names.cc:11: const char kRapporCohort[] =
"rappor.cohort";
On 2014/05/20 18:28:03, Steven Holte wrote:
> On 2014/05/20 06:59:49, Alexei Svitkine wrote:
> > Keep the old pref and add code to Clear it, so that clients' prefs won't be
> > littered with old prefs. Then, after a few releases, remove code related to
> it.
> 
> Is there a reliable pattern for doing a one time Clear of data, so that I can
> ensure this only gets re-rolled once?

Renaming the pref key the correct way to do this, but also keeping an entry for
the old name, i.e.:

const char kRapporCohortSeed[] = "rappor.cohort_seed";
// comment about it:
const char kRapporCohortDeprecated[] = "rappor.cohort";

And in your code that reads the new pref also clear the old pref.

See also metrics_state_manager.cc which does this for some prefs (e.g.
kMetricsOldLowEntropySource).

https://codereview.chromium.org/264123004/diff/60001/components/rappor/rappor...
File components/rappor/rappor_parameters.cc (right):

https://codereview.chromium.org/264123004/diff/60001/components/rappor/rappor...
components/rappor/rappor_parameters.cc:23: STATIC_CONST_MEMBER_DEFINITION const
int RapporParameters::kMaxCohorts;
Hmm, I'd prefer if this just used the more standard syntax of:

.cc file:

// static
bool MetricsStateManager::kMaxCohorts = 512;

header file:

static const int kMaxCohorts;

[Steven Holte, 2014-05-21 19:02:56]

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
File components/rappor/rappor_pref_names.cc (left):

https://codereview.chromium.org/264123004/diff/20001/components/rappor/rappor...
components/rappor/rappor_pref_names.cc:11: const char kRapporCohort[] =
"rappor.cohort";
On 2014/05/21 09:13:37, Alexei Svitkine wrote:
> On 2014/05/20 18:28:03, Steven Holte wrote:
> > On 2014/05/20 06:59:49, Alexei Svitkine wrote:
> > > Keep the old pref and add code to Clear it, so that clients' prefs won't
be
> > > littered with old prefs. Then, after a few releases, remove code related
to
> > it.
> > 
> > Is there a reliable pattern for doing a one time Clear of data, so that I
can
> > ensure this only gets re-rolled once?
> 
> Renaming the pref key the correct way to do this, but also keeping an entry
for
> the old name, i.e.:
> 
> const char kRapporCohortSeed[] = "rappor.cohort_seed";
> // comment about it:
> const char kRapporCohortDeprecated[] = "rappor.cohort";
> 
> And in your code that reads the new pref also clear the old pref.
> 
> See also metrics_state_manager.cc which does this for some prefs (e.g.
> kMetricsOldLowEntropySource).

Done. PTAL.

https://codereview.chromium.org/264123004/diff/60001/components/rappor/rappor...
File components/rappor/rappor_parameters.cc (right):

https://codereview.chromium.org/264123004/diff/60001/components/rappor/rappor...
components/rappor/rappor_parameters.cc:23: STATIC_CONST_MEMBER_DEFINITION const
int RapporParameters::kMaxCohorts;
On 2014/05/21 09:13:37, Alexei Svitkine wrote:
> Hmm, I'd prefer if this just used the more standard syntax of:
> 
> .cc file:
> 
> // static
> bool MetricsStateManager::kMaxCohorts = 512;
> 
> header file:
> 
> static const int kMaxCohorts;

Done.

[Alexei Svitkine (slow), 2014-05-22 13:17:11]

code changes LGTM % nit

Maybe have Ulfar or someone else familiar with the intention of the change
review for the logic?

https://codereview.chromium.org/264123004/diff/80001/components/rappor/rappor...
File components/rappor/rappor_parameters.h (right):

https://codereview.chromium.org/264123004/diff/80001/components/rappor/rappor...
components/rappor/rappor_parameters.h:28: // The number of cohorts to divide the
reports for this metric into.
Nit: Empty line before this please.

[Steven Holte, 2014-05-22 20:02:26]

+felt@ for chrome/browser/extensions/activity_log/activity_actions.cc
+battre@ chrome/browser/prefs/pref_metrics_service.cc

Both of these files just change the name of the metric so we can distinguish
them from reports with the old parameter values.

+ulfar@ for review of the new parameter logic

[battre, 2014-05-22 20:18:17]

chrome/browser/prefs/pref_metrics_service.cc LGTM

[felt, 2014-05-22 23:30:01]

On 2014/05/22 20:18:17, battre wrote:
> chrome/browser/prefs/pref_metrics_service.cc LGTM

lgtm, cc'ing devlin so that he knows about this

[Devlin, 2014-05-22 23:36:44]

https://codereview.chromium.org/264123004/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/264123004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_actions.cc:49:
"Extensions.PossibleAdInjection2";
Nit: please add a short comment stating why this is "2", so the next person to
wonder doesn't have to look up in the revision history.

[Steven Holte, 2014-05-23 21:54:04]

https://codereview.chromium.org/264123004/diff/80001/components/rappor/rappor...
File components/rappor/rappor_parameters.h (right):

https://codereview.chromium.org/264123004/diff/80001/components/rappor/rappor...
components/rappor/rappor_parameters.h:28: // The number of cohorts to divide the
reports for this metric into.
On 2014/05/22 13:17:11, Alexei Svitkine wrote:
> Nit: Empty line before this please.

Done.

https://codereview.chromium.org/264123004/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/264123004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_actions.cc:49:
"Extensions.PossibleAdInjection2";
On 2014/05/22 23:36:45, Devlin wrote:
> Nit: please add a short comment stating why this is "2", so the next person to
> wonder doesn't have to look up in the revision history.

Done.

[Devlin, 2014-05-23 22:04:19]

lgtm, thanks. :)

[ulfar, 2014-05-27 17:52:13]

https://codereview.chromium.org/264123004/diff/120001/components/rappor/rappo...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/264123004/diff/120001/components/rappor/rappo...
components/rappor/rappor_metric.cc:18: (cohort_seed % parameters.num_cohorts) *
This is the only place where I see parameters.num_cohorts being used.  In
particular, I didn't see anyplace where the actual cohort number (mod
num_cohorts) is being reported to the service.  Is the cohort_seed now being
reported to the service, and the service is expected to do the modulus before
analysis?

[Steven Holte, 2014-05-27 17:58:42]

https://codereview.chromium.org/264123004/diff/120001/components/rappor/rappo...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/264123004/diff/120001/components/rappor/rappo...
components/rappor/rappor_metric.cc:18: (cohort_seed % parameters.num_cohorts) *
On 2014/05/27 17:52:14, ulfar wrote:
> This is the only place where I see parameters.num_cohorts being used.  In
> particular, I didn't see anyplace where the actual cohort number (mod
> num_cohorts) is being reported to the service.  Is the cohort_seed now being
> reported to the service, and the service is expected to do the modulus before
> analysis?

Yes, it's reporting the seed, and we'll just aggregate over the modulus in the
analysis.

[ulfar, 2014-05-27 18:49:50]

lgtm

LGTM.

These parameters offer better utility, as with about a 100 million reports from
different clients we should get to the detection threshold for 0.1% frequency. 
Each client is still protected by epsilon-differential privacy, and a huge false
discovery rate.

With the old parameters, significantly more than one billion RAPPOR reports
would have to be required to detect a homepage domain whose frequency was more
than 0.1% (i.e., present on one in a thousand clients).

[Steven Holte, 2014-05-27 18:51:54]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-05-27 18:52:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/holte@chromium.org/264123004/120001

[commit-bot: I haz the power, 2014-05-27 23:27:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-28 01:22:03]

Change committed as 273100