Create a mechanism for reporting rappor metrics from non-UMA

Create a mechanism for reporting rappor metrics from non-UMA
users.

BUG=418242
Committed: https://crrev.com/ee8e70627d8d9ed12f3c92763dc2bf2fc7084600
Cr-Commit-Position: refs/heads/master@{#302682}

[Steven Holte, 2014-08-28 00:24:07]

holte@chromium.org changed reviewers:
+ jwd@chromium.org

[Steven Holte, 2014-08-28 00:24:07]

This is just a rough draft, but FYI.

[jwd, 2014-08-29 15:45:47]

https://codereview.chromium.org/509203002/diff/1/components/rappor/rappor_par...
File components/rappor/rappor_parameters.h (right):

https://codereview.chromium.org/509203002/diff/1/components/rappor/rappor_par...
components/rappor/rappor_parameters.h:48: // to be enabled.
The comment seems backwards to me. Like true means it requires all metric
reporting to be enabled.

Maybe something like:
Whether this metric can be reported when all rappor metric reporting isn't
enabled.

https://codereview.chromium.org/509203002/diff/1/components/rappor/rappor_ser...
File components/rappor/rappor_service.cc (right):

https://codereview.chromium.org/509203002/diff/1/components/rappor/rappor_ser...
components/rappor/rappor_service.cc:57: kRapporRolloutRequireUmaParam) !=
"False";
Super good, awesome choice for the condition!

[vpihur, 2014-09-12 17:42:20]

vpihur@google.com changed reviewers:
+ vpihur@google.com

[vpihur, 2014-09-12 17:42:20]

Because of some recent discussions on anonymity and tracking, we decided to use
a much smaller Bloom filter and a large number of cohorts.

This makes tracking of users overtime using their report data harder, yet
provides equally good utility. 

The proposed values at this time are 8 bits Bloom filters and 128 cohorts.

[Steven Holte, 2014-09-19 22:48:47]

Updated the parameter to 8 bits 128 cohorts.

https://codereview.chromium.org/509203002/diff/1/components/rappor/rappor_par...
File components/rappor/rappor_parameters.h (right):

https://codereview.chromium.org/509203002/diff/1/components/rappor/rappor_par...
components/rappor/rappor_parameters.h:48: // to be enabled.
On 2014/08/29 15:45:47, Jesse Doherty wrote:
> The comment seems backwards to me. Like true means it requires all metric
> reporting to be enabled.
> 
> Maybe something like:
> Whether this metric can be reported when all rappor metric reporting isn't
> enabled.

Done.

[Steven Holte, 2014-09-26 22:04:10]

jwd@, this should be ready for review now.  PTAL.

[jwd, 2014-10-03 19:53:03]

LGTM with one comment.

https://codereview.chromium.org/509203002/diff/40001/components/rappor/rappor...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/509203002/diff/40001/components/rappor/rappor...
components/rappor/rappor_metric.cc:24: DCHECK_EQ(RapporParameters::kMaxCohorts %
parameters.num_cohorts, 0);
Can you add a comment about this in rappor_parametrs.h. And maybe a note here
about it. I don't see why this is the case.

[Steven Holte, 2014-10-03 20:09:22]

holte@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Steven Holte, 2014-10-03 20:09:23]

+asvitkine for OWNERs

https://codereview.chromium.org/509203002/diff/40001/components/rappor/rappor...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/509203002/diff/40001/components/rappor/rappor...
components/rappor/rappor_metric.cc:24: DCHECK_EQ(RapporParameters::kMaxCohorts %
parameters.num_cohorts, 0);
On 2014/10/03 19:53:03, Jesse Doherty wrote:
> Can you add a comment about this in rappor_parametrs.h. And maybe a note here
> about it. I don't see why this is the case. 

Done.

[Alexei Svitkine (slow), 2014-10-03 20:28:51]

I assume it's already been discussed, but can you get someone from privacy team
to sign off on the crbug, so that it's clear this has been approved from their
end? Thanks!

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

https://codereview.chromium.org/509203002/diff/60001/components/rappor/rappor...
components/rappor/rappor_metric.cc:26: DCHECK_EQ(RapporParameters::kMaxCohorts %
parameters.num_cohorts, 0);
Nit: Expected value goes on the left for DCHECK_EQ.

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

https://codereview.chromium.org/509203002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.cc:90: prefs::kRapporLastDailySample,
Nit: Align.

https://codereview.chromium.org/509203002/diff/60001/components/rappor/rappor...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/509203002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:126: bool all_metrics_enabled_;
You need to initialize this in the ctor.

[Steven Holte, 2014-10-03 21:41:11]

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

https://codereview.chromium.org/509203002/diff/60001/components/rappor/rappor...
components/rappor/rappor_metric.cc:26: DCHECK_EQ(RapporParameters::kMaxCohorts %
parameters.num_cohorts, 0);
On 2014/10/03 20:28:50, Alexei Svitkine wrote:
> Nit: Expected value goes on the left for DCHECK_EQ.

Done.

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

https://codereview.chromium.org/509203002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.cc:90: prefs::kRapporLastDailySample,
On 2014/10/03 20:28:50, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/509203002/diff/60001/components/rappor/rappor...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/509203002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:126: bool all_metrics_enabled_;
On 2014/10/03 20:28:50, Alexei Svitkine wrote:
> You need to initialize this in the ctor.

