Measure and send update time to UMA.

Measure and send update time to UMA.

BUG=4852
TEST=unit tests, gmerged, force update, looked at about:histograms.

[petkov, 2010-07-19 20:43:30]

There's a metrics library gmock available through the metrics package so we can
use that if/when we start testing UpdateAttempter.

Also, current max time is set at 20 minutes. Samples higher than that will still
be recorded, however, we should up the max if we think we'll routinely get
update times higher than 20 minutes. What do you think?

[adlr, 2010-07-20 00:08:57]

So, just asking to make sure: nothing bad will happen if the metrics daemon (or
anything else like that) is not running right? Just want to make sure this
doesn't impact the stability of the updater.

http://codereview.chromium.org/2868061/diff/1/4
File update_attempter.cc (right):

http://codereview.chromium.org/2868061/diff/1/4#newcode214
update_attempter.cc:214: metrics_lib_->SendToUMA("Installer.UpdateTime",
s/Installer/UpdateEngine/?

http://codereview.chromium.org/2868061/diff/1/4#newcode217
update_attempter.cc:217: 20 * 60,  // max = 20 minutes
i could definitely see the long tail of updates taking much longer, esp on a
very slow network. Can we do a log scale, or some other non-uniform scale?

[petkov, 2010-07-20 00:19:43]

The call to the metrics library will simply try to append to a binary file
/var/log/metrics/uma-events. The data will stay there until Chrome picks it up.
If the file write fails for some reason, the SendToUMA call should simply return
false.

There's no interaction with metrics daemon or other services.

So, I think it's safe.

I guess the only failure scenario is if Chrome crashes while reading the file
and keeps the lock to that file. I'm not sure if this can happen, actually.

http://codereview.chromium.org/2868061/diff/1/4
File update_attempter.cc (right):

http://codereview.chromium.org/2868061/diff/1/4#newcode214
update_attempter.cc:214: metrics_lib_->SendToUMA("Installer.UpdateTime",
On 2010/07/20 00:08:57, adlr wrote:
> s/Installer/UpdateEngine/?

The convention we've been trying to follow is to keep the group name the same as
the issue tracker focus area. I believe the focus area for the update engine is
Area-Installer?

We have, however, diverged from that convention on a few occasions. So, if you
believe that the tracker area doesn't make sense in this context, I'll switch.
Please confirm.

http://codereview.chromium.org/2868061/diff/1/4#newcode217
update_attempter.cc:217: 20 * 60,  // max = 20 minutes
On 2010/07/20 00:08:57, adlr wrote:
> i could definitely see the long tail of updates taking much longer, esp on a
> very slow network. Can we do a log scale, or some other non-uniform scale?

The default histogram layout is exponential -- smaller bucket ranges at the
lower end and bigger bucket ranges at the higher end.

[adlr, 2010-07-20 00:27:30]

Sounds pretty reasonable. I think in general it would be good for the
metrics library if a crashed chrome couldn't hold a lock on the file. Is
that something we should ask kmixter about?

LGTM

On Mon, Jul 19, 2010 at 5:19 PM, <petkov@chromium.org> wrote:

> The call to the metrics library will simply try to append to a binary file
> /var/log/metrics/uma-events. The data will stay there until Chrome picks it
> up.
> If the file write fails for some reason, the SendToUMA call should simply
> return
> false.
>
> There's no interaction with metrics daemon or other services.
>
> So, I think it's safe.
>
> I guess the only failure scenario is if Chrome crashes while reading the
> file
> and keeps the lock to that file. I'm not sure if this can happen, actually.
>
>
>
>
> http://codereview.chromium.org/2868061/diff/1/4
> File update_attempter.cc (right):
>
> http://codereview.chromium.org/2868061/diff/1/4#newcode214
> update_attempter.cc:214: metrics_lib_->SendToUMA("Installer.UpdateTime",
> On 2010/07/20 00:08:57, adlr wrote:
>
>> s/Installer/UpdateEngine/?
>>
>
> The convention we've been trying to follow is to keep the group name the
> same as the issue tracker focus area. I believe the focus area for the
> update engine is Area-Installer?
>
> We have, however, diverged from that convention on a few occasions. So,
> if you believe that the tracker area doesn't make sense in this context,
> I'll switch. Please confirm.
>
>
> http://codereview.chromium.org/2868061/diff/1/4#newcode217
> update_attempter.cc:217: 20 * 60,  // max = 20 minutes
> On 2010/07/20 00:08:57, adlr wrote:
>
>> i could definitely see the long tail of updates taking much longer,
>>
> esp on a
>
>> very slow network. Can we do a log scale, or some other non-uniform
>>
> scale?
>
> The default histogram layout is exponential -- smaller bucket ranges at
> the lower end and bigger bucket ranges at the higher end.
>
>
> http://codereview.chromium.org/2868061/show
>

[petkov, 2010-07-20 00:42:23]

On 2010/07/20 00:27:30, adlr wrote:
> Sounds pretty reasonable. I think in general it would be good for the
> metrics library if a crashed chrome couldn't hold a lock on the file. Is
> that something we should ask kmixter about?

I think the crashed process should release the lock anyway -- I'll experiment
just in case.

On the bright side -- even if a deadlock happens, you'd have an updated system
:-)

> 
> LGTM
> 
> On Mon, Jul 19, 2010 at 5:19 PM, <mailto:petkov@chromium.org> wrote:
> 
> > The call to the metrics library will simply try to append to a binary file
> > /var/log/metrics/uma-events. The data will stay there until Chrome picks it
> > up.
> > If the file write fails for some reason, the SendToUMA call should simply
> > return
> > false.
> >
> > There's no interaction with metrics daemon or other services.
> >
> > So, I think it's safe.
> >
> > I guess the only failure scenario is if Chrome crashes while reading the
> > file
> > and keeps the lock to that file. I'm not sure if this can happen, actually.
> >
> >
> >
> >
> > http://codereview.chromium.org/2868061/diff/1/4
> > File update_attempter.cc (right):
> >
> > http://codereview.chromium.org/2868061/diff/1/4#newcode214
> > update_attempter.cc:214: metrics_lib_->SendToUMA("Installer.UpdateTime",
> > On 2010/07/20 00:08:57, adlr wrote:
> >
> >> s/Installer/UpdateEngine/?
> >>
> >
> > The convention we've been trying to follow is to keep the group name the
> > same as the issue tracker focus area. I believe the focus area for the
> > update engine is Area-Installer?
> >
> > We have, however, diverged from that convention on a few occasions. So,
> > if you believe that the tracker area doesn't make sense in this context,
> > I'll switch. Please confirm.
> >
> >
> > http://codereview.chromium.org/2868061/diff/1/4#newcode217
> > update_attempter.cc:217: 20 * 60,  // max = 20 minutes
> > On 2010/07/20 00:08:57, adlr wrote:
> >
> >> i could definitely see the long tail of updates taking much longer,
> >>
> > esp on a
> >
> >> very slow network. Can we do a log scale, or some other non-uniform
> >>
> > scale?
> >
> > The default histogram layout is exponential -- smaller bucket ranges at
> > the lower end and bigger bucket ranges at the higher end.
> >
> >
> > http://codereview.chromium.org/2868061/show
> >
>

[adlr, 2010-07-20 00:45:02]

LGTM

On Mon, Jul 19, 2010 at 5:42 PM, <petkov@chromium.org> wrote:

> On 2010/07/20 00:27:30, adlr wrote:
>
>> Sounds pretty reasonable. I think in general it would be good for the
>> metrics library if a crashed chrome couldn't hold a lock on the file. Is
>> that something we should ask kmixter about?
>>
>
> I think the crashed process should release the lock anyway -- I'll
> experiment
> just in case.
>
> On the bright side -- even if a deadlock happens, you'd have an updated
> system
> :-)
>

Right now, yes, but who knows where we'll insert metrics library calls in
the future.


>
>
>  LGTM
>>
>
>  On Mon, Jul 19, 2010 at 5:19 PM, <mailto:petkov@chromium.org> wrote:
>>
>
>  > The call to the metrics library will simply try to append to a binary
>> file
>> > /var/log/metrics/uma-events. The data will stay there until Chrome picks
>> it
>> > up.
>> > If the file write fails for some reason, the SendToUMA call should
>> simply
>> > return
>> > false.
>> >
>> > There's no interaction with metrics daemon or other services.
>> >
>> > So, I think it's safe.
>> >
>> > I guess the only failure scenario is if Chrome crashes while reading the
>> > file
>> > and keeps the lock to that file. I'm not sure if this can happen,
>> actually.
>> >
>> >
>> >
>> >
>> > http://codereview.chromium.org/2868061/diff/1/4
>> > File update_attempter.cc (right):
>> >
>> > http://codereview.chromium.org/2868061/diff/1/4#newcode214
>> > update_attempter.cc:214: metrics_lib_->SendToUMA("Installer.UpdateTime",
>> > On 2010/07/20 00:08:57, adlr wrote:
>> >
>> >> s/Installer/UpdateEngine/?
>> >>
>> >
>> > The convention we've been trying to follow is to keep the group name the
>> > same as the issue tracker focus area. I believe the focus area for the
>> > update engine is Area-Installer?
>> >
>> > We have, however, diverged from that convention on a few occasions. So,
>> > if you believe that the tracker area doesn't make sense in this context,
>> > I'll switch. Please confirm.
>> >
>> >
>> > http://codereview.chromium.org/2868061/diff/1/4#newcode217
>> > update_attempter.cc:217: 20 * 60,  // max = 20 minutes
>> > On 2010/07/20 00:08:57, adlr wrote:
>> >
>> >> i could definitely see the long tail of updates taking much longer,
>> >>
>> > esp on a
>> >
>> >> very slow network. Can we do a log scale, or some other non-uniform
>> >>
>> > scale?
>> >
>> > The default histogram layout is exponential -- smaller bucket ranges at
>> > the lower end and bigger bucket ranges at the higher end.
>> >
>> >
>> > http://codereview.chromium.org/2868061/show
>> >
>>
>
>
>
>
> http://codereview.chromium.org/2868061/show
>