Merge subprocess metrics into global StatisticsRecorder.

Merge subprocess metrics into global StatisticsRecorder.

BUG=546019
Committed: https://crrev.com/05dc092bb18cf559b7959bf39072a96c0faf5f58
Cr-Commit-Position: refs/heads/master@{#397637}

[bcwhite, 2016-06-01 15:06:03]

Patchset #2 (id:20001) has been deleted

[bcwhite, 2016-06-01 16:00:44]

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

[Alexei Svitkine (slow), 2016-06-01 22:02:15]

Sorry, still reviewing this, but here's what I have so far. (Not much yet..)

https://codereview.chromium.org/2023253002/diff/40001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/2023253002/diff/40001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:487: void
PersistentHistogramAllocator::MergeHistogramToStatisticsRecorder(
Can you add unit test coverage for this function? (outside of the metrics
provider itself context)

https://codereview.chromium.org/2023253002/diff/40001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:490: // created there are already
within the global statistics recorder.
Please reproduce this comment in the header file comment for this function.

[Alexei Svitkine (slow), 2016-06-02 17:24:09]

https://codereview.chromium.org/2023253002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/2023253002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/subprocess_metrics_provider.cc:50:
MergeHistogramDeltasFromAllocator(id, allocator.get());
Is it the case that if metrics reporting is disabled, we'll still merge the
metrics to the statistics recorder - but only here at process exit? (And I guess
also from chrome://histograms)

If that's the case, I think it would be useful to have a comment about this to
help the reader understand the system better.

[bcwhite, 2016-06-02 19:19:26]

https://codereview.chromium.org/2023253002/diff/40001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/2023253002/diff/40001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:487: void
PersistentHistogramAllocator::MergeHistogramToStatisticsRecorder(
On 2016/06/01 22:02:15, Alexei Svitkine (slow) wrote:
> Can you add unit test coverage for this function? (outside of the metrics
> provider itself context)

Done.

https://codereview.chromium.org/2023253002/diff/40001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:490: // created there are already
within the global statistics recorder.
On 2016/06/01 22:02:15, Alexei Svitkine (slow) wrote:
> Please reproduce this comment in the header file comment for this function.

Done.

https://codereview.chromium.org/2023253002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/2023253002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/subprocess_metrics_provider.cc:50:
MergeHistogramDeltasFromAllocator(id, allocator.get());
On 2016/06/02 17:24:09, Alexei Svitkine (slow) wrote:
> Is it the case that if metrics reporting is disabled, we'll still merge the
> metrics to the statistics recorder - but only here at process exit? (And I
guess
> also from chrome://histograms)
> 
> If that's the case, I think it would be useful to have a comment about this to
> help the reader understand the system better.

Done.

[Alexei Svitkine (slow), 2016-06-02 19:22:30]

lgtm

https://codereview.chromium.org/2023253002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/2023253002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/subprocess_metrics_provider.cc:50:
MergeHistogramDeltasFromAllocator(id, allocator.get());
On 2016/06/02 19:19:26, bcwhite wrote:
> On 2016/06/02 17:24:09, Alexei Svitkine (slow) wrote:
> > Is it the case that if metrics reporting is disabled, we'll still merge the
> > metrics to the statistics recorder - but only here at process exit? (And I
> guess
> > also from chrome://histograms)
> > 
> > If that's the case, I think it would be useful to have a comment about this
to
> > help the reader understand the system better.
> 
> Done.

I actually meant to specifically call this out in a comment inside this
function. The comment you added about the class is good too, but add the
specific info about non-UMA opted case here as well, please.

[bcwhite, 2016-06-03 03:55:35]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-06-03 03:55:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2023253002/#ps100001
(title: "improved comment about when merges occur")

[commit-bot: I haz the power, 2016-06-03 03:55:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023253002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2023253002/100001

[commit-bot: I haz the power, 2016-06-03 04:59:55]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-03 05:01:29]

Description was changed from

==========
Merge subprocess metrics into global StatisticsRecorder.

BUG=546019
==========

to

==========
Merge subprocess metrics into global StatisticsRecorder.

BUG=546019

Committed: https://crrev.com/05dc092bb18cf559b7959bf39072a96c0faf5f58
Cr-Commit-Position: refs/heads/master@{#397637}
==========

[commit-bot: I haz the power, 2016-06-03 05:01:33]

Patchset 5 (id:??) landed as
https://crrev.com/05dc092bb18cf559b7959bf39072a96c0faf5f58
Cr-Commit-Position: refs/heads/master@{#397637}

[bcwhite, 2016-06-03 19:43:22]

https://codereview.chromium.org/2023253002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/2023253002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/subprocess_metrics_provider.cc:50:
MergeHistogramDeltasFromAllocator(id, allocator.get());
On 2016/06/02 19:22:30, Alexei Svitkine (slow) wrote:
> On 2016/06/02 19:19:26, bcwhite wrote:
> > On 2016/06/02 17:24:09, Alexei Svitkine (slow) wrote:
> > > Is it the case that if metrics reporting is disabled, we'll still merge
the
> > > metrics to the statistics recorder - but only here at process exit? (And I
> > guess
> > > also from chrome://histograms)
> > > 
> > > If that's the case, I think it would be useful to have a comment about
this
> to
> > > help the reader understand the system better.
> > 
> > Done.
> 
> I actually meant to specifically call this out in a comment inside this
> function. The comment you added about the class is good too, but add the
> specific info about non-UMA opted case here as well, please.

Done.