Factor a scheduler object out of MetricsService.

Factor a scheduler object out of MetricsService.

The logic for when to upload is now separate from the core of MetricsService. For now the object is internally constructed so that the MetricsService interface doesn't change, but it could easily be pulled out later if we need DI-style construction.

Fixes a bug where backoff for server errors would be bypassed if the logs were large.

Also removes a bit of dead code I missed when removing the server response parsing in the last CL.

BUG=None
TEST=Metrics continue to show up on the dashboards unchanged.

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

[jar (doing other things), 2011-04-16 02:03:13]

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_reporting_scheduler.cc (right):

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_reporting_scheduler.cc:43:
MetricsReportingScheduler::~MetricsReportingScheduler() {}
Why bother with a destructor?  ...or at best, list it in the header explicitly
(so that reader doesn't have to wonder what it does).

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_reporting_scheduler.cc:94:
!metrics_service_->recording_active())
I thought you reduced these two items into a single state variable in a prior
CL.  Am I confused?

In addition, you could leave this state checking to metrics_service_, and then
you'd have exactly one method you'd ever call in that class (re:
StartScheduledUpload()).

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_reporting_scheduler.cc:114: if (upload_interval_
> max_interval) {
You may as well be defensive and also check:
|| upload_interval_ < 0

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_reporting_scheduler.h (right):

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_reporting_scheduler.h:18: virtual
~MetricsReportingScheduler();
Why the virtual destructor?  There is no base, and I didn't see tests that
derive from this class.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:902:
waiting_for_asynchronus_reporting_step_ = true;
If this is part of the timer scheduling, it should be located exclusively in the
TimerScheduling instance.

Note how the old code is more readable in lines 937-941.  We check... bail... or
set the value.

If this value is checked remotely (ilke line 937) we should be able to at least
DCHECK_FALSE(waiting_for_asynchronous_reporting_step_) before we set the value.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:933:
waiting_for_asynchronus_reporting_step_ = false;
If you move this line down to line 944, then you wouldn't need to have line 941
to set it back to true.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:951: scheduler_->Stop();
Stop() appears to be very permanent.  This seems like it will shut off all UMA
uploads as soon as we've been idle since last transmission.  What will then
cause transmission to re Start()?

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.h:135: friend class
MetricsReportingScheduler;
Making classes friends of each other, outside of the context of test classes
playing ith non-public access, is usually a bad direction.

Better is to work to surface *only* the API you want called from the would-be
friend. For example, exporting to said class a method it can call.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.h:402: // Indicates that an asynchronous
reporting step is running.
If this is only for debugging, you should probably indicate it in the comment. 
I think you didn't every branch on this value (from what I could spot).

[stuartmorgan, 2011-04-18 17:03:00]

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_reporting_scheduler.cc (right):

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_reporting_scheduler.cc:43:
MetricsReportingScheduler::~MetricsReportingScheduler() {}
On 2011/04/16 02:03:13, jar wrote:
> Why bother with a destructor?  ...or at best, list it in the header explicitly
> (so that reader doesn't have to wonder what it does).

My understanding of
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
is that this is the preferred style in our code (in fact, this class may reach
the score threshold where doing anything else would be a hard error under
clang).

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_reporting_scheduler.cc:94:
!metrics_service_->recording_active())
On 2011/04/16 02:03:13, jar wrote:
> I thought you reduced these two items into a single state variable in a prior
> CL.  Am I confused?

The prior CL eliminate a variable that was essentially a second copy of
reporting_active. MetricsService still tracks recording and reporting separately
(mostly for unit tests, it seems).

> In addition, you could leave this state checking to metrics_service_, and then
> you'd have exactly one method you'd ever call in that class (re:
> StartScheduledUpload()).

Done.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_reporting_scheduler.cc:114: if (upload_interval_
> max_interval) {
On 2011/04/16 02:03:13, jar wrote:
> You may as well be defensive and also check:
> || upload_interval_ < 0

Done.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_reporting_scheduler.h (right):

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_reporting_scheduler.h:18: virtual
~MetricsReportingScheduler();
On 2011/04/16 02:03:13, jar wrote:
> Why the virtual destructor?  There is no base, and I didn't see tests that
> derive from this class.

Just habit; removed.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:902:
waiting_for_asynchronus_reporting_step_ = true;
On 2011/04/16 02:03:13, jar wrote:
> If this is part of the timer scheduling, it should be located exclusively in
the
> TimerScheduling instance.

It's not, it's for debugging (see my response to your comment in the header).

> Note how the old code is more readable in lines 937-941.  We check... bail...
or
> set the value.

This method is replacing LogTransmissionTimerDone. 937-941 is part of a method
that doesn't exist any more, the diff algorithm just got confused by the blank
lines they share.

> If this value is checked remotely (ilke line 937) we should be able to at
least
> DCHECK_FALSE(waiting_for_asynchronous_reporting_step_) before we set the
value.

Done.

(FWIW, I'd be fine with eliminating the variable, since DCHECKING a bool just to
make sure nobody accidentally calls an internal method seems like overkill to
me. I was just trying to keep as much of the old behavior as possible when it
wasn't directly related to the refactoring.)

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:933:
waiting_for_asynchronus_reporting_step_ = false;
On 2011/04/16 02:03:13, jar wrote:
> If you move this line down to line 944, then you wouldn't need to have line
941
> to set it back to true.

Done.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:951: scheduler_->Stop();
On 2011/04/16 02:03:13, jar wrote:
> Stop() appears to be very permanent.  This seems like it will shut off all UMA
> uploads as soon as we've been idle since last transmission.  What will then
> cause transmission to re Start()?

The observer calls Start() (just as it used to call StartLogTransmissionTimer).
I haven't actually changed the behavior here, I've just made it more explicit.
In the old code early returns sometimes called StartLogTransmissionTimer, and
sometimes just returned; the latter had the effect of stopping the timer cycle,
but it was much less obvious. I've made it so that the behavior is clear to a
reader (and also much harder for someone to do accidentally), without actually
changing the effect.

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.h:135: friend class
MetricsReportingScheduler;
On 2011/04/16 02:03:13, jar wrote:
> Making classes friends of each other, outside of the context of test classes
> playing ith non-public access, is usually a bad direction.
> 
> Better is to work to surface *only* the API you want called from the would-be
> friend. For example, exporting to said class a method it can call.

True; do you have any pointers to how we normally handle that pattern in
Chromium? E.g., do we use RunnableMethod objects for this kind of thing, or are
those only intended for message-loop usage?

http://codereview.chromium.org/6869034/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.h:402: // Indicates that an asynchronous
reporting step is running.
On 2011/04/16 02:03:13, jar wrote:
> If this is only for debugging, you should probably indicate it in the comment.

> I think you didn't every branch on this value (from what I could spot).

Done. (This used to be a variable that sometimes was used for branching and
sometimes was used for DCHECKs, and only the latter remained in this class after
refactoring.)

[jar (doing other things), 2011-04-18 22:06:27]

LGTM

Please (assuming I understand) at least do the minimal code-motion of moving up
lines around 937 to be ahead of line 930 (unless I'm confused).

Thanks,

Jim

http://codereview.chromium.org/6869034/diff/7001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6869034/diff/7001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:935: for (RenderProcessHost::iterator
i(RenderProcessHost::AllHostsIterator());
I don't know when/where this iteration line came from.... but it sure looks like
it is sending messages to all renderers to ask them to push their stats into
histograms.  It would seem that this should (if I'm groking the intent) be done
before line 930, where we ask each renderer to send all their current histograms
to the browser. <sigh>

In fact, I suspect we should push line 937 into the
FetchRendererHistogtramsAsynchronously method, since *it* does an iteration over
the renderers.  Going one step further... we could probably make this part of
the renderer side activity <bigger sigh>.  Perchance this is more that you
wanted to play with... but this CL is making such errors more visible.

[stuartmorgan, 2011-04-19 16:14:19]

http://codereview.chromium.org/6869034/diff/7001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6869034/diff/7001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:935: for (RenderProcessHost::iterator
i(RenderProcessHost::AllHostsIterator());
On 2011/04/18 22:06:27, jar wrote:
> I don't know when/where this iteration line came from.... but it sure looks
like
> it is sending messages to all renderers to ask them to push their stats into
> histograms.  It would seem that this should (if I'm groking the intent) be
done
> before line 930, where we ask each renderer to send all their current
histograms
> to the browser. <sigh>

+sail, who made this change recently in bug 43207. The checkin has no review
link, so I have no idea why the ordering was reversed here. Was this a mistake,
or intentional?

[sail, 2011-04-19 16:26:23]

http://codereview.chromium.org/6869034/diff/7001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/6869034/diff/7001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:935: for (RenderProcessHost::iterator
i(RenderProcessHost::AllHostsIterator());
On 2011/04/19 16:14:19, stuartmorgan wrote:
> On 2011/04/18 22:06:27, jar wrote:
> > I don't know when/where this iteration line came from.... but it sure looks
> like
> > it is sending messages to all renderers to ask them to push their stats into
> > histograms.  It would seem that this should (if I'm groking the intent) be
> done
> > before line 930, where we ask each renderer to send all their current
> histograms
> > to the browser. <sigh>
> 
> +sail, who made this change recently in bug 43207. The checkin has no review
> link, so I have no idea why the ordering was reversed here. Was this a
mistake,
> or intentional?

Sorry, it looks like a mistake. Here's the review:
http://codereview.chromium.org/6621052/

[stuartmorgan, 2011-04-19 16:38:56]

On 2011/04/19 16:26:23, sail wrote:
> Sorry, it looks like a mistake. Here's the review:
> http://codereview.chromium.org/6621052/

jar, can you take a look at that review and see if it makes sense? I don't think
thakis or mento is familiar with MetricsService, so it doesn't look like it got
sufficient review. It's unclear to me if there were side-effects of the
memory-detail-gathering that were lost by removing the call.

We should definitely flip the ordering, but I want to make sure that's
sufficient to fix the mistake.

[stuartmorgan, 2011-04-19 17:23:47]

I'll hold off on landing this until the revert of that CL is in so that it can
be reverted cleanly, then update this onto the new CL and send it out for
another review.

[jar (doing other things), 2011-04-19 17:53:38]

I spoke to sail@, and he'll do the revert.  Please do wait until the revert
is done before (merge/updating) and then landing.

Thanks for checking on this!!

Jim

On Tue, Apr 19, 2011 at 9:38 AM, <stuartmorgan@chromium.org> wrote:

> On 2011/04/19 16:26:23, sail wrote:
>
>> Sorry, it looks like a mistake. Here's the review:
>> http://codereview.chromium.org/6621052/
>>
>
> jar, can you take a look at that review and see if it makes sense? I don't
> think
> thakis or mento is familiar with MetricsService, so it doesn't look like it
> got
> sufficient review. It's unclear to me if there were side-effects of the
> memory-detail-gathering that were lost by removing the call.
>
> We should definitely flip the ordering, but I want to make sure that's
> sufficient to fix the mistake.
>
>
> http://codereview.chromium.org/6869034/
>

[stuartmorgan, 2011-04-19 20:10:10]

Rebased onto the revert. Please take a look at that part of the code and the new
callback. Thanks for all the reviews!

[jar (doing other things), 2011-04-19 22:37:39]

LGTM

To test, you should build with a change in browser_main.cc to ensure that
metrics in enabled (it is turned off by default in all dev builds).  Metrics
will (as you probably recall from this CL) do a first upload in 60 seconds, and
you should see a lot of these paths exercised. 

To get to see the ongoing logic work, you can set the delay (temporarily) lower
in your build, and see that additional uploads take place as expected.

[stuartmorgan, 2011-04-19 22:45:46]

On 2011/04/19 22:37:39, jar wrote:
> To test, you should build with a change in browser_main.cc to ensure that
> metrics in enabled (it is turned off by default in all dev builds).  Metrics
> will (as you probably recall from this CL) do a first upload in 60 seconds,
and
> you should see a lot of these paths exercised. 
> 
> To get to see the ongoing logic work, you can set the delay (temporarily)
lower
> in your build, and see that additional uploads take place as expected.

Yep, way ahead of you on all counts :)