Connect SparseHistogram with the rest of stats system

Connect SparseHistogram with the rest of stats system

With this CL, SparseHistogram is usable with SparseHistogram::FactoryGet.
Next step is to implement a Histogram like macro and implement WriteHTMLGraph
and WriteAscii to have a nice output.

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

[kaiwang, 2013-02-07 05:24:02]

Please take a look.

I'll take a 2 weeks' vacation, so it's not a hurry :)

[Ilya Sherman, 2013-02-21 05:36:06]

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram.cc#...
base/metrics/histogram.cc:108: }
Please check with Raman whether he's ok with removing this code now.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_fla...
File base/metrics/histogram_flattener.h (right):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_fla...
base/metrics/histogram_flattener.h:27: // Will be called each time a type of
Inconsistenies is seen on a histogram,
nit: Please update the comment.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_fla...
base/metrics/histogram_flattener.h:31: // Will be called when a type of
Inconsistenies is seen for the first time
nit: Please update the comment.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_sna...
File base/metrics/histogram_snapshot_manager.cc (right):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:50: int corruption =
histogram.FindCorruption(*snapshot);
int32?

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:70:
static_cast<HistogramInconsistency>(corruption));
Seems like if you're willing to cast here, you might as well just have the
return type for HistogramBase::FindCorruption be HistogramInconsistency.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
File base/metrics/sparse_histogram.cc (right):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
base/metrics/sparse_histogram.cc:10: #include "base/stringprintf.h"
nit: Doesn't look like this is needed.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
base/metrics/sparse_histogram.cc:32: CHECK_EQ(SPARSE_HISTOGRAM,
histogram->GetHistogramType());
nit: DCHECK_EQ

https://codereview.chromium.org/12207058/diff/1001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/12207058/diff/1001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_service.cc:160: #include
"base/metrics/sparse_histogram.h"
nit: Doesn't seem like this is needed.

[kaiwang, 2013-02-27 04:39:42]

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram.cc#...
base/metrics/histogram.cc:108: }
emailed him

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_fla...
File base/metrics/histogram_flattener.h (right):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_fla...
base/metrics/histogram_flattener.h:27: // Will be called each time a type of
Inconsistenies is seen on a histogram,
On 2013/02/21 05:36:06, Ilya Sherman wrote:
> nit: Please update the comment.

Done.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_fla...
base/metrics/histogram_flattener.h:31: // Will be called when a type of
Inconsistenies is seen for the first time
On 2013/02/21 05:36:06, Ilya Sherman wrote:
> nit: Please update the comment.

Done.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_sna...
File base/metrics/histogram_snapshot_manager.cc (right):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:50: int corruption =
histogram.FindCorruption(*snapshot);
On 2013/02/21 05:36:06, Ilya Sherman wrote:
> int32?

Done.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:70:
static_cast<HistogramInconsistency>(corruption));
the value FindCorruption returns can be a combination of HistogramInconsistency.
As the comment above mentioned, in this case, only one type of inconsistency can
happen, hence it's directly casted.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
File base/metrics/sparse_histogram.cc (right):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
base/metrics/sparse_histogram.cc:10: #include "base/stringprintf.h"
On 2013/02/21 05:36:06, Ilya Sherman wrote:
> nit: Doesn't look like this is needed.

Done.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
base/metrics/sparse_histogram.cc:32: CHECK_EQ(SPARSE_HISTOGRAM,
histogram->GetHistogramType());
This is following histogram.h
Seems this kind of errors are treated as serious enough to CHECK

https://codereview.chromium.org/12207058/diff/1001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/12207058/diff/1001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_service.cc:160: #include
"base/metrics/sparse_histogram.h"
On 2013/02/21 05:36:06, Ilya Sherman wrote:
> nit: Doesn't seem like this is needed.

Done.

[kaiwang, 2013-02-27 18:24:26]

OWNERS, please review this change:

isherman: base/metrics/
jar: net/
brettw: chrome/ and content/
keybuk: dbus/

Thanks a lot!

[jar (doing other things), 2013-02-27 23:51:37]

