Add UMA Histogram Manager to Cronet.

Add UMA Histogram Manager to Cronet.

BUG=428929

[mef, 2014-12-01 19:34:22]

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

[mef, 2014-12-01 19:34:22]

Hi Alexei,

could you take a look to check that I'm going in the right direction? 
Should I make HistogramManager a singleton? 
Where/how do I need to hook up pref service for serialization?
Are there any threading considerations?

[Alexei Svitkine (slow), 2014-12-03 16:46:53]

Here's an initial set of comments.

I think we can worry about pref service serialization later (not for this CL).

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
File components/cronet/android/histogram_manager.cc (right):

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
components/cronet/android/histogram_manager.cc:36: new
base::HistogramSnapshotManager(histogram_flattener_.get()));
Instead of creating a flattener,  HistogramManager should just implement
base::HistogramFlattener.

Then, RecordDelta() will be called on HistogramManager, which can then call
RecordHistogramDelta() from that method, passing histogram.histogram_name() as
the first param.

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
components/cronet/android/histogram_manager.cc:42: void
HistogramManager::RecordHistogramDelta(const std::string& histogram_name,
I think we should avoid duplicating this code - so that we make sure to keep the
code in sync as it evolves.

Here's my suggestion:

Create a new file in components/metrics:

histogram_encoder.cc

We can then define this as a stand-alone method in there that takes uma_proto as
an additional param as a pointer.

Then, both this file and metrics_log.cc can call that version of the code.

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
components/cronet/android/histogram_manager.cc:84:
RecordHistogramDelta(kCronetHistogramName, *snapshot.get());
You shouldn't need to call this manually. Instead, PrepareDeltas() will call
back to its flattener - which per my comment above this class should implement
and call RecordHistogramDeltas from its RecordDelta method.

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
components/cronet/android/histogram_manager.cc:89: scoped_ptr<HistogramManager>
histogram_manager(new HistogramManager());
This isn't right. We need to get the deltas only since the last call - which
means that HistogramManager object needs to be kept around.

So we can either make it a singleton or I think preferably if it could hang off
of the Java object (e.g. using a long to store the address of the object).

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
File components/cronet/android/histogram_manager.h (right):

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
components/cronet/android/histogram_manager.h:21: };
Nit: No need for }

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
components/cronet/android/histogram_manager.h:32: void UpdateUmaProto();
Please add some comments on the methods and on the class (i.e. how the class is
meant to be used).

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
components/cronet/android/histogram_manager.h:45:
scoped_ptr<base::HistogramSnapshotManager> histogram_snapshot_manager_;
I don't think this needs to be a scoped_ptr - can just be a member.

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/HistogramManager.java
(right):

https://codereview.chromium.org/769873003/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/HistogramManager.java:18: *
Get histogram deltas serialized as protobuf.
Mention that it's the deltas since the last call.

[Alexei Svitkine (slow), 2014-12-09 21:59:11]

FYI: This is my last week of work for this year, afterwards I'll be away on
vacation.

Happy to iterate on this review this week and get it landed before my vacation
if you are too. :)

[ramant (doing other things), 2014-12-10 03:38:43]

On 2014/12/09 21:59:11, Alexei Svitkine wrote:
> FYI: This is my last week of work for this year, afterwards I'll be away on
> vacation.
> 
> Happy to iterate on this review this week and get it landed before my vacation
> if you are too. :)

Hi Alexei,
  I have made most of the changes you had suggested (used singleton. We could
change it to a pointed in Java code). This is work in progress. Would like to
add some more tests. I haven't tested cronet side of the code (will work mef to
do that). If these changes look ok, will add histograms to hostograms.xml

[ramant (doing other things), 2014-12-10 03:39:46]

On 2014/12/10 03:38:43, ramant wrote:
> On 2014/12/09 21:59:11, Alexei Svitkine wrote:
> > FYI: This is my last week of work for this year, afterwards I'll be away on
> > vacation.
> > 
> > Happy to iterate on this review this week and get it landed before my
vacation
> > if you are too. :)
> 
> Hi Alexei,
>   I have made most of the changes you had suggested (used singleton. We could
> change it to a pointed in Java code). This is work in progress. Would like to
> add some more tests. I haven't tested cronet side of the code (will work mef
to
> do that). If these changes look ok, will add histograms to hostograms.xml

The following is the CL, where I made the changes (work in progress, will try to
complete it tomorrow with mef).

https://codereview.chromium.org/794493002/