Inform GetEntropySource of whether or not metrics reporting will be enabled.

Inform GetEntropySource of whether or not metrics reporting will be enabled.

This resolves an issue where we were calling GetEntropySource too early before MetricsService::Start was called. Instead of checking the reporting_active member in MetricsService, we instead just check if the criteria for metrics reporting is allowed or not, ahead of time.

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

[SteveT, 2012-08-09 15:01:18]

Hey Ilya, mind taking a look?

If you can think of a cleaner way of handling this that doesn't involve adding
the bool param to GetEntropySource, let me know. I find it sort of weird that
we're telling MetricsService that we are going to "eventually" allow metrics
reporting. Normally we do this by calling Start on it.

[Ilya Sherman, 2012-08-09 16:47:29]

Can you also add a test to make sure this doesn't regress?

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:616: }
Another option is to move this method into the MetricsService implementation. 
That might be cleaner in the long term, but is probably harder to merge...

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:541: std::string
MetricsService::GetEntropySource(bool reporting_will_be_enabled) {
Optional nit: Perhaps add a comment mentioning why we can't just use
reporting_active() in this method.

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.h (right):

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.h:91: // The caller must determine if
metrics reorting will be enabled for this
nit: "reorting" -> "reporting"

[SteveT, 2012-08-09 17:18:30]

Addressed your comments except for the test...

As for testing, I am not sure if it is straight forward to test the
chrome_browser_main part (which is actually affected by this bug). I could test
that GetEntropySource now returns something different for different
|reporting_will_be_enabled| values... but I'm not sure if that is sufficient.

Thoughts?

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:616: }
I'd rather go for a reasonable and simpler fix that is easier to merge, and do
some refactoring afterwards (hopefully without jigging the EntropySource again).

The way MetricsService is currently set up, it is always told by the caller if
metrics reporting is enabled or not. This seems like an important design
decision made a while ago, so I'm hesitant to change this in a fix patch.

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:541: std::string
MetricsService::GetEntropySource(bool reporting_will_be_enabled) {
Good call. I did this in the header.

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.h (right):

https://chromiumcodereview.appspot.com/10830241/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.h:91: // The caller must determine if
metrics reorting will be enabled for this
On 2012/08/09 16:47:29, Ilya Sherman wrote:
> nit: "reorting" -> "reporting"

Done.

[Ilya Sherman, 2012-08-09 17:24:29]

On 2012/08/09 17:18:30, SteveT wrote:
> Addressed your comments except for the test...
> 
> As for testing, I am not sure if it is straight forward to test the
> chrome_browser_main part (which is actually affected by this bug). I could
test
> that GetEntropySource now returns something different for different
> |reporting_will_be_enabled| values... but I'm not sure if that is sufficient.
> 
> Thoughts?

I was thinking we should be able to write this as a browser test, which should
bring up the full browser and go through the full init sequence.  Does that seem
reasonable?

[SteveT, 2012-08-09 17:26:41]

Do browser_tests run the browser as a Chromium instance, or an official Chrome
instance?

The issue here is that some of the code (such as checking if UMA is enabled) is
not compiled if we're on an unofficial build.

Do you know of any browser_tests that rely on UMA-enabled state? (or simulate it
in some way?)

[Ilya Sherman, 2012-08-09 17:50:43]

On 2012/08/09 17:26:41, SteveT wrote:
> Do browser_tests run the browser as a Chromium instance, or an official Chrome
> instance?
> 
> The issue here is that some of the code (such as checking if UMA is enabled)
is
> not compiled if we're on an unofficial build.
> 
> Do you know of any browser_tests that rely on UMA-enabled state? (or simulate
it
> in some way?)

browser_tests usually run as a Chromium instance, yes.  However, I'm pretty sure
that browser_tests can set commandline flags to affect the run, and so we should
be able to tickle the right code path.  Maybe Jim can provide some more color as
to whether something like this has been tried in the past...

[SteveT, 2012-08-09 17:55:30]

Not sure how far the command line flags will take us in this case - the build
will completely remove the code that checks for the UMA-Enabled bit if we are
built as Chromium. And I don't think there is a command line flag that can force
this bit on.

I'll ping Jim for pointers.

[jar (doing other things), 2012-08-09 18:03:09]

Metrics is always compiled in... we just don't turn it on from browser_main.cc
unles we're in an OFFICIAL build.  

IMO, you should add a command line switch to force it on (in any build) and then
construct a browser test with that command line switch.


On 2012/08/09 17:50:43, Ilya Sherman wrote:
> On 2012/08/09 17:26:41, SteveT wrote:
> > Do browser_tests run the browser as a Chromium instance, or an official
Chrome
> > instance?
> > 
> > The issue here is that some of the code (such as checking if UMA is enabled)
> is
> > not compiled if we're on an unofficial build.
> > 
> > Do you know of any browser_tests that rely on UMA-enabled state? (or
simulate
> it
> > in some way?)
> 
> browser_tests usually run as a Chromium instance, yes.  However, I'm pretty
sure
> that browser_tests can set commandline flags to affect the run, and so we
should
> be able to tickle the right code path.  Maybe Jim can provide some more color
as
> to whether something like this has been tried in the past...

[SteveT, 2012-08-09 18:16:51]

Taking Jim's advice and adding a new command line flag to force-enable metrics
reporting. As discussed, this would be useful for testing in general.

[SteveT, 2012-08-09 20:24:13]

Back to you. Let me know what you think.

[Ilya Sherman, 2012-08-09 20:41:18]

LGTM --thanks for adding test coverage! :)