Nit to consider below (which I stumbled over while looking at net/*)

LGTM for patch set 4 and net/*

https://codereview.chromium.org/12207058/diff/13001/base/metrics/histogram_ba...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/12207058/diff/13001/base/metrics/histogram_ba...
base/metrics/histogram_base.h:119: virtual int32 FindCorruption(const
HistogramSamples& samples) const;
nit: I see why you made it an integral return type, rather than an enum, so as
to avoid the cast on the OR of enumeators.

Why int32?  Why not the more common int, or if you really care about the size
(relative to the flags you pass), then int16?

[Ilya Sherman, 2013-02-28 00:29:55]

LGTM.

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
File base/metrics/sparse_histogram.cc (right):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
base/metrics/sparse_histogram.cc:32: CHECK_EQ(SPARSE_HISTOGRAM,
histogram->GetHistogramType());
On 2013/02/27 04:39:42, kaiwang wrote:
> This is following histogram.h
> Seems this kind of errors are treated as serious enough to CHECK

They shouldn't be.  It would be good to fix histogram.h as well.

[kaiwang, 2013-02-28 04:58:04]

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
File base/metrics/sparse_histogram.cc (right):

https://codereview.chromium.org/12207058/diff/1001/base/metrics/sparse_histog...
base/metrics/sparse_histogram.cc:32: CHECK_EQ(SPARSE_HISTOGRAM,
histogram->GetHistogramType());
On 2013/02/28 00:29:56, Ilya Sherman wrote:
> On 2013/02/27 04:39:42, kaiwang wrote:
> > This is following histogram.h
> > Seems this kind of errors are treated as serious enough to CHECK
> 
> They shouldn't be.  It would be good to fix histogram.h as well.

Done.

https://codereview.chromium.org/12207058/diff/13001/base/metrics/histogram_ba...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/12207058/diff/13001/base/metrics/histogram_ba...
base/metrics/histogram_base.h:119: virtual int32 FindCorruption(const
HistogramSamples& samples) const;
On 2013/02/27 23:51:38, jar wrote:
> nit: I see why you made it an integral return type, rather than an enum, so as
> to avoid the cast on the OR of enumeators.
> 
> Why int32?  Why not the more common int, or if you really care about the size
> (relative to the flags you pass), then int16?

Done.

[kaiwang, 2013-02-28 18:14:34]

kindly ping brettw and keybuk...

[keybuk, 2013-02-28 21:42:03]

lgtm for dbus/

sorry for the delay, this one slipped through the tracks

[brettw, 2013-02-28 22:56:25]

LGTM with a few suggestions.

https://codereview.chromium.org/12207058/diff/21002/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/12207058/diff/21002/base/metrics/histogram.h#...
base/metrics/histogram.h:416: virtual int FindCorruption(const HistogramSamples&
samples) const OVERRIDE;
This should be moved to the HistogramBase overrides section (and you can delete
the comment). Does this comment need to be moved to the base class? There's some
stuff about race conditions.

https://codereview.chromium.org/12207058/diff/21002/base/metrics/histogram_ba...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/12207058/diff/21002/base/metrics/histogram_ba...
base/metrics/histogram_base.h:53: NO_INCONSISTENCIES = 0x0,
This creates things named base::COUNT_LOW_ERROR, for example, which isn't
sufficiently unique or self-documenting. Either this should be a member of
HistogramBase or it needs some kind of unique prefix. I'd do the member like it
used to be in Histogram.

[kaiwang, 2013-03-01 00:52:12]

https://codereview.chromium.org/12207058/diff/21002/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/12207058/diff/21002/base/metrics/histogram.h#...
base/metrics/histogram.h:416: virtual int FindCorruption(const HistogramSamples&
samples) const OVERRIDE;
This function is not pure virtual in HistogramBase, and this comment is only for
Histogram corruption. This is way I keep it in this way...

https://codereview.chromium.org/12207058/diff/21002/base/metrics/histogram_ba...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/12207058/diff/21002/base/metrics/histogram_ba...
base/metrics/histogram_base.h:53: NO_INCONSISTENCIES = 0x0,
On 2013/02/28 22:56:25, brettw wrote:
> This creates things named base::COUNT_LOW_ERROR, for example, which isn't
> sufficiently unique or self-documenting. Either this should be a member of
> HistogramBase or it needs some kind of unique prefix. I'd do the member like
it
> used to be in Histogram.

Done.

[commit-bot: I haz the power, 2013-03-01 01:14:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12207058/20007

[commit-bot: I haz the power, 2013-03-01 01:14:20]

Failed to apply patch for content/browser/histogram_message_filter.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/histogram_message_filter.cc
  Hunk #1 FAILED at 51.
  1 out of 1 hunk FAILED -- saving rejects to file
content/browser/histogram_message_filter.cc.rej

Patch:       content/browser/histogram_message_filter.cc
Index: content/browser/histogram_message_filter.cc
diff --git a/content/browser/histogram_message_filter.cc
b/content/browser/histogram_message_filter.cc
index
733ee87931143434fec6dfd96ad9945308c8caae..e5a021544c27df8e0eb889d076c5601893bf7e01
100644
--- a/content/browser/histogram_message_filter.cc
+++ b/content/browser/histogram_message_filter.cc
@@ -51,7 +51,8 @@ void HistogramMessageFilter::OnGetBrowserHistogram(
     return;
   if (name != "Memory.BrowserUsed")
     return;
-  base::Histogram* histogram = base::StatisticsRecorder::FindHistogram(name);
+  base::HistogramBase* histogram =
+      base::StatisticsRecorder::FindHistogram(name);
   if (!histogram) {
     *histogram_json = "{}";
   } else {

[commit-bot: I haz the power, 2013-03-01 01:38:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12207058/20008

[commit-bot: I haz the power, 2013-03-01 03:53:28]

Change committed as 185451