Allow PolicyLoadStatusSample to override reporting method

Allow PolicyLoadStatusSample to override reporting method

PolicyLoadStatusSample reports stats to base::LinearHistogram by
default. However, when used from libchrome in Chrome OS, this does
not work and causes memory leaks. This CL introduces PolicyLoadStatusSampler, which just gathers samples without doing anything with them, and its derived class PolicyLoadStatusAutoSubmitter, which sends them to base::LinearHistogram in its destructor and matches the old behavior. The slightly different name '*SampleR' should prevent accidental usage. Chrome OS can introduce its own submitter class or use the sampler class to dispose samples.

BUG=717888
TEST=Added test and ran it

Review-Url: https://codereview.chromium.org/2860973002
Cr-Commit-Position: refs/heads/master@{#469654}
Committed: https://chromium.googlesource.com/chromium/src/+/6410def39290ed829e0d56b92ab03ddd985a0c04

[ljusten (tachyonic), 2017-05-04 09:56:02]

ljusten@chromium.org changed reviewers:
+ emaxx@chromium.org

[ljusten (tachyonic), 2017-05-04 09:56:02]

Maxim, please take a look. Thiemo is out this and next week.

[emaxx, 2017-05-04 13:00:34]

https://codereview.chromium.org/2860973002/diff/20001/components/policy/core/...
File components/policy/core/common/policy_load_status.h (right):

https://codereview.chromium.org/2860973002/diff/20001/components/policy/core/...
components/policy/core/common/policy_load_status.h:51: // If |report_callback|
is set, status is sent to this callback. Otherwise,
To me this looks like stretching a single class for two quite different usages.
Also it's a bit worrying to have a generic callback that is being called from
the destructor (it's nontrivial to track down the execution flow; also thinking
about potential reentrancy issues).

Can this be separated into two different classes instead? One just for UMA, as
before, and another would be for your purposes. For instance, if you don't
really need the "automatically call callback on destruction" behavior, then this
can be boiled down to an interface with just Add() and GetAllStatuses().

[ljusten (tachyonic), 2017-05-05 07:45:15]

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

[commit-bot: I haz the power, 2017-05-05 07:45:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ljusten (tachyonic), 2017-05-05 08:16:02]

Description was changed from

==========
Allow PolicyLoadStatusSample to override reporting method

PolicyLoadStatusSample reports stats to base::LinearHistogram by
default. However, when used from libchrome in Chrome OS, this does
not work and causes memory leaks. This CL adds a way to override
the stats reporting method, so that a proper way to report stats
in Chrome OS can be used.

BUG=717888
TEST=Added test and ran it
==========

to

==========
Allow PolicyLoadStatusSample to override reporting method

PolicyLoadStatusSample reports stats to base::LinearHistogram by
default. However, when used from libchrome in Chrome OS, this does
not work and causes memory leaks. This CL introduces PolicyLoadStatusSampler,
which just gathers samples without doing anything with them, and its derived
class PolicyLoadStatusAutoSubmitter, which sends them to base::LinearHistogram
in its destructor and matches the old behavior. The slightly different name
'*SampleR' should prevent accidental usage. Chrome OS can introduce its own
submitter class or use the sampler class to dispose samples.

BUG=717888
TEST=Added test and ran it
==========

[ljusten (tachyonic), 2017-05-05 08:16:27]

Maksim, PTAL.

https://codereview.chromium.org/2860973002/diff/20001/components/policy/core/...
File components/policy/core/common/policy_load_status.h (right):

https://codereview.chromium.org/2860973002/diff/20001/components/policy/core/...
components/policy/core/common/policy_load_status.h:51: // If |report_callback|
is set, status is sent to this callback. Otherwise,
Thanks, I like that better.

[ljusten (tachyonic), 2017-05-05 08:17:03]

And sorry for misspelling your name first ;-)

[commit-bot: I haz the power, 2017-05-05 08:28:46]

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

[commit-bot: I haz the power, 2017-05-05 08:28:47]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-05-05 10:44:26]

LGTM with nits.
Thanks!

On 2017/05/05 08:17:03, ljusten (instantaneous) wrote:
> And sorry for misspelling your name first ;-)