In an ideal world, I think it would be a little bit better to have some sort of
statistical test (that passes with 99.9999999% likelihood), rather than adding
code specifically for testing.  My worry is that the code that manipulates the
state that the test relies on could drift from the code that actually sets the
entropy source...

On the other hand, that sounds like it might be tricky, and this test is already
miles better than no test :)

https://chromiumcodereview.appspot.com/10830241/diff/2004/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.h (right):

https://chromiumcodereview.appspot.com/10830241/diff/2004/chrome/browser/metr...
chrome/browser/metrics/metrics_service.h:81: };
nit: Can we make this (and the corresponding getter function) private, and
friend the tests?

https://chromiumcodereview.appspot.com/10830241/diff/2004/chrome/browser/metr...
File chrome/browser/metrics/metrics_service_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10830241/diff/2004/chrome/browser/metr...
chrome/browser/metrics/metrics_service_browsertest.cc:64:
command_line->AppendSwitch(switches::kEnableMetricsReporting);
Before committing this, can you verify that we're not going to make any network
requests from test code as a result of this flag?  If we currently are, we
should make sure to also change the upload URL for the metrics server to
something bogus, like localhost.

[SteveT, 2012-08-09 21:07:59]

Agreed that this is not the optimal test for this behaviour, but yeah, it at
least guarantees that we hit one of the paths at some point during startup.

If you like, I can log a bug against myself to revisit this (and also look at
the refactoring you discussed in your earlier comment set).

I'll commit this once I get through the manual tests (including network
testing).

https://chromiumcodereview.appspot.com/10830241/diff/2004/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.h (right):

https://chromiumcodereview.appspot.com/10830241/diff/2004/chrome/browser/metr...
chrome/browser/metrics/metrics_service.h:81: };
Sure can! Done.

https://chromiumcodereview.appspot.com/10830241/diff/2004/chrome/browser/metr...
File chrome/browser/metrics/metrics_service_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10830241/diff/2004/chrome/browser/metr...
chrome/browser/metrics/metrics_service_browsertest.cc:64:
command_line->AppendSwitch(switches::kEnableMetricsReporting);
Sure, I can take a look at that. I thought that there was something already in
place that prevented this... but I guess not.

[SteveT, 2012-08-09 21:46:56]

Okay, manual tests look good for network accesses (as in there are none). Gotta
love tcpdump | grep :)

