Add a StatisticsDeltaReader class and use cases

Add a StatisticsDeltaReader class and use cases

This CL adds a utility class to streamline the task of testing that
metrics have been updated as expected. Specifically, the
StatisticsDeltaReader class allows a client to obtain the differential
value of a given histogram in the interval since the given
StatisticsDeltaReader instance was created.

This reverts commit 6a7dfb3cd638829a04e002ea9bc5b18ed3c9c03f.

BUG=232414
TBR=rsleevy, groby

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

[lpromero, 2013-12-23 16:05:32]

First patch is the revert of the revert.
Coming up in patch 2, the unadressed feedback from mark in
https://codereview.chromium.org/99553004/

[ppi, 2013-12-23 16:37:47]

Thanks, Louis! Hopefully this time it sticks.

Please find my comments inline.

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder.cc
File base/test/histogram_recorder.cc (right):

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.cc:14: void HistogramRecorder::Initialize() {
Should we drop it and call StatisticsRecorder directly?

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.cc:35: bool HistogramRecorder::IsActive() {
Should we drop it and call StatisticsRecorder directly?

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder.h
File base/test/histogram_recorder.h (right):

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.h:17: // tests to check that metrics were recorded
as they should be.
I suggest:

"This class acts as a differential reader for histogram samples recorded in the
global StatisticsRecorder, allowing to inspect samples collected during the
lifetime of the class."

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.h:18: class HistogramRecorder {
Let's rename it to HistogramDeltaReader ?

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.h:21: static void Initialize();
Clients can call StatisticsRecorder::Initialize() directly, so this seems an
unnecessary wiring - should we drop it?

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.h:26: static bool IsActive();
What do we need that for? See also the comment above, if this is available in
StatisticsRecorder(), we shouldn't need to expose it here.

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
File base/test/histogram_recorder_unittest.cc (right):

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder_unittest.cc:17: }
I'm afraid we have to follow suit of existing unit tests - we can't leave the
global state initialized after the tests run or subsequent base tests will
crash.

It should go along the lines:

  virtual void SetUp() {
    statistics_recorder_ = new StatisticsRecorder();
  }

  virtual void TearDown() {
    delete statistics_recorder_;
    statistics_recorder_ = NULL;
  }

  StatisticsRecorder* statistics_recorder_;

https://codereview.chromium.org/120753002/diff/1/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc (right):

https://codereview.chromium.org/120753002/diff/1/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc:26:
base::HistogramRecorder::Initialize();
StatisticsRecorder::Initialize()?

https://codereview.chromium.org/120753002/diff/1/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc:42:
scoped_ptr<base::HistogramRecorder> histogram_recorder_;
We can drop this field along with ResetHistogramRecorder() and just create an
instance (through local variable, not a pointer) in the test whenever needed:

HistogramRecorder recorder;

https://codereview.chromium.org/120753002/diff/1/chrome/browser/ui/cocoa/brow...
File
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/120753002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:69:
scoped_ptr<base::HistogramRecorder> histogram_recorder_;
I'd drop it and just create an instance in test when needed.

https://codereview.chromium.org/120753002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_throttler_unittest.cc (right):

https://codereview.chromium.org/120753002/diff/1/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:185:
scoped_ptr<base::HistogramRecorder> histogram_recorder_;
I'd drop it and just create an instance in test when needed.

[lpromero, 2013-12-23 17:22:54]

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder.cc
File base/test/histogram_recorder.cc (right):

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.cc:14: void HistogramRecorder::Initialize() {
On 2013/12/23 16:37:48, ppi wrote:
> Should we drop it and call StatisticsRecorder directly?

I liked the encapsulation. Then, when using HistogramDeltaReader, no need to
include statistics_recorder. What do you think?

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.cc:35: bool HistogramRecorder::IsActive() {
On 2013/12/23 16:37:48, ppi wrote:
> Should we drop it and call StatisticsRecorder directly?

Ditto.

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
File base/test/histogram_recorder_unittest.cc (right):

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder_unittest.cc:17: }
On 2013/12/23 16:37:48, ppi wrote:
> I'm afraid we have to follow suit of existing unit tests - we can't leave the
> global state initialized after the tests run or subsequent base tests will
> crash.
> 
> It should go along the lines:
> 
>   virtual void SetUp() {
>     statistics_recorder_ = new StatisticsRecorder();
>   }
> 
>   virtual void TearDown() {
>     delete statistics_recorder_;
>     statistics_recorder_ = NULL;
>   }
> 
>   StatisticsRecorder* statistics_recorder_;

Done.

https://codereview.chromium.org/120753002/diff/1/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc (right):

https://codereview.chromium.org/120753002/diff/1/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc:42:
scoped_ptr<base::HistogramRecorder> histogram_recorder_;
On 2013/12/23 16:37:48, ppi wrote:
> We can drop this field along with ResetHistogramRecorder() and just create an
> instance (through local variable, not a pointer) in the test whenever needed:
> 
> HistogramRecorder recorder;

Done.

https://codereview.chromium.org/120753002/diff/1/chrome/browser/ui/cocoa/brow...
File
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/120753002/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:69:
scoped_ptr<base::HistogramRecorder> histogram_recorder_;
On 2013/12/23 16:37:48, ppi wrote:
> I'd drop it and just create an instance in test when needed.

Then I have to get rid of GetHistogramSamples() above. Uploading a patch.

https://codereview.chromium.org/120753002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_throttler_unittest.cc (right):

https://codereview.chromium.org/120753002/diff/1/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:185:
scoped_ptr<base::HistogramRecorder> histogram_recorder_;
On 2013/12/23 16:37:48, ppi wrote:
> I'd drop it and just create an instance in test when needed.

Done.

[ppi, 2013-12-23 17:31:37]

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder.cc
File base/test/histogram_recorder.cc (right):

https://codereview.chromium.org/120753002/diff/1/base/test/histogram_recorder...
base/test/histogram_recorder.cc:14: void HistogramRecorder::Initialize() {
On 2013/12/23 17:22:54, lpromero wrote:
> On 2013/12/23 16:37:48, ppi wrote:
> > Should we drop it and call StatisticsRecorder directly?
> 
> I liked the encapsulation. Then, when using HistogramDeltaReader, no need to
> include statistics_recorder. What do you think?

This is interesting point, but note that when using HistogramDeltaReader, one
will probably record some histograms. So, StatisticsRecorder needs to be
initialized anyway for the sake of histograms, and we probably don't need to
pretend that HistogramDeltaReader::Initialize() and
StatisticsRecorder::Initialize() are different things.

Not terribly important, so please feel free to skip if you're not convinced:).

[ppi, 2013-12-23 17:41:44]

lgtm modulo class rename - thanks!

[ppi, 2014-01-03 12:21:09]

One more remark (and explanation for trybot failures) below.

https://codereview.chromium.org/120753002/diff/180001/base/test/statistics_de...
File base/test/statistics_delta_reader_unittest.cc (right):

https://codereview.chromium.org/120753002/diff/180001/base/test/statistics_de...
base/test/statistics_delta_reader_unittest.cc:17:
StatisticsRecorder::Initialize();
We shouldn't do this - the singleton gets initialized when StatisticsRecorder()
instance is created. This is making the trybots fail.

[lpromero, 2014-01-03 18:14:05]

Launching new try jobs with the fix.
Thanks for the help!

https://codereview.chromium.org/120753002/diff/180001/base/test/statistics_de...
File base/test/statistics_delta_reader_unittest.cc (right):

https://codereview.chromium.org/120753002/diff/180001/base/test/statistics_de...
base/test/statistics_delta_reader_unittest.cc:17:
StatisticsRecorder::Initialize();
On 2014/01/03 12:21:10, ppi wrote:
> We shouldn't do this - the singleton gets initialized when
StatisticsRecorder()
> instance is created. This is making the trybots fail.

Done.

[lpromero, 2014-01-06 15:20:10]

Adding original reviewers to review the fix.

Patch 1 is original CL (https://codereview.chromium.org/19866004), patch 2 is
mark feedback from https://codereview.chromium.org/99553004/.

Subsequent patches are what is new.

[Mark Mentovai, 2014-01-06 15:25:24]

LGTM in base

[ppi, 2014-01-06 15:29:59]

A brief summary for our kind reviewers and OWNERS.

This is another attempt to land this CL upstreaming a helper class for
differential reading of recorded histograms developed by Chrome/iOS team.

The reason it bounced back the previous time is that the base unittests that
exercise the metrics recording system create and demolish it each time; and
crash if run with the system already initialized.

So in this CL (versus already reviewed ps1 & ps2) we just adhere to the
convention in the new base unittest + the class gets renamed.

Hopefully this time it will stick.

[Ilya Sherman, 2014-01-06 23:31:35]

Drive-by nits, otherwise LGTM.  Thanks for upstreaming this utility class!

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
File base/test/statistics_delta_reader.cc (right):

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
base/test/statistics_delta_reader.cc:17: StatisticsRecorder::GetSnapshot("",
&histograms);
nit: "" -> std::string()

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
base/test/statistics_delta_reader.cc:18: for (size_t i = 0; i <
histograms.size(); i++) {
nit: i++ -> ++i

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
File base/test/statistics_delta_reader.h (right):

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
base/test/statistics_delta_reader.h:32: // lifecycle. This isntance takes
ownership of the samples, which are deleted
nit: "isntance" -> "instance"

[lpromero, 2014-01-07 10:20:01]

Thanks for the review!

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
File base/test/statistics_delta_reader.cc (right):

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
base/test/statistics_delta_reader.cc:17: StatisticsRecorder::GetSnapshot("",
&histograms);
On 2014/01/06 23:31:35, Ilya Sherman (Away Jan11-Feb2) wrote:
> nit: "" -> std::string()

Done.

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
base/test/statistics_delta_reader.cc:18: for (size_t i = 0; i <
histograms.size(); i++) {
On 2014/01/06 23:31:35, Ilya Sherman (Away Jan11-Feb2) wrote:
> nit: i++ -> ++i

Done.

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
File base/test/statistics_delta_reader.h (right):

https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
base/test/statistics_delta_reader.h:32: // lifecycle. This isntance takes
ownership of the samples, which are deleted
On 2014/01/06 23:31:35, Ilya Sherman (Away Jan11-Feb2) wrote:
> nit: "isntance" -> "instance"

Done.

[lpromero, 2014-01-07 13:01:43]

On 2014/01/07 10:20:01, lpromero wrote:
> Thanks for the review!
> 
>
https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
> File base/test/statistics_delta_reader.cc (right):
> 
>
https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
> base/test/statistics_delta_reader.cc:17: StatisticsRecorder::GetSnapshot("",
> &histograms);
> On 2014/01/06 23:31:35, Ilya Sherman (Away Jan11-Feb2) wrote:
> > nit: "" -> std::string()
> 
> Done.
> 
>
https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
> base/test/statistics_delta_reader.cc:18: for (size_t i = 0; i <
> histograms.size(); i++) {
> On 2014/01/06 23:31:35, Ilya Sherman (Away Jan11-Feb2) wrote:
> > nit: i++ -> ++i
> 
> Done.
> 
>
https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
> File base/test/statistics_delta_reader.h (right):
> 
>
https://codereview.chromium.org/120753002/diff/270001/base/test/statistics_de...
> base/test/statistics_delta_reader.h:32: // lifecycle. This isntance takes
> ownership of the samples, which are deleted
> On 2014/01/06 23:31:35, Ilya Sherman (Away Jan11-Feb2) wrote:
> > nit: "isntance" -> "instance"
> 
> Done.

TBRing rsleevy and groby

[commit-bot: I haz the power, 2014-01-07 13:01:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lpromero@chromium.org/120753002/370001

[commit-bot: I haz the power, 2014-01-07 15:30:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lpromero@chromium.org/120753002/370001

[commit-bot: I haz the power, 2014-01-07 16:12:22]

Change committed as 243303