No problem - actually that spelling was used in my old passport, and then they
changed the Latin transliteration rules for some reason :)

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
File components/policy/core/common/policy_load_status.h (right):

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
components/policy/core/common/policy_load_status.h:10: #include
"base/callback.h"
nit: Not necessary anymore.

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
components/policy/core/common/policy_load_status.h:65: class POLICY_EXPORT
PolicyLoadStatusAutoSubmitter
nit: Not insisting, but as you're already changing the name of the class, I'd
personally like something that explicitly refers to UMA - like
"...AutoUmaReporter" (this fact wasn't clear to me from the old name, and isn't
fully obvious to me now). But leaving this up to you.

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
File components/policy/core/common/preg_parser_unittest.cc (right):

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
components/policy/core/common/preg_parser_unittest.cc:178:
TEST_F(PRegParserTest, OverrideReportCallback) {
nit: Update the test name - there's no callback now.

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
components/policy/core/common/preg_parser_unittest.cc:180:
PolicyLoadStatusAutoSubmitter status;
Probably testing the PolicyLoadStatusSampler class is more appropriate here? As
this test needs only GetStatusSet().
(BTW, I would prefer compositing to inheritance of classes - then such misplace
won't be possible. But not insisting.)

[ljusten (tachyonic), 2017-05-05 12:58:17]

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

[commit-bot: I haz the power, 2017-05-05 12:58:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ljusten (tachyonic), 2017-05-05 12:58:36]

Thanks!

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
File components/policy/core/common/policy_load_status.h (right):

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
components/policy/core/common/policy_load_status.h:10: #include
"base/callback.h"
On 2017/05/05 10:44:25, emaxx wrote:
> nit: Not necessary anymore.

Done.

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
components/policy/core/common/policy_load_status.h:65: class POLICY_EXPORT
PolicyLoadStatusAutoSubmitter
On 2017/05/05 10:44:25, emaxx wrote:
> nit: Not insisting, but as you're already changing the name of the class, I'd
> personally like something that explicitly refers to UMA - like
> "...AutoUmaReporter" (this fact wasn't clear to me from the old name, and
isn't
> fully obvious to me now). But leaving this up to you.

Done, but I dropped the 'Auto' - it's cleaner.

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
File components/policy/core/common/preg_parser_unittest.cc (right):

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
components/policy/core/common/preg_parser_unittest.cc:178:
TEST_F(PRegParserTest, OverrideReportCallback) {
Done.

https://codereview.chromium.org/2860973002/diff/40001/components/policy/core/...
components/policy/core/common/preg_parser_unittest.cc:180:
PolicyLoadStatusAutoSubmitter status;
On 2017/05/05 10:44:25, emaxx wrote:
> Probably testing the PolicyLoadStatusSampler class is more appropriate here?
As
> this test needs only GetStatusSet().

Wasn't sure which one I should use. On the one hand, it makes sense to test
what's used in prod, on the other hand it's not necessary here. I'll keep anyway
it since this is used in prod.

> (BTW, I would prefer compositing to inheritance of classes - then such
misplace
> won't be possible. But not insisting.)

I didn't want to change all locations where status is passed as parameter. If I
used composition, then I'd probably embed the sampler in the reporter and have a
getter and call reporter->GetSampler() or something like that instead of passing
in the status.

[commit-bot: I haz the power, 2017-05-05 13:52:40]

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

[commit-bot: I haz the power, 2017-05-05 13:52:41]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-05-05 14:40:29]

Still LGTM.

On 2017/05/05 12:58:36, ljusten (instantaneous) wrote:
> > (BTW, I would prefer compositing to inheritance of classes - then such
> misplace
> > won't be possible. But not insisting.)
> 
> I didn't want to change all locations where status is passed as parameter. If
I
> used composition, then I'd probably embed the sampler in the reporter and have
a
> getter and call reporter->GetSampler() or something like that instead of
passing
> in the status.

Or, instead of exposing the sampler, simply expose Add() that will call the
sampler internally.
But I'm fine with the current approach either - the classes are tiny enough to
understand them quickly.

[ljusten (tachyonic), 2017-05-05 15:08:43]

The CQ bit was checked by ljusten@chromium.org

[ljusten (tachyonic), 2017-05-05 15:08:48]

The CQ bit was unchecked by ljusten@chromium.org

[ljusten (tachyonic), 2017-05-05 15:14:36]

> Or, instead of exposing the sampler, simply expose Add() that will call the
> sampler internally.

But then I wouldn't be able to pass both classes to preg_parser::ReadFile.
However, I need to do
  PolicyLoadStatusUmaReporter status;
  preg_parser::ReadFile(..., &status);
in Chrome as well as
  PolicyLoadStatusSampler status;
  preg_parser::ReadFile(..., &status);
in Chrome OS. I could add a base interface to both classes that just exposes
Add(), but that seems arguably more convoluted.

[ljusten (tachyonic), 2017-05-05 15:15:07]

The CQ bit was checked by ljusten@chromium.org

[commit-bot: I haz the power, 2017-05-05 15:15:35]

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-05-05 15:20:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493997307434810,
"parent_rev": "09721eaab9e4d8dd62796f991da68dc2b89a325c", "commit_rev":
"6410def39290ed829e0d56b92ab03ddd985a0c04"}

[commit-bot: I haz the power, 2017-05-05 15:21:03]

Description was changed from

==========
Allow PolicyLoadStatusSample to override reporting method

PolicyLoadStatusSample reports stats to base::LinearHistogram by
default. However, when used from libchrome in Chrome OS, this does
not work and causes memory leaks. This CL introduces PolicyLoadStatusSampler,
which just gathers samples without doing anything with them, and its derived
class PolicyLoadStatusAutoSubmitter, which sends them to base::LinearHistogram
in its destructor and matches the old behavior. The slightly different name
'*SampleR' should prevent accidental usage. Chrome OS can introduce its own
submitter class or use the sampler class to dispose samples.

BUG=717888
TEST=Added test and ran it
==========

to

==========
Allow PolicyLoadStatusSample to override reporting method

PolicyLoadStatusSample reports stats to base::LinearHistogram by
default. However, when used from libchrome in Chrome OS, this does
not work and causes memory leaks. This CL introduces PolicyLoadStatusSampler,
which just gathers samples without doing anything with them, and its derived
class PolicyLoadStatusAutoSubmitter, which sends them to base::LinearHistogram
in its destructor and matches the old behavior. The slightly different name
'*SampleR' should prevent accidental usage. Chrome OS can introduce its own
submitter class or use the sampler class to dispose samples.

BUG=717888
TEST=Added test and ran it

Review-Url: https://codereview.chromium.org/2860973002
Cr-Commit-Position: refs/heads/master@{#469654}
Committed:
https://chromium.googlesource.com/chromium/src/+/6410def39290ed829e0d56b92ab0...
==========

[commit-bot: I haz the power, 2017-05-05 15:21:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6410def39290ed829e0d56b92ab0...