Histogram for recording actual log upload interval added.

Histogram for recording actual log upload interval added.

Histogram is in minutes. In case logs are uploaded by multiple tasks the interval between those is not recorded.

BUG=421543
Committed: https://crrev.com/aa0b30fe93d8dbcf54cb693918c46f95f933d954
Cr-Commit-Position: refs/heads/master@{#299902}

[gayane -on leave until 09-2017, 2014-10-15 19:43:12]

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

[gayane -on leave until 09-2017, 2014-10-15 19:43:36]

Please have a look.

[Alexei Svitkine (slow), 2014-10-15 20:44:19]

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
File components/metrics/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.cc:69:
base::TimeDelta::FromHours(4).InMinutes(),
Let's make this longer to ensure we catch any outliers - how about 12h?

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.cc:70: 100);
Let's use 50 buckets - I'm not convinced we need 100.

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.cc:92:
last_upload_finish_time_(),
This line is not necessary if there's a default ctor for the field.

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.cc:168:
last_upload_finish_time_.FromInternalValue(0);
FromInternalValue is a static method that returns a new object, so this isn't
doing what you think it's doing.

I think you just want:

last_upload_finish_time_ = base::TimeTicks();

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
File components/metrics/metrics_reporting_scheduler.h (right):

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.h:69: // The tick count of the
last time log upload has been finished and 0 if no
Nit: "and 0" -> "and null"

[gayane -on leave until 09-2017, 2014-10-15 21:02:00]

Fixed.

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
File components/metrics/metrics_reporting_scheduler.cc (right):

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.cc:69:
base::TimeDelta::FromHours(4).InMinutes(),
On 2014/10/15 20:44:19, Alexei Svitkine wrote:
> Let's make this longer to ensure we catch any outliers - how about 12h?

Done.

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.cc:70: 100);
On 2014/10/15 20:44:19, Alexei Svitkine wrote:
> Let's use 50 buckets - I'm not convinced we need 100.

Done.

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.cc:92:
last_upload_finish_time_(),
On 2014/10/15 20:44:19, Alexei Svitkine wrote:
> This line is not necessary if there's a default ctor for the field.

Done.

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.cc:168:
last_upload_finish_time_.FromInternalValue(0);
On 2014/10/15 20:44:19, Alexei Svitkine wrote:
> FromInternalValue is a static method that returns a new object, so this isn't
> doing what you think it's doing.
> 
> I think you just want:
> 
> last_upload_finish_time_ = base::TimeTicks();

Done.

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
File components/metrics/metrics_reporting_scheduler.h (right):

https://codereview.chromium.org/633373011/diff/1/components/metrics/metrics_r...
components/metrics/metrics_reporting_scheduler.h:69: // The tick count of the
last time log upload has been finished and 0 if no
On 2014/10/15 20:44:19, Alexei Svitkine wrote:
> Nit: "and 0" -> "and null"

Done.

[Alexei Svitkine (slow), 2014-10-15 21:14:45]

LGTM with 2 more comments for you to address.

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

https://codereview.chromium.org/633373011/diff/20001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:66:
UMA_HISTOGRAM_CUSTOM_COUNTS("UMA.ActualLogUploadInterval",
Nit: I think this is indented 1 space too much. Please fix.

https://codereview.chromium.org/633373011/diff/20001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:167: last_upload_finish_time_=
base::TimeTicks();
Nit: Space =.

[gayane -on leave until 09-2017, 2014-10-16 14:25:19]

Fixed. Committing now.

[gayane -on leave until 09-2017, 2014-10-16 14:25:37]

The CQ bit was checked by gayane@chromium.org

[commit-bot: I haz the power, 2014-10-16 14:26:32]

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

[Alexei Svitkine (slow), 2014-10-16 14:50:17]

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

https://codereview.chromium.org/633373011/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:66:
UMA_HISTOGRAM_CUSTOM_COUNTS("UMA.ActualLogUploadInterval",
This still looks too indented, please fix.

[Alexei Svitkine (slow), 2014-10-16 14:50:21]

The CQ bit was unchecked by asvitkine@chromium.org

[gayane -on leave until 09-2017, 2014-10-16 15:05:42]

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

https://codereview.chromium.org/633373011/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:66:
UMA_HISTOGRAM_CUSTOM_COUNTS("UMA.ActualLogUploadInterval",
On 2014/10/16 14:50:17, Alexei Svitkine wrote:
> This still looks too indented, please fix.

I don't see it. Arguments start right under the bracket.

[gayane -on leave until 09-2017, 2014-10-16 15:28:25]

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

https://codereview.chromium.org/633373011/diff/40001/components/metrics/metri...
components/metrics/metrics_reporting_scheduler.cc:66:
UMA_HISTOGRAM_CUSTOM_COUNTS("UMA.ActualLogUploadInterval",
On 2014/10/16 14:50:17, Alexei Svitkine wrote:
> This still looks too indented, please fix.

Done.

[gayane -on leave until 09-2017, 2014-10-16 15:35:13]

The CQ bit was checked by gayane@chromium.org

[commit-bot: I haz the power, 2014-10-16 15:35:35]

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

[commit-bot: I haz the power, 2014-10-16 16:35:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-16 16:36:25]

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