Log time between network drops -- from online to offline.

Log time between network drops -- from online to offline.

Rewrite most of metrics_daemon. Convert to low-level D-Bus API -- this
simplifies the code a little and also allows us to catch the power
state signal. I still suspect we may be abusing D-Bus a little but it
seems to work.

snanda@ -- please review the power state code specifically.

BUG=none
TEST=tested on target platform and arm-generic builds

[petkov, 2010-04-26 21:50:41]

Uploaded patch 2.

[Sameer Nanda, 2010-04-26 22:06:24]

LGTM w/ nits for power state related code.

http://codereview.chromium.org/1799001/diff/3001/1012
File src/platform/metrics/metrics_daemon.cc (right):

http://codereview.chromium.org/1799001/diff/3001/1012#newcode138
src/platform/metrics/metrics_daemon.cc:138: // dropping. Don't log going offline
as part of suspend-to-memory as
nit: suspend-to-RAM is a more common usage.

http://codereview.chromium.org/1799001/diff/3001/1012#newcode171
src/platform/metrics/metrics_daemon.cc:171: MetricsDaemon::GetPowerState(const
char* state_name) {
nit: change method name to something like MapPowerState? GetPowerState seems to
mean that it actually fetches the current power state.

[Luigi Semenzato, 2010-04-26 22:45:51]

Comment style nitpicks, plus one issue about the relationship of network states
and power states.

Also thank you for running that dbus performance test.

http://codereview.chromium.org/1799001/diff/3001/1012
File src/platform/metrics/metrics_daemon.cc (right):

http://codereview.chromium.org/1799001/diff/3001/1012#newcode141
src/platform/metrics/metrics_daemon.cc:141: // seems to and should be the case.
I am wondering about this.  If the network drops (for unrelated reasons) just
before the system goes to sleep, then that time is going to be counted as
offline, which may be unfair because given a chance it might reacquire the
network pretty quickly.  Perhaps you need similar code in the PowerStateChanged
callback?

Also, is it possible to ignore these issues and statistically reconstruct what
happened (by subtracting the histograms, for some meaning of "subtract").

http://codereview.chromium.org/1799001/diff/3001/1013
File src/platform/metrics/metrics_daemon.h (right):

http://codereview.chromium.org/1799001/diff/3001/1013#newcode53
src/platform/metrics/metrics_daemon.h:53: // Process network state change.
In this comment, and in other places, you use the imperative, which is what I
personally prefer, but it's not Google's style, which is to use the third
person.  So if you want to be googley, please convert all imperatives to third
person and live with the weirdness.

[petkov, 2010-04-27 00:27:21]

Thanks for the quick review. PTAL.

http://codereview.chromium.org/1799001/diff/3001/1012
File src/platform/metrics/metrics_daemon.cc (right):

http://codereview.chromium.org/1799001/diff/3001/1012#newcode138
src/platform/metrics/metrics_daemon.cc:138: // dropping. Don't log going offline
as part of suspend-to-memory as
On 2010/04/26 22:06:24, sameer wrote:
> nit: suspend-to-RAM is a more common usage.

Done.

http://codereview.chromium.org/1799001/diff/3001/1012#newcode141
src/platform/metrics/metrics_daemon.cc:141: // seems to and should be the case.
On 2010/04/26 22:45:51, Luigi Semenzato wrote:
> I am wondering about this.  If the network drops (for unrelated reasons) just
> before the system goes to sleep, then that time is going to be counted as
> offline, which may be unfair because given a chance it might reacquire the
> network pretty quickly.  Perhaps you need similar code in the
PowerStateChanged
> callback?

Note that this histogram is not intended to measure the online time. It's
intended to measure the mean time to network dropping. So the case you describe
is a real network drop that should be reported. I.e., for the purposes of this
metric, the fact that it might reacquire the network pretty quickly is not
relevant.

> 
> Also, is it possible to ignore these issues and statistically reconstruct what
> happened (by subtracting the histograms, for some meaning of "subtract").

http://codereview.chromium.org/1799001/diff/3001/1012#newcode171
src/platform/metrics/metrics_daemon.cc:171: MetricsDaemon::GetPowerState(const
char* state_name) {
On 2010/04/26 22:06:24, sameer wrote:
> nit: change method name to something like MapPowerState? GetPowerState seems
to
> mean that it actually fetches the current power state.

Done.

http://codereview.chromium.org/1799001/diff/3001/1013
File src/platform/metrics/metrics_daemon.h (right):

http://codereview.chromium.org/1799001/diff/3001/1013#newcode53
src/platform/metrics/metrics_daemon.h:53: // Process network state change.
On 2010/04/26 22:45:51, Luigi Semenzato wrote:
> In this comment, and in other places, you use the imperative, which is what I
> personally prefer, but it's not Google's style, which is to use the third
> person.  So if you want to be googley, please convert all imperatives to third
> person and live with the weirdness.

Done.

[Luigi Semenzato, 2010-04-27 00:53:46]

LGTM

Optionally fix the imperatives in the shell script too.

I also wonder if it wouldn't be useful to histogram the duration of the offline
intervals.

[petkov, 2010-04-27 18:06:30]

On 2010/04/27 00:53:46, Luigi Semenzato wrote:
> LGTM
> 
> Optionally fix the imperatives in the shell script too.

Done.

> 
> I also wonder if it wouldn't be useful to histogram the duration of the
offline
> intervals.

Might be, separate CL. Trying to go through P0s now...

Pushed.

Thanks.