Adding reporting of metrics sampling rate.

Adding reporting of metrics sampling rate.

This rate is controlled by a variation param associated with the kMetricsReportingFeature. It is expressed as the number of clients per 1000.

BUG=609987
Committed: https://crrev.com/fbceca6039fa8d127d7b43a7e5fdb7b8f7e97a81
Cr-Commit-Position: refs/heads/master@{#409198}

[jwd, 2016-07-28 13:35:29]

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

[commit-bot: I haz the power, 2016-07-28 13:35:42]

Dry run: 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, 2016-07-28 14:07:16]

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

[commit-bot: I haz the power, 2016-07-28 14:07:17]

Dry run: This issue passed the CQ dry run.

[jwd, 2016-07-28 14:24:44]

Description was changed from

==========
Adding reporting of sampling rate.
==========

to

==========
Adding reporting of metrics sampling rate.

This rate is controlled by a variation param associated with the
kMetricsReportingFeature. It is expressed as the number of clients per 1000.

BUG=609987
==========

[jwd, 2016-07-28 14:25:01]

jwd@chromium.org changed reviewers:
+ asvitkine@chromium.org, gayane@chromium.org

[Alexei Svitkine (slow), 2016-08-01 15:49:56]

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right):

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_services_manager_client.cc:93: if
(!IsClientEligibleForSampling()) {
Nit: No {}'s


Same below for 1-liners.

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_services_manager_client.cc:111: // The
parameter can be empty either if the feature is dissabled, or the
disabled - also mispeled below

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/sampling_metrics_provider.h (right):

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/sampling_metrics_provider.h:10: namespace metrics {
Nit: Add an empty line.

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/sampling_metrics_provider.h:16: // MetricsProvider:
Add private: before this.

[jwd, 2016-08-01 16:45:30]

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right):

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_services_manager_client.cc:93: if
(!IsClientEligibleForSampling()) {
On 2016/08/01 15:49:56, Alexei Svitkine (slow) wrote:
> Nit: No {}'s
> 
> 
> Same below for 1-liners.

Done.

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_services_manager_client.cc:111: // The
parameter can be empty either if the feature is dissabled, or the
On 2016/08/01 15:49:56, Alexei Svitkine (slow) wrote:
> disabled - also mispeled below

Done.

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/sampling_metrics_provider.h (right):

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/sampling_metrics_provider.h:10: namespace metrics {
On 2016/08/01 15:49:56, Alexei Svitkine (slow) wrote:
> Nit: Add an empty line.

Done.

https://codereview.chromium.org/2180373003/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/sampling_metrics_provider.h:16: // MetricsProvider:
On 2016/08/01 15:49:56, Alexei Svitkine (slow) wrote:
> Add private: before this.

Done.

[Alexei Svitkine (slow), 2016-08-01 17:23:51]

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right):

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_services_manager_client.cc:108: if
(rate_str.empty())
This *should* have {}'s however.

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/sampling_metrics_provider.cc (right):

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/sampling_metrics_provider.cc:16: void
SamplingMetricsProvider::ProvideGeneralMetrics(
Maybe this should be ProvideStabilityMetrics - so this is also included with UMA
reports from the previous session with stability counts?

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/sampling_metrics_provider.cc:18:
UMA_HISTOGRAM_SPARSE_SLOWLY(
Should we just not log this for users who are not being sampled? I think that
would be best, since platforms where we're not doing sampling at all don't need
to send the extra histograms. Also, server-side we need to handle those cases
anyways since there will be old clients who don't send this anyway.

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/sampling_metrics_provider.h (right):

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/sampling_metrics_provider.h:12: class
SamplingMetricsProvider : public MetricsProvider {
Please add a comment about what this is for.

[jwd, 2016-08-01 18:57:15]

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right):

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_services_manager_client.cc:108: if
(rate_str.empty())
On 2016/08/01 17:23:51, Alexei Svitkine (slow) wrote:
> This *should* have {}'s however.

Done.

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/sampling_metrics_provider.cc (right):

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/sampling_metrics_provider.cc:16: void
SamplingMetricsProvider::ProvideGeneralMetrics(
On 2016/08/01 17:23:51, Alexei Svitkine (slow) wrote:
> Maybe this should be ProvideStabilityMetrics - so this is also included with
UMA
> reports from the previous session with stability counts?

Ok, so UMA_STABILITY_HISTOGRAM_ENUMERATION will just log it as usual?
Are we fine with a non-sparse histogram with 1000 buckets?

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/sampling_metrics_provider.cc:18:
UMA_HISTOGRAM_SPARSE_SLOWLY(
On 2016/08/01 17:23:51, Alexei Svitkine (slow) wrote:
> Should we just not log this for users who are not being sampled? I think that
> would be best, since platforms where we're not doing sampling at all don't
need
> to send the extra histograms. Also, server-side we need to handle those cases
> anyways since there will be old clients who don't send this anyway.

Done.

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/sampling_metrics_provider.h (right):

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/sampling_metrics_provider.h:12: class
SamplingMetricsProvider : public MetricsProvider {
On 2016/08/01 17:23:51, Alexei Svitkine (slow) wrote:
> Please add a comment about what this is for.

Done.

[Alexei Svitkine (slow), 2016-08-01 18:59:02]

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/sampling_metrics_provider.cc (right):

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/sampling_metrics_provider.cc:16: void
SamplingMetricsProvider::ProvideGeneralMetrics(
On 2016/08/01 18:57:14, jwd wrote:
> On 2016/08/01 17:23:51, Alexei Svitkine (slow) wrote:
> > Maybe this should be ProvideStabilityMetrics - so this is also included with
> UMA
> > reports from the previous session with stability counts?
> 
> Ok, so UMA_STABILITY_HISTOGRAM_ENUMERATION will just log it as usual?
> Are we fine with a non-sparse histogram with 1000 buckets?

I think we should still use a sparse histogram. You can deconstruct the sparse
macro and pass in the flag that marks it to be reported as a stability metric in
call.

[jwd, 2016-08-01 20:11:35]

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/sampling_metrics_provider.cc (right):

https://codereview.chromium.org/2180373003/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/sampling_metrics_provider.cc:16: void
SamplingMetricsProvider::ProvideGeneralMetrics(
On 2016/08/01 18:59:02, Alexei Svitkine (slow) wrote:
> On 2016/08/01 18:57:14, jwd wrote:
> > On 2016/08/01 17:23:51, Alexei Svitkine (slow) wrote:
> > > Maybe this should be ProvideStabilityMetrics - so this is also included
with
> > UMA
> > > reports from the previous session with stability counts?
> > 
> > Ok, so UMA_STABILITY_HISTOGRAM_ENUMERATION will just log it as usual?
> > Are we fine with a non-sparse histogram with 1000 buckets?
> 
> I think we should still use a sparse histogram. You can deconstruct the sparse
> macro and pass in the flag that marks it to be reported as a stability metric
in
> call.

Done.

[Alexei Svitkine (slow), 2016-08-01 20:28:27]

lgtm

https://codereview.chromium.org/2180373003/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right):

https://codereview.chromium.org/2180373003/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_services_manager_client.h:36: // rate.
Nit: Expand comment to explain that if rate is not defined, there is no
sampling. Also mention something about this only returning a sample rate for
eligible clients.

[jwd, 2016-08-02 14:26:38]

https://codereview.chromium.org/2180373003/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right):

https://codereview.chromium.org/2180373003/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_services_manager_client.h:36: // rate.
On 2016/08/01 20:28:27, Alexei Svitkine (slow) wrote:
> Nit: Expand comment to explain that if rate is not defined, there is no
> sampling. Also mention something about this only returning a sample rate for
> eligible clients.

Done.

[jwd, 2016-08-02 14:30:04]

The CQ bit was checked by jwd@chromium.org

[jwd, 2016-08-02 14:30:04]

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/2180373003/#ps160001
(title: "fix comments")

[commit-bot: I haz the power, 2016-08-02 14:30:14]

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, 2016-08-02 15:43:42]

Description was changed from

==========
Adding reporting of metrics sampling rate.

This rate is controlled by a variation param associated with the
kMetricsReportingFeature. It is expressed as the number of clients per 1000.

BUG=609987
==========

to

==========
Adding reporting of metrics sampling rate.

This rate is controlled by a variation param associated with the
kMetricsReportingFeature. It is expressed as the number of clients per 1000.

BUG=609987
==========

[commit-bot: I haz the power, 2016-08-02 15:43:44]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-02 15:45:49]

Description was changed from

==========
Adding reporting of metrics sampling rate.

This rate is controlled by a variation param associated with the
kMetricsReportingFeature. It is expressed as the number of clients per 1000.

BUG=609987
==========

to

==========
Adding reporting of metrics sampling rate.

This rate is controlled by a variation param associated with the
kMetricsReportingFeature. It is expressed as the number of clients per 1000.

BUG=609987

Committed: https://crrev.com/fbceca6039fa8d127d7b43a7e5fdb7b8f7e97a81
Cr-Commit-Position: refs/heads/master@{#409198}
==========

[commit-bot: I haz the power, 2016-08-02 15:45:50]

Patchset 9 (id:??) landed as
https://crrev.com/fbceca6039fa8d127d7b43a7e5fdb7b8f7e97a81
Cr-Commit-Position: refs/heads/master@{#409198}