Done.

[Alexei Svitkine (slow), 2014-11-03 21:48:21]

Thanks. I'm wondering if hodie@ is in the loop for this plan?

Maybe I'm not in the loop, but my understanding from last meeting where this was
discussed was that we wanted to explore UI treatments that could offer an
opt-out for RAPPOR before turning it on for everyone.

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
File components/rappor/rappor_service.cc (right):

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
components/rappor/rappor_service.cc:95: bool metrics_enabled) {
Can you document the 2nd param better in the header file?

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
components/rappor/rappor_service.cc:214: if (!all_metrics_enabled_ &&
!parameters.always_enabled)
Can you add a test for this?

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
components/rappor/rappor_service.h:125: // True if we are collecting all metrics
instead of only whitelisted ones.
Can you expand this comment to give a concrete use case? (e.g. "All metrics are
enabled for UMA users, while whitelisted metrics are enabled for all users.",
for example).

https://codereview.chromium.org/509203002/diff/100001/tools/metrics/rappor/ra...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/509203002/diff/100001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:21: components/rappor/rappor_service.cc.
Can you also file a bug that we should make a presubmit that checks the two are
the same?

https://codereview.chromium.org/509203002/diff/100001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:45: chrome users.
Nit: capitalize Chrome

[Steven Holte, 2014-11-04 02:44:20]

hodie@ should be aware of this.  I've tweaked this change a bit, which should
make it even more "infrastructural".

I've made ReportingLevel enum that determines the level of metrics that are
reported, which can be REPORTING_DISABLED, COARSE_LEVEL, or FINE_LEVEL metrics. 
The metrics are then tagged as coarse or fine and we just report the metrics
that are below the current reporting level.  Currently UMA being on or off will
change the metrics between DISABLED/FINE.  We can figure out what options get
you to "COARSE" later.  This should enable us to start adding these metrics, but
will only send them for UMA enabled users for now.

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
File components/rappor/rappor_service.cc (right):

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
components/rappor/rappor_service.cc:95: bool metrics_enabled) {
On 2014/11/03 21:48:21, Alexei Svitkine wrote:
> Can you document the 2nd param better in the header file?

Done.

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
components/rappor/rappor_service.cc:214: if (!all_metrics_enabled_ &&
!parameters.always_enabled)
On 2014/11/03 21:48:21, Alexei Svitkine wrote:
> Can you add a test for this?

Done.

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/509203002/diff/100001/components/rappor/rappo...
components/rappor/rappor_service.h:125: // True if we are collecting all metrics
instead of only whitelisted ones.
On 2014/11/03 21:48:21, Alexei Svitkine wrote:
> Can you expand this comment to give a concrete use case? (e.g. "All metrics
are
> enabled for UMA users, while whitelisted metrics are enabled for all users.",
> for example).

Replaced this with a ReportingLevel.

https://codereview.chromium.org/509203002/diff/100001/tools/metrics/rappor/ra...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/509203002/diff/100001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:21: components/rappor/rappor_service.cc.
On 2014/11/03 21:48:21, Alexei Svitkine wrote:
> Can you also file a bug that we should make a presubmit that checks the two
are
> the same?

Done. https://code.google.com/p/chromium/issues/detail?id=429923

https://codereview.chromium.org/509203002/diff/100001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:45: chrome users.
On 2014/11/03 21:48:21, Alexei Svitkine wrote:
> Nit: capitalize Chrome

Changed this description.

[Alexei Svitkine (slow), 2014-11-04 15:39:24]

LGTM

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

https://codereview.chromium.org/509203002/diff/120001/components/rappor/rappo...
components/rappor/rappor_service.cc:107: if (reporting_level <=
REPORTING_DISABLED) {
Nit: No {}'s

https://codereview.chromium.org/509203002/diff/120001/components/rappor/rappo...
components/rappor/rappor_service.cc:193: };
Nit: Remove trailing ;

[Steven Holte, 2014-11-04 18:57:21]

holte@chromium.org changed reviewers:
+ felt@chromium.org

[Steven Holte, 2014-11-04 18:57:22]

+felt for chrome/browser/extensions/activity_log/ad_injection_unittest.cc

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

https://codereview.chromium.org/509203002/diff/120001/components/rappor/rappo...
components/rappor/rappor_service.cc:107: if (reporting_level <=
REPORTING_DISABLED) {
On 2014/11/04 15:39:24, Alexei Svitkine wrote:
> Nit: No {}'s

Done.

https://codereview.chromium.org/509203002/diff/120001/components/rappor/rappo...
components/rappor/rappor_service.cc:193: };
On 2014/11/04 15:39:24, Alexei Svitkine wrote:
> Nit: Remove trailing ;

Done.

[felt, 2014-11-04 20:12:43]

felt@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[felt, 2014-11-04 20:12:59]

+Devlin for /activity_log/ code, since he wrote that portion

[Devlin, 2014-11-04 20:19:55]

lgtm as long as the test still passes

[Steven Holte, 2014-11-04 20:38:17]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-11-04 20:39:57]

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

[commit-bot: I haz the power, 2014-11-04 22:27:22]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-04 22:29:24]

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