[commit-bot: I haz the power, 2012-08-09 21:51:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10830241/7005

[commit-bot: I haz the power, 2012-08-09 21:51:19]

Presubmit check for 10830241-7005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.1s to calculate.

[SteveT, 2012-08-09 21:53:35]

Hey Nico - looks like I need OWNERS approval for chrome_browser_main.cc.

[Nico, 2012-08-09 21:57:52]

lgtm

https://chromiumcodereview.appspot.com/10830241/diff/7005/chrome/common/chrom...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10830241/diff/7005/chrome/common/chrom...
chrome/common/chrome_switches.cc:579: const char kEnableMetricsReporting[]      
 = "enable-metrics-reporting";
kEnableMetricsReportForTesting / enable-metrics-report-for-testing ?

[SteveT, 2012-08-09 22:05:02]

Thanks for the quick look, Nico! Going to CQ this bad boy.

https://chromiumcodereview.appspot.com/10830241/diff/7005/chrome/common/chrom...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10830241/diff/7005/chrome/common/chrom...
chrome/common/chrome_switches.cc:579: const char kEnableMetricsReporting[]      
 = "enable-metrics-reporting";
On 2012/08/09 21:57:52, Nico wrote:
> kEnableMetricsReportForTesting / enable-metrics-report-for-testing ?

Done.

[commit-bot: I haz the power, 2012-08-09 22:05:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10830241/13010

[commit-bot: I haz the power, 2012-08-09 23:07:41]

Change committed as 150934

[Mark P, 2012-08-10 18:03:18]

I know this is a little late, but wouldn't it better to have
entropy_source_returned_ keep a copy of the *first* entropy source returned, not
the last?  That way you'd notice problems if, for instance, IsMetricsRecording
in chrome_browser_main.cc starts returning false before startup is complete, and
thus the first field trials created have the low entropy source but the latter
ones have the high entropy source.  The current way you test will hide this
situation because the last entropy source return is likely to be the high one
(because a field trial might've been created late in startup that sees the
correct value for IsMetricsReporting).

--mark

[SteveT, 2012-08-10 19:53:45]

On 2012/08/10 18:03:18, Mark P wrote:
> I know this is a little late, but wouldn't it better to have
> entropy_source_returned_ keep a copy of the *first* entropy source returned,
not
> the last?  That way you'd notice problems if, for instance, IsMetricsRecording
> in chrome_browser_main.cc starts returning false before startup is complete,
and
> thus the first field trials created have the low entropy source but the latter
> ones have the high entropy source.  The current way you test will hide this
> situation because the last entropy source return is likely to be the high one
> (because a field trial might've been created late in startup that sees the
> correct value for IsMetricsReporting).
> 
> --mark

Good suggestion. I can make this change in a new CL.

[Mark P, 2012-08-16 19:00:21]

On 2012/08/10 19:53:45, SteveT wrote:
> On 2012/08/10 18:03:18, Mark P wrote:
> > I know this is a little late, but wouldn't it better to have
> > entropy_source_returned_ keep a copy of the *first* entropy source returned,
> not
> > the last?  That way you'd notice problems if, for instance,
IsMetricsRecording
> > in chrome_browser_main.cc starts returning false before startup is complete,
> and
> > thus the first field trials created have the low entropy source but the
latter
> > ones have the high entropy source.  The current way you test will hide this
> > situation because the last entropy source return is likely to be the high
one
> > (because a field trial might've been created late in startup that sees the
> > correct value for IsMetricsReporting).
> > 
> > --mark
> 
> Good suggestion. I can make this change in a new CL.

I haven't seen this new CL.

Do I need to file a bug to remind you to do it?

thanks,
mark

[SteveT, 2012-08-16 19:03:07]

On 2012/08/16 19:00:21, Mark P wrote:
> On 2012/08/10 19:53:45, SteveT wrote:
> > On 2012/08/10 18:03:18, Mark P wrote:
> > > I know this is a little late, but wouldn't it better to have
> > > entropy_source_returned_ keep a copy of the *first* entropy source
returned,
> > not
> > > the last?  That way you'd notice problems if, for instance,
> IsMetricsRecording
> > > in chrome_browser_main.cc starts returning false before startup is
complete,
> > and
> > > thus the first field trials created have the low entropy source but the
> latter
> > > ones have the high entropy source.  The current way you test will hide
this
> > > situation because the last entropy source return is likely to be the high
> one
> > > (because a field trial might've been created late in startup that sees the
> > > correct value for IsMetricsReporting).
> > > 
> > > --mark
> > 
> > Good suggestion. I can make this change in a new CL.
> 
> I haven't seen this new CL.
> 
> Do I need to file a bug to remind you to do it?
> 
> thanks,
> mark

Yes please - haven't gotten around to this change yet. Please file a bug against
me.