Switch MetricsReportingScheduler to using an actual timer object

Switch MetricsReportingScheduler to using an actual timer object

This simplifies the code a bit, and makes it possible to handle Stop() more cleanly.

BUG=None
TEST=Existing metrics all continue to work as normal


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124656

[stuartmorgan, 2011-05-10 19:27:11]

(including Ben to satisfy OWNERS; metrics still needs an OWNERS file)

[stuartmorgan, 2011-05-24 01:30:00]

Ping?

[jar (doing other things), 2011-05-25 00:16:44]

Can you motivate this a bit more?  Are we transitioning away from using task
factories with delayed posts?  Are we uniformly moving to use this pattern
instead?

Can you cite the advantages?

This source file you're suggesting to change is IMO quite convoluted, but this
aspect is the least of its troubles IMO, and is relatively straight forward (and
working well).

Thanks,

Jim

[jar (doing other things), 2011-05-25 19:36:28]

Based on offline discussion, it appears that the issue is that you want to
resume an app in similar state, and you want control of the state externally (to
save and restore).

Is that the goal?

Note that if you are aiming for this, then it is very critical that you do a
graceful shutdown of the logging system, which will save any unsent logs, rather
than discard them.  Is that all in the works as well?  That graceful saving
currently cancels (effectively) the pending timer, rather than pausing it.

If you can some how freeze-dry the entire process, and potentially restore it,
then you could get away with a state save of the timer.  However, if there was a
risk that the freeze-dry *might* be discarded, I think it would be much more
correct to use the current log state-save mechanism which works across browser
shutdowns and restarts (and could be invoked across a freeze dry with relative
ease).

Also note that at restart, chrome (currently) restarts this log-upload timer on
a MUCH shorter interval initially.  If you rely on this restart, it would be a
mistake to reset the timer to a paused (remnant) duration.  

As a result, rather than pausing this timer, I *think* you really need to shut
down the metrics logging system (inducing a save).  The restart of the browser
will restart the timer correctly (60 seconds post start for the initial log
upload).

It would IMO be a mistake to restart the time at the same (relative) time when
restarting, unless you handled a complete save of logging state (outside the
current state saves).

Can you clarify how you're planning on handling this??

Thanks,

Jim

[stuartmorgan, 2011-05-25 20:38:54]

On 2011/05/25 19:36:28, jar wrote:
> Note that if you are aiming for this, then it is very critical that you do a
> graceful shutdown of the logging system, which will save any unsent logs,
rather
> than discard them.  Is that all in the works as well?

Hm, it sounds like I misunderstood how MetricsService behaves in the face of a
crash. I'll need to step back, think about this more, and then follow up
offline.


So getting back to this specific CL: I think it's a cleaner expression of the
the scheduler object to have its "Stop" actually cause it not to trigger more
uploads, rather than relying on the fact that MetricsService has its own
started/stopped state and will know to ignore some upload requests. The current
situation is fragile, and I actually did find it to be a source of confusion
when I was trying to learn the MetricsServer code. So from that standpoint I
think this change is worthwhile absent any other motivation.

Is that sufficient motivation, or should I just close this CL?

[stuartmorgan, 2012-02-14 09:39:53]

Reviving a really old CL. First, I do still think this comment is valid:

On 2011/05/25 20:38:54, stuartmorgan wrote:
> So getting back to this specific CL: I think it's a cleaner expression of the
> the scheduler object to have its "Stop" actually cause it not to trigger more
> uploads, rather than relying on the fact that MetricsService has its own
> started/stopped state and will know to ignore some upload requests. The
current
> situation is fragile, and I actually did find it to be a source of confusion
> when I was trying to learn the MetricsServer code. So from that standpoint I
> think this change is worthwhile absent any other motivation.

More importantly (and the reason I'm reviving it), we actually do need Stop to
take effect. Per your recommendation earlier in this CL's discussion,
save/restore now uses a shut down and then restart of most of the metrics
internals. The problem is that if calling Stop doesn't prevent a callback, then
instead of the later Start causing a clean slate with the right interval, it
will cause the delayed callback to take effect.

I'm not sure if that's clear, so an example:
1) Start a 10 minute timer
2) Do a partial metrics shutdown, including cutting a log and calling Stop
3) Suspend the process
4) Resume the process 15 minutes later, including calling Start

We don't want the old 10 minute callback to immediately fire because >10 mins
has elapsed. Instead the scheduler should be able to start with a clean slate
and set a reasonable interval.

[stuartmorgan, 2012-03-01 13:03:14]

FYI, this change is orthogonal to the create/upload separation. This is
necessary purely for stop and restart to work as expected when used frequently.

[jar (doing other things), 2012-03-01 19:53:15]

lgtm

[commit-bot: I haz the power, 2012-03-02 09:10:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/7001003/10001

[commit-bot: I haz the power, 2012-03-02 16:04:48]

Change committed as 124656