Alternative Multi-dimensional Rappor

Multi-dimensional Rappor Implementation

This implements support for Rappor metrics which contain
multiple fields that support correlation analysis.
Example:
scoped_ptr<Sample> sample = rappor_service->CreateSample(COARSE_RAPPOR_TYPE);
sample->SetStringField("Domain", "google.com");
sample->SetFlagsField("Flags", 0x10, 8 /* # of bits */);
rappor_service->RecordSampleObj("MyMetric", sample.Pass());

This change also removes metric_name from the personalization string for the PRR.  This means that two different metrics which report the same value will use the same PRR, which will improve privacy when multiple metrics report the same value.

BUG=451647
Committed: https://crrev.com/feb4e55d126e711b0a5e9dd321371a4923768346
Cr-Commit-Position: refs/heads/master@{#327207}

[Steven Holte, 2015-04-15 21:14:24]

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

[Steven Holte, 2015-04-15 21:14:24]

This is an alternative implementation of multi-dimensional rappors (from
https://codereview.chromium.org/1058333002/)

This one would use the interface:

scoped_ptr<Sample> sample = rappor_service->MakeSampleObj(UMA_RAPPOR_TYPE);
sample->SetStringField("domain", GetDomainAndRegistry(url));
sample->SetStringField("port", GetPort(url));
sample->SetFlagsField("flags", ABC|XYZ);
rappor_service->RecordSampleObj("MyMetric", sample.Pass());

The values would be sampled together, so the could be correlated, but would show
up in the report as three seperate metrics:

MyMetric.domain
MyMetric.port
MyMetric.flags

[Alexei Svitkine (slow), 2015-04-15 21:21:06]

On 2015/04/15 21:14:24, Steven Holte wrote:
> This is an alternative implementation of multi-dimensional rappors (from
> https://codereview.chromium.org/1058333002/)
> 
> This one would use the interface:
> 
> scoped_ptr<Sample> sample = rappor_service->MakeSampleObj(UMA_RAPPOR_TYPE);
> sample->SetStringField("domain", GetDomainAndRegistry(url));
> sample->SetStringField("port", GetPort(url));
> sample->SetFlagsField("flags", ABC|XYZ);
> rappor_service->RecordSampleObj("MyMetric", sample.Pass());
> 
> The values would be sampled together, so the could be correlated, but would
show
> up in the report as three seperate metrics:
> 
> MyMetric.domain
> MyMetric.port
> MyMetric.flags

This new API seems much more flexible.

Though, one concern with it - if we want users to go through an API that takes a
rappor_service param that could be null, then it's not immediately obvious how
that would look with the above api. Perhaps construction of the sample object
can happen without needing to use rappor_service? Then, the record call would
just be the one that's a no-op.

Or maybe we think that correlated metrics will be sufficiently rarely used that
the extra manual code that null checks itself is OK?

[Steven Holte, 2015-04-15 21:39:39]

On 2015/04/15 21:21:06, Alexei Svitkine wrote:
> On 2015/04/15 21:14:24, Steven Holte wrote:
> > This is an alternative implementation of multi-dimensional rappors (from
> > https://codereview.chromium.org/1058333002/)
> > 
> > This one would use the interface:
> > 
> > scoped_ptr<Sample> sample = rappor_service->MakeSampleObj(UMA_RAPPOR_TYPE);
> > sample->SetStringField("domain", GetDomainAndRegistry(url));
> > sample->SetStringField("port", GetPort(url));
> > sample->SetFlagsField("flags", ABC|XYZ);
> > rappor_service->RecordSampleObj("MyMetric", sample.Pass());
> > 
> > The values would be sampled together, so the could be correlated, but would
> show
> > up in the report as three seperate metrics:
> > 
> > MyMetric.domain
> > MyMetric.port
> > MyMetric.flags
> 
> This new API seems much more flexible.
> 
> Though, one concern with it - if we want users to go through an API that takes
a
> rappor_service param that could be null, then it's not immediately obvious how
> that would look with the above api. Perhaps construction of the sample object
> can happen without needing to use rappor_service? Then, the record call would
> just be the one that's a no-op.
> 
> Or maybe we think that correlated metrics will be sufficiently rarely used
that
> the extra manual code that null checks itself is OK?

To avoid going through the service object, we would need to store copies of the
strings themselves in the sample object, since the cohort is needed to apply the
bloom filtering step.  That wouldn't necessarily be that bad since samples
shouldn't really be large strings anyway.

An alternative to that would be that a null-checking API could just return a
dummy object for storing the samples in.

[Alexei Svitkine (slow), 2015-04-16 15:09:44]

https://codereview.chromium.org/1090683003/diff/1/components/rappor/bloom_fil...
File components/rappor/bloom_filter.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/bloom_fil...
components/rappor/bloom_filter.cc:64: }
// namespace internal

https://codereview.chromium.org/1090683003/diff/1/components/rappor/reports.cc
File components/rappor/reports.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/reports.c...
components/rappor/reports.cc:26: personalization_string);
Any way this can share code with the other place we're doing this? (I notice
you're adding the new function without removing anything...)

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc...
components/rappor/sample.cc:41: sizes[fieldName] = parameters_.flag_bytes;
Hmm, this flag_bytes field now specified the value for all possible flag fields?

What would be the downside to always using e.g. 64bits here? Would that increase
the payload?

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc...
components/rappor/sample.cc:47: for (std::map<std::string,
uint64_t>::const_iterator it = fields.begin();
Nit: Use C++ for loop syntax.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h
File components/rappor/sample.h (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
2015. Fix throughout.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:18: class Sample {
Comment.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:23: void SetStringField(const std::string& fieldName,
const std::string& value);
Nit: field_name

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:27: const std::string& metricName,
metric_name

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:35: std::map<std::string, size_t> sizes;
sizes_

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:36: std::map<std::string, uint64_t> fields;
fields_

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:37: DISALLOW_COPY_AND_ASSIGN(Sample);
Empty line before.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.cc
File components/rappor/sampler.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.c...
components/rappor/sampler.cc:29: for (auto it = samples_.begin(); it !=
samples_.end(); ++it) {
Nit: Use C++11 for loop syntax, i.e.

for (const auto& entry : samples_) {
  entry.second->ExportMetrics(...);
}

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.h
File components/rappor/sampler.h (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.h...
components/rappor/sampler.h:21: class Sampler {
Comment.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.h...
components/rappor/sampler.h:34: DISALLOW_COPY_AND_ASSIGN(Sampler);
Blank line above.

[Nathan Parker, 2015-04-22 15:47:10]

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

[Nathan Parker, 2015-04-22 15:47:10]

Steven -- This interface looks great to me, and does everything we need for a
number of use cases.

 -- Nathan

[Steven Holte, 2015-04-22 19:24:28]

https://codereview.chromium.org/1090683003/diff/1/components/rappor/bloom_fil...
File components/rappor/bloom_filter.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/bloom_fil...
components/rappor/bloom_filter.cc:64: }
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> // namespace internal

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/reports.cc
File components/rappor/reports.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/reports.c...
components/rappor/reports.cc:26: personalization_string);
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> Any way this can share code with the other place we're doing this? (I notice
> you're adding the new function without removing anything...)

Done.  There is a slight change here to the existing code, where we stop using
metric_name as part of the personalization_string, but this should not cause any
issues, and I was intending to fix that anyway, since there is better privacy
with related metrics that way.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc...
components/rappor/sample.cc:41: sizes[fieldName] = parameters_.flag_bytes;
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> Hmm, this flag_bytes field now specified the value for all possible flag
fields?
> 
> What would be the downside to always using e.g. 64bits here? Would that
increase
> the payload?

The main drawback is that we would generate unnecessary PRR bits for this, which
could potentially be used for fingerprinting.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc...
components/rappor/sample.cc:47: for (std::map<std::string,
uint64_t>::const_iterator it = fields.begin();
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> Nit: Use C++ for loop syntax.

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h
File components/rappor/sample.h (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> 2015. Fix throughout.

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:18: class Sample {
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> Comment.

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:23: void SetStringField(const std::string& fieldName,
const std::string& value);
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> Nit: field_name

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:27: const std::string& metricName,
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> metric_name

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:35: std::map<std::string, size_t> sizes;
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> sizes_

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:36: std::map<std::string, uint64_t> fields;
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> fields_

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.h#...
components/rappor/sample.h:37: DISALLOW_COPY_AND_ASSIGN(Sample);
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> Empty line before.

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.cc
File components/rappor/sampler.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.c...
components/rappor/sampler.cc:29: for (auto it = samples_.begin(); it !=
samples_.end(); ++it) {
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> Nit: Use C++11 for loop syntax, i.e.
> 
> for (const auto& entry : samples_) {
>   entry.second->ExportMetrics(...);
> }

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.h
File components/rappor/sampler.h (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.h...
components/rappor/sampler.h:21: class Sampler {
On 2015/04/16 15:09:44, Alexei Svitkine wrote:
> Comment.

Done.

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sampler.h...
components/rappor/sampler.h:34: DISALLOW_COPY_AND_ASSIGN(Sampler);
On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> Blank line above.

Done.

[Alexei Svitkine (slow), 2015-04-22 20:26:30]

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc...
components/rappor/sample.cc:41: sizes[fieldName] = parameters_.flag_bytes;
On 2015/04/22 19:24:28, Steven Holte wrote:
> On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> > Hmm, this flag_bytes field now specified the value for all possible flag
> fields?
> > 
> > What would be the downside to always using e.g. 64bits here? Would that
> increase
> > the payload?
> 
> The main drawback is that we would generate unnecessary PRR bits for this,
which
> could potentially be used for fingerprinting.

I see, how about having the user specify the number of bits as a param to the
SetFlagField() call?

[Steven Holte, 2015-04-22 20:52:31]

On 2015/04/22 20:26:30, Alexei Svitkine wrote:
> https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc
> File components/rappor/sample.cc (right):
> 
>
https://codereview.chromium.org/1090683003/diff/1/components/rappor/sample.cc...
> components/rappor/sample.cc:41: sizes[fieldName] = parameters_.flag_bytes;
> On 2015/04/22 19:24:28, Steven Holte wrote:
> > On 2015/04/16 15:09:43, Alexei Svitkine wrote:
> > > Hmm, this flag_bytes field now specified the value for all possible flag
> > fields?
> > > 
> > > What would be the downside to always using e.g. 64bits here? Would that
> > increase
> > > the payload?
> > 
> > The main drawback is that we would generate unnecessary PRR bits for this,
> which
> > could potentially be used for fingerprinting.
> 
> I see, how about having the user specify the number of bits as a param to the
> SetFlagField() call?

Already did that actually.  Also added a test.

[Alexei Svitkine (slow), 2015-04-22 21:00:13]

looking good, some more comments

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/rappo...
File components/rappor/rappor_service_unittest.cc (right):

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/rappo...
components/rappor/rappor_service_unittest.cc:107: uint64_t urlHash =
metrics::HashMetricName("ObjMetric.Url");
Nit: hacker_style

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sample.cc:26: 
Nit: Remove empty line

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sample.cc:41: uint64_t flags, size_t num_flags) {
1 param per line.

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
File components/rappor/sampler.cc (right):

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sampler.cc:25: if
(base::RandGenerator(sample_counts_[metric_name]) == 0) {
Nit: No {}

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
File components/rappor/sampler.h (right):

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sampler.h:37: void ExportMetrics(const std::string& secret,
RapporReports* reports);
Could you add a unit test for this class?

i.e. that ExportMetrics really does clear them and that logging multiple times
has one or the other metric (e.g. log two values for the same metric and check
that the export of that is either one or the other).

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sampler.h:44: // The map owns the metrics it contains.
Nit: Last line is uneccessary since the map type implies that. Also, rephrase
first line without using "we".

[Steven Holte, 2015-04-22 21:59:33]

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/rappo...
File components/rappor/rappor_service_unittest.cc (right):

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/rappo...
components/rappor/rappor_service_unittest.cc:107: uint64_t urlHash =
metrics::HashMetricName("ObjMetric.Url");
On 2015/04/22 21:00:13, Alexei Svitkine wrote:
> Nit: hacker_style

Done.

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sample.cc:26: 
On 2015/04/22 21:00:13, Alexei Svitkine wrote:
> Nit: Remove empty line

Done.

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sample.cc:41: uint64_t flags, size_t num_flags) {
On 2015/04/22 21:00:13, Alexei Svitkine wrote:
> 1 param per line.

Done.

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
File components/rappor/sampler.cc (right):

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sampler.cc:25: if
(base::RandGenerator(sample_counts_[metric_name]) == 0) {
On 2015/04/22 21:00:13, Alexei Svitkine wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
File components/rappor/sampler.h (right):

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sampler.h:37: void ExportMetrics(const std::string& secret,
RapporReports* reports);
On 2015/04/22 21:00:13, Alexei Svitkine wrote:
> Could you add a unit test for this class?
> 
> i.e. that ExportMetrics really does clear them and that logging multiple times
> has one or the other metric (e.g. log two values for the same metric and check
> that the export of that is either one or the other).

Done.

https://codereview.chromium.org/1090683003/diff/80001/components/rappor/sampl...
components/rappor/sampler.h:44: // The map owns the metrics it contains.
On 2015/04/22 21:00:13, Alexei Svitkine wrote:
> Nit: Last line is uneccessary since the map type implies that. Also, rephrase
> first line without using "we".

Done.

[Alexei Svitkine (slow), 2015-04-23 21:42:34]

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
File components/rappor/bloom_filter.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter.cc:52: DCHECK_LE(bytes_size, 8u);
Add a comment above this check (i.e. in order for the hash to fit into 64 bits).

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter.cc:58: CityHash64WithSeed(str.data(), str.size(),
hash_seed_offset + i);
Nit: Make a helper in the anon namespace that wraps this so this comment doesn't
have to be repeated.

Should take str, and seed as params.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter.cc:59: output |= 1 << (index % (bytes_size * 8));
Nit: Move bytes_size * 8 to a variable outside the loop.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
File components/rappor/bloom_filter_unittest.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter_unittest.cc:54: TEST(BloomFilterTest,
GetFunction) {
Call the test GetBloomBits?

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter_unittest.cc:56: EXPECT_EQ(uint64_t(0xa8),
bytes_from_get);
This style cast is against style guide - either static_cast or maybe you can
express the literal with a 'u' suffix to not need the cast?

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
components/rappor/rappor_metric.cc:42: return internal::GenerateReport(secret,
parameters(), bytes());
It looks like with this refactoring we're losing the metric_name_ prefix when
building the personalization_string. I don't think that's intentional?

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

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
components/rappor/rappor_service.cc:271: // Skip this metric if it's reporting
level is less than the enabled
Nit: it's -> its

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

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
components/rappor/rappor_service.h:78: scoped_ptr<Sample>
MakeSampleObj(RapporType);
I don't think we should use "SampleObj" in the signature. How about
CreateSample() and RecordSample(). Also, these methods need documentation -
should be above the static function above.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/samp...
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/samp...
components/rappor/sample.cc:58: value_bytes[i] = (value & (0xff << (i * 8))) >>
(i * 8);
I think this needs a comment.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/samp...
File components/rappor/sample.h (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/samp...
components/rappor/sample.h:25: // RapporService::MakeSampleObj to create a
sample.
How about making this ctor private and making RapporService a friend to enforce
this?

[Steven Holte, 2015-04-24 16:59:06]

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
File components/rappor/bloom_filter.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter.cc:52: DCHECK_LE(bytes_size, 8u);
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> Add a comment above this check (i.e. in order for the hash to fit into 64
bits).

Done.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter.cc:58: CityHash64WithSeed(str.data(), str.size(),
hash_seed_offset + i);
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> Nit: Make a helper in the anon namespace that wraps this so this comment
doesn't
> have to be repeated.
> 
> Should take str, and seed as params.

Done.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter.cc:59: output |= 1 << (index % (bytes_size * 8));
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> Nit: Move bytes_size * 8 to a variable outside the loop.

Done.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
File components/rappor/bloom_filter_unittest.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter_unittest.cc:54: TEST(BloomFilterTest,
GetFunction) {
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> Call the test GetBloomBits?

Done.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/bloo...
components/rappor/bloom_filter_unittest.cc:56: EXPECT_EQ(uint64_t(0xa8),
bytes_from_get);
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> This style cast is against style guide - either static_cast or maybe you can
> express the literal with a 'u' suffix to not need the cast?

Done.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
components/rappor/rappor_metric.cc:42: return internal::GenerateReport(secret,
parameters(), bytes());
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> It looks like with this refactoring we're losing the metric_name_ prefix when
> building the personalization_string. I don't think that's intentional?

It's intentional, I mentioned it in this earlier comment:
https://codereview.chromium.org/1090683003/diff/1/components/rappor/reports.c...

It should actually improve privacy slightly.

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

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
components/rappor/rappor_service.cc:271: // Skip this metric if it's reporting
level is less than the enabled
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> Nit: it's -> its

Done.

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

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
components/rappor/rappor_service.h:78: scoped_ptr<Sample>
MakeSampleObj(RapporType);
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> I don't think we should use "SampleObj" in the signature. How about
> CreateSample() and RecordSample(). Also, these methods need documentation -
> should be above the static function above.

Renamed to CreateSampleObj, but RecordSample is already used by the string
recording function.  It probably makes sense to rename that to RecordString, but
that would require changing a bunch of callers.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/samp...
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/samp...
components/rappor/sample.cc:58: value_bytes[i] = (value & (0xff << (i * 8))) >>
(i * 8);
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> I think this needs a comment.

Done.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/samp...
File components/rappor/sample.h (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/samp...
components/rappor/sample.h:25: // RapporService::MakeSampleObj to create a
sample.
On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> How about making this ctor private and making RapporService a friend to
enforce
> this?

Done.

[Alexei Svitkine (slow), 2015-04-24 18:10:02]

LGTM % remaining comments.

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/1090683003/diff/100001/components/rappor/rapp...
components/rappor/rappor_metric.cc:42: return internal::GenerateReport(secret,
parameters(), bytes());
On 2015/04/24 16:59:05, Steven Holte wrote:
> On 2015/04/23 21:42:34, Alexei Svitkine wrote:
> > It looks like with this refactoring we're losing the metric_name_ prefix
when
> > building the personalization_string. I don't think that's intentional?
> 
> It's intentional, I mentioned it in this earlier comment:
>
https://codereview.chromium.org/1090683003/diff/1/components/rappor/reports.c...
> 
> It should actually improve privacy slightly.

Okay, please mention this in the CL description. Also, please expand CL
description to briefly summarize what's being added here - maybe even include a
sample of the API use in it.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
File components/rappor/bloom_filter.cc (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
components/rappor/bloom_filter.cc:19: return index;
Nit: Might as well return it directly.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
components/rappor/bloom_filter.cc:24: 
Nit: Remove empty line.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
components/rappor/bloom_filter.cc:62: // Make sure result fits in uint64
Nit: add a period.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
components/rappor/bloom_filter.cc:65: uint32_t bits_size = bytes_size * 8;
Nit: const

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
components/rappor/rappor_service.h:68: // The constructs and returns a Sample
object for the caller to record
Nit: Reword ("The constructs").

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
components/rappor/rappor_service.h:79: void RecordSampleObj(const std::string&
metric_name,
Maybe add a TODO to rename?

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/samp...
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/samp...
components/rappor/sample.cc:59: uint64_t byte_mask = 0xff << (i * 8);
Nit: i * 8 is repeated - make a variable, e.g. |shift|.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/samp...
File components/rappor/sample.h (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/samp...
components/rappor/sample.h:51: Sample(int32_t cohort_seed, const
RapporParameters& parameters);
Nit: Add an empty line after this.

[Nathan Parker, 2015-04-24 18:26:25]

lgtm

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
components/rappor/rappor_service.h:78: //
rappor_service->RecordSample("MyMetric", sample.Pass());
Can you add comment that this will lead to a report setting
MyMetric.Field1
MyMetric.Field2

[Steven Holte, 2015-04-24 18:39:57]

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
File components/rappor/bloom_filter.cc (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
components/rappor/bloom_filter.cc:19: return index;
On 2015/04/24 18:10:01, Alexei Svitkine wrote:
> Nit: Might as well return it directly.

Done.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
components/rappor/bloom_filter.cc:24: 
On 2015/04/24 18:10:01, Alexei Svitkine wrote:
> Nit: Remove empty line.

Done.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
components/rappor/bloom_filter.cc:62: // Make sure result fits in uint64
On 2015/04/24 18:10:01, Alexei Svitkine wrote:
> Nit: add a period.

Done.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/bloo...
components/rappor/bloom_filter.cc:65: uint32_t bits_size = bytes_size * 8;
On 2015/04/24 18:10:01, Alexei Svitkine wrote:
> Nit: const

Done.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
components/rappor/rappor_service.h:68: // The constructs and returns a Sample
object for the caller to record
On 2015/04/24 18:10:01, Alexei Svitkine wrote:
> Nit: Reword ("The constructs").

Done.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
components/rappor/rappor_service.h:78: //
rappor_service->RecordSample("MyMetric", sample.Pass());
On 2015/04/24 18:26:25, nparker wrote:
> Can you add comment that this will lead to a report setting
> MyMetric.Field1
> MyMetric.Field2

Done.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/rapp...
components/rappor/rappor_service.h:79: void RecordSampleObj(const std::string&
metric_name,
On 2015/04/24 18:10:01, Alexei Svitkine wrote:
> Maybe add a TODO to rename?

Done.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/samp...
File components/rappor/sample.cc (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/samp...
components/rappor/sample.cc:59: uint64_t byte_mask = 0xff << (i * 8);
On 2015/04/24 18:10:01, Alexei Svitkine wrote:
> Nit: i * 8 is repeated - make a variable, e.g. |shift|.

Done.

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/samp...
File components/rappor/sample.h (right):

https://codereview.chromium.org/1090683003/diff/120001/components/rappor/samp...
components/rappor/sample.h:51: Sample(int32_t cohort_seed, const
RapporParameters& parameters);
On 2015/04/24 18:10:01, Alexei Svitkine wrote:
> Nit: Add an empty line after this.

Done.

[Steven Holte, 2015-04-27 20:55:19]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2015-04-27 20:55:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1090683003/#ps160001
(title: "Fix test")

[commit-bot: I haz the power, 2015-04-27 20:55:57]

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

[commit-bot: I haz the power, 2015-04-27 21:15:44]

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

[commit-bot: I haz the power, 2015-04-27 21:15:45]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Steven Holte, 2015-04-27 21:58:57]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2015-04-27 21:58:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1090683003/#ps200001
(title: "Fix android build")

[commit-bot: I haz the power, 2015-04-27 21:59:22]

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

[commit-bot: I haz the power, 2015-04-27 22:35:16]

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

[commit-bot: I haz the power, 2015-04-27 22:35:17]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Steven Holte, 2015-04-27 22:50:53]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2015-04-27 22:50:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1090683003/#ps220001
(title: "Use 64-bit shift")

[commit-bot: I haz the power, 2015-04-27 22:51:00]

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

[commit-bot: I haz the power, 2015-04-28 01:05:08]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-04-28 01:06:30]

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

[brucedawson, 2015-04-28 23:30:31]

This CL triggered a new /analyze warning which looks like it is a real bug. The
warning is:

src\components\rappor\sample.cc(64) : warning C6297: Arithmetic overflow: 
32-bit value is shifted, then cast to 64-bit value.  Results might not be an
expected value.

The line of code is:

    uint64_t byte_mask = 0xff << shift;

This code will fail (result might not be an expected value) if shift is greater
than 24. It looks like shift is intended to be as large as 56. Presumably either
the 0xff constant should be a uint64_t constant, or else byte_mask should be a
uint32_t variable.

Something like this will fix the code:

    uint64_t byte_mask = uint64_t(0xff) << shift;

[Steven Holte, 2015-04-29 01:10:54]

On 2015/04/28 23:30:31, brucedawson wrote:
> This CL triggered a new /analyze warning which looks like it is a real bug.
The
> warning is:
> 
> src\components\rappor\sample.cc(64) : warning C6297: Arithmetic overflow: 
> 32-bit value is shifted, then cast to 64-bit value.  Results might not be an
> expected value.
> 
> The line of code is:
> 
>     uint64_t byte_mask = 0xff << shift;
> 
> This code will fail (result might not be an expected value) if shift is
greater
> than 24. It looks like shift is intended to be as large as 56. Presumably
either
> the 0xff constant should be a uint64_t constant, or else byte_mask should be a
> uint32_t variable.
> 
> Something like this will fix the code:
> 
>     uint64_t byte_mask = uint64_t(0xff) << shift;

Created a CL to fix and test this, but awkwardly it works fine in my local linux
build without the cast.
https://codereview.chromium.org/1112003002