UMA: use a more frequent upload interval for Chromecast.

UMA: use a more frequent upload interval for Chromecast.

R=asvitkine@chromium.org
BUG=464554
Committed: https://crrev.com/7cbdcb216ea70a52c1a0d65f4bdf0348e11f0144
Cr-Commit-Position: refs/heads/master@{#320515}

[Alexei Svitkine (slow), 2015-03-06 16:43:54]

I don't like this approach. If we extrapolate, then every new user of the
component will add some new ifdef and setting to the component.

Whereas in reality, the component should not have to have changes to support new
clients.

Can we expose this as a setting through the Client interface?

[gunsch, 2015-03-07 00:19:23]

gunsch@chromium.org changed reviewers:
+ gayane@chromium.org

[gunsch, 2015-03-07 00:19:58]

You're right, this was a pretty awful first attempt, mostly inspired by how we
hacked it in downstream :)

I've added the upload interval to MetricsServiceClient now and tried to move
some of the experimental upload logic / cellular to ChromeMetricsServiceClient
in the process. Let me know if that's overstepping, but it seems appropriate to
me t hat that logic should be Chrome-specific.

+gayane for changes to recently-added code.

[Ilya Sherman, 2015-03-07 00:28:12]

On 2015/03/07 00:19:58, gunsch wrote:
> You're right, this was a pretty awful first attempt, mostly inspired by how we
> hacked it in downstream :)
> 
> I've added the upload interval to MetricsServiceClient now and tried to move
> some of the experimental upload logic / cellular to ChromeMetricsServiceClient
> in the process. Let me know if that's overstepping, but it seems appropriate
to
> me t hat that logic should be Chrome-specific.

Please note that -- confusingly -- the ChromeMetricsServiceClient is not used by
Chrome on iOS.

[gunsch, 2015-03-07 00:37:11]

On 2015/03/07 00:28:12, Ilya Sherman wrote:
> On 2015/03/07 00:19:58, gunsch wrote:
> > You're right, this was a pretty awful first attempt, mostly inspired by how
we
> > hacked it in downstream :)
> > 
> > I've added the upload interval to MetricsServiceClient now and tried to move
> > some of the experimental upload logic / cellular to
ChromeMetricsServiceClient
> > in the process. Let me know if that's overstepping, but it seems appropriate
> to
> > me t hat that logic should be Chrome-specific.
> 
> Please note that -- confusingly -- the ChromeMetricsServiceClient is not used
by
> Chrome on iOS.

Interesting---how is metrics used on iOS, then? MetricsService checks that it is
provided a non-null MetricsServiceClient, and the only implementations in
chromium/ seem to be {Cast,Chrome,Test}MetricsServiceClient.

[Alexei Svitkine (slow), 2015-03-07 01:08:54]

I think iOS still uses it right now, but the eventual plan is to only
depend on code in components/. I don't think they've made that transition
yet.

On Fri, Mar 6, 2015 at 7:37 PM, <gunsch@chromium.org> wrote:

> On 2015/03/07 00:28:12, Ilya Sherman wrote:
>
>> On 2015/03/07 00:19:58, gunsch wrote:
>> > You're right, this was a pretty awful first attempt, mostly inspired by
>> how
>>
> we
>
>> > hacked it in downstream :)
>> >
>> > I've added the upload interval to MetricsServiceClient now and tried to
>> move
>> > some of the experimental upload logic / cellular to
>>
> ChromeMetricsServiceClient
>
>> > in the process. Let me know if that's overstepping, but it seems
>> appropriate
>> to
>> > me t hat that logic should be Chrome-specific.
>>
>
>  Please note that -- confusingly -- the ChromeMetricsServiceClient is not
>> used
>>
> by
>
>> Chrome on iOS.
>>
>
> Interesting---how is metrics used on iOS, then? MetricsService checks that
> it is
> provided a non-null MetricsServiceClient, and the only implementations in
> chromium/ seem to be {Cast,Chrome,Test}MetricsServiceClient.
>
> https://codereview.chromium.org/979363003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2015-03-09 16:53:45]

The diff looks incorrect - looks like you picked up gayane's CL somehow in it?
Can you fix it?

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.h:177:
base::Callback<void(bool*)> cellular_callback_;
Looks like a bad re-base?

[gayane -on leave until 09-2017, 2015-03-09 16:56:19]

On 2015/03/09 16:53:45, Alexei Svitkine wrote:
> The diff looks incorrect - looks like you picked up gayane's CL somehow in it?
> Can you fix it?
> 
>
https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
> File chrome/browser/metrics/chrome_metrics_service_client.h (right):
> 
>
https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
> chrome/browser/metrics/chrome_metrics_service_client.h:177:
> base::Callback<void(bool*)> cellular_callback_;
> Looks like a bad re-base?

I think he added it there so that the decision on the interval can be made in
the client.

[Alexei Svitkine (slow), 2015-03-09 17:26:17]

Thanks gayane@ for pointing that out - upon closer inspection, that makes sense.

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:310: cellular_callback_
= network_metrics_provider->GetConnectionCallback();
Instead of having this callback, how about just keeping a pointer to the
NetworkMetricsProvider? Then, you can call it directly from
GetStandardUploadInterval().

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
File components/metrics/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:180: base::TimeDelta
MetricsReportingScheduler::GetUploadIntervalFromExperiment() {
Hmm, I don't think we need this here anymore. Looks like it's just being called
from Start() and the return value is being ignored.

gayane@ - could you remind me why Start() is calling this? If we're doing it
just for the side effect of this call, then this can be done in another way
(e.g. in the client). So I suggest removing this method from this file.

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
File components/metrics/metrics_reporting_scheduler.h (right):

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.h:67: base::TimeDelta
GetUploadIntervalFromExperiment();
This can be removed - see my other comment.

[gayane -on leave until 09-2017, 2015-03-09 18:18:37]

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:310: cellular_callback_
= network_metrics_provider->GetConnectionCallback();
On 2015/03/09 17:26:17, Alexei Svitkine wrote:
> Instead of having this callback, how about just keeping a pointer to the
> NetworkMetricsProvider? Then, you can call it directly from
> GetStandardUploadInterval().

In that case we need to think about how to pass a callback to
ProfilerMetricsProvider.

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
File components/metrics/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:180: base::TimeDelta
MetricsReportingScheduler::GetUploadIntervalFromExperiment() {
On 2015/03/09 17:26:17, Alexei Svitkine wrote:
> Hmm, I don't think we need this here anymore. Looks like it's just being
called
Right, we don't need that anymore. The experiment expired and I already closed
that issue. We are safe to remove it. I could do that now if needed. 

> from Start() and the return value is being ignored.
> 
> gayane@ - could you remind me why Start() is calling this? If we're doing it
> just for the side effect of this call, then this can be done in another way
> (e.g. in the client). So I suggest removing this method from this file.

[Alexei Svitkine (slow), 2015-03-09 18:20:42]

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:310: cellular_callback_
= network_metrics_provider->GetConnectionCallback();
On 2015/03/09 18:18:36, gayane wrote:
> On 2015/03/09 17:26:17, Alexei Svitkine wrote:
> > Instead of having this callback, how about just keeping a pointer to the
> > NetworkMetricsProvider? Then, you can call it directly from
> > GetStandardUploadInterval().
> 
> In that case we need to think about how to pass a callback to
> ProfilerMetricsProvider.

Ah you're right. I guess it's fine keeping the callback as-is then.

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
File components/metrics/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:180: base::TimeDelta
MetricsReportingScheduler::GetUploadIntervalFromExperiment() {
On 2015/03/09 18:18:36, gayane wrote:
> On 2015/03/09 17:26:17, Alexei Svitkine wrote:
> > Hmm, I don't think we need this here anymore. Looks like it's just being
> called
> Right, we don't need that anymore. The experiment expired and I already closed
> that issue. We are safe to remove it. I could do that now if needed. 
> 
> > from Start() and the return value is being ignored.
> > 
> > gayane@ - could you remind me why Start() is calling this? If we're doing it
> > just for the side effect of this call, then this can be done in another way
> > (e.g. in the client). So I suggest removing this method from this file.
> 

Got it - I think given that this CL is already changing this file - it makes to
do the clean up as part of it.

[gunsch, 2015-03-13 00:48:25]

Thanks for taking a look. Was OOO the last few days but back now and updated to
remove the experiment code.

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:310: cellular_callback_
= network_metrics_provider->GetConnectionCallback();
On 2015/03/09 18:20:42, Alexei Svitkine (slow) wrote:
> On 2015/03/09 18:18:36, gayane wrote:
> > On 2015/03/09 17:26:17, Alexei Svitkine wrote:
> > > Instead of having this callback, how about just keeping a pointer to the
> > > NetworkMetricsProvider? Then, you can call it directly from
> > > GetStandardUploadInterval().
> > 
> > In that case we need to think about how to pass a callback to
> > ProfilerMetricsProvider.
> 
> Ah you're right. I guess it's fine keeping the callback as-is then.

Acknowledged.

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
File components/metrics/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:180: base::TimeDelta
MetricsReportingScheduler::GetUploadIntervalFromExperiment() {
On 2015/03/09 18:20:42, Alexei Svitkine (slow) wrote:
> On 2015/03/09 18:18:36, gayane wrote:
> > On 2015/03/09 17:26:17, Alexei Svitkine wrote:
> > > Hmm, I don't think we need this here anymore. Looks like it's just being
> > called
> > Right, we don't need that anymore. The experiment expired and I already
closed
> > that issue. We are safe to remove it. I could do that now if needed. 
> > 
> > > from Start() and the return value is being ignored.
> > > 
> > > gayane@ - could you remind me why Start() is calling this? If we're doing
it
> > > just for the side effect of this call, then this can be done in another
way
> > > (e.g. in the client). So I suggest removing this method from this file.
> > 
> 
> Got it - I think given that this CL is already changing this file - it makes
to
> do the clean up as part of it.

Done.

[gunsch, 2015-03-13 00:50:13]

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/979363003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.h:177:
base::Callback<void(bool*)> cellular_callback_;
On 2015/03/09 16:53:45, Alexei Svitkine (slow) wrote:
> Looks like a bad re-base?

Nope, stored here now and referenced a few places (looks like you and gayane@
discussed on the .cc file)

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
File components/metrics/metrics_reporting_scheduler.h (right):

https://codereview.chromium.org/979363003/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.h:67: base::TimeDelta
GetUploadIntervalFromExperiment();
On 2015/03/09 17:26:17, Alexei Svitkine (slow) wrote:
> This can be removed - see my other comment.

Done.

[Alexei Svitkine (slow), 2015-03-13 02:44:53]

lgtm % comment

https://codereview.chromium.org/979363003/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/979363003/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:271: if
(!cellular_callback_.is_null())
This if shouldn't be needed anymore.

[gunsch, 2015-03-13 15:48:02]

https://codereview.chromium.org/979363003/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/979363003/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:271: if
(!cellular_callback_.is_null())
On 2015/03/13 02:44:53, Alexei Svitkine (slow) wrote:
> This if shouldn't be needed anymore.

Done.

[gunsch, 2015-03-13 15:48:13]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2015-03-13 15:48:13]

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/979363003/#ps80001 (title:
"removes conditional")

[commit-bot: I haz the power, 2015-03-13 15:48:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/979363003/80001

[commit-bot: I haz the power, 2015-03-13 17:02:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-13 17:03:43]

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