Make VariationsService simulate received seeds.

Make VariationsService simulate received seeds.

Uses VariationsSeedSimulator to simulate incoming
variation seeds and log the results of simulation
in histograms, as well as the amount of time taken.

Makes VariationsService take a MetricsStateManager
parameter for creating the entropy provider.

BUG=281474, 315807
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269899

[Ilya Sherman, 2014-04-25 06:27:45]

Sorry, didn't quite get to this today; will try to review by EOD tomorrow.

[jwd, 2014-04-25 14:30:33]

https://codereview.chromium.org/238443008/diff/70001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/238443008/diff/70001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:166: VariationsSeed*
seed) {
Should you check that seed isn't null?

https://codereview.chromium.org/238443008/diff/70001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:173: if
(!seed->ParseFromString(seed_data)) {
What's in seed when it fails to parse? Should it be cleared before returning? I
don't feel strongly that it should.

[Alexei Svitkine (slow), 2014-04-25 15:21:45]

https://codereview.chromium.org/238443008/diff/70001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/238443008/diff/70001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:166: VariationsSeed*
seed) {
Changed logic to allow passing NULL and only filling the out param on success,
and updated function declaration comment.

https://codereview.chromium.org/238443008/diff/70001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_seed_store.cc:173: if
(!seed->ParseFromString(seed_data)) {
On 2014/04/25 14:30:34, Jesse Doherty wrote:
> What's in seed when it fails to parse? Should it be cleared before returning?
I
> don't feel strongly that it should.

Ditto.

[jwd, 2014-04-25 15:29:22]

https://codereview.chromium.org/238443008/diff/110001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/238443008/diff/110001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:38: // with the
de-serialized protobuf decoded from |seed_data|.
Can you add a sanity test that |seed_data| has the correct data on success.

[Alexei Svitkine (slow), 2014-04-25 15:43:36]

https://codereview.chromium.org/238443008/diff/110001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_seed_store.h (right):

https://codereview.chromium.org/238443008/diff/110001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_seed_store.h:38: // with the
de-serialized protobuf decoded from |seed_data|.
On 2014/04/25 15:29:23, Jesse Doherty wrote:
> Can you add a sanity test that |seed_data| has the correct data on success.

Done.

[jwd, 2014-04-25 15:50:53]

lgtm

[Ilya Sherman, 2014-04-25 18:40:53]

It looks like you only need to expose the entropy provider so that you can
eventually pass an entropy value back into
FieldTrial::CreateSimulatedFieldTrial().  WDYT of instead updating the
CreateSimulatedFieldTrial() API to calculate an entropy value internally?

[Alexei Svitkine (slow), 2014-04-28 18:28:21]

On 2014/04/25 18:40:53, Ilya Sherman wrote:
> It looks like you only need to expose the entropy provider so that you can
> eventually pass an entropy value back into
> FieldTrial::CreateSimulatedFieldTrial().  WDYT of instead updating the
> CreateSimulatedFieldTrial() API to calculate an entropy value internally?

Thinking about this some more... Right now the CreateSimulatedFieldTrial() API
is on FieldTrial and not FieldTrialList so that it can be used without a global
FieldTrialList.

There's two benefits from that:
  1. Being able to use it where there isn't a FieldTrialList - e.g. from
chrome_exe code before chrome_dll is loaded. For example, had that API been
available at the time the PreRead experiment was done, we wouldn't need all this
complicated code that does its own group selection in chrome_exe:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/client_...
  2. The flexibility of passing in a different entropy provider than what's used
by the current process.

In fact, 2. is beneficial even for this current CL. The reason is that if the
user had opted out of UMA while Chrome is running, the current CL would simulate
what would happen incorrectly for that user (it will give a more conservative
answer on number of changes than the real one which will be a large percentage
since all trials will be re-rolled).

So actually, to make the simulation more correct in that case, I think I'll
change this CL to instead use CreateEntropyProvider() from MetricsService to
create a new entropy provider based on the current UMA prefs. It will add a
slightly unfortunate dependency from VariationsService onto MetricsService which
previously didn't exist, but hopefully we can live with that (and maybe refactor
things differently in the future).

[Ilya Sherman, 2014-04-28 19:40:47]

On 2014/04/28 18:28:21, Alexei Svitkine wrote:
> So actually, to make the simulation more correct in that case, I think I'll
> change this CL to instead use CreateEntropyProvider() from MetricsService to
> create a new entropy provider based on the current UMA prefs. It will add a
> slightly unfortunate dependency from VariationsService onto MetricsService
which
> previously didn't exist, but hopefully we can live with that (and maybe
refactor
> things differently in the future).

Sounds good.  How hard would it be to factor out CreateEntropyProvider() to some
shared location as part of this CL, so that we avoid pushing technical debt into
the future?  Actually, what's the intended dependency structure between the
variations component and the metrics component (were these parts of the codebase
to be fully componentized)?

[Alexei Svitkine (slow), 2014-04-28 20:16:58]

It's an interesting question and I've been thinking about it too.

One challenge is that CreateEntropyProvider() uses values from prefs that
are currently managed by the MetricsService - mainly client_id and
low_entropy_source. We still need to be able to get at those values even if
that function doesn't live in MetricsService. (And just because we can
access them through the prefs API directly doesn't mean we should - since
that's bad design).

I think one thing we can do is to create a helper class to manage client id
/ low entropy source. Maybe MetricsIdsManager? (open to suggestions for
better names)

Then, both MetricsService and VariationsService can use it - this will
definitely simply MetricsService code but doesn't quite solve the
dependency problem. Someone still has to own this new object (unless we
decide that it's safe to create multiple instances of it ...). One
possibility is to have MetricsService still own this but to have the
chrome_browser_main.cc init VariationsService with a pointer to that
sub-object when creating it. If we do that, probably we'd have to make it a
RefPtr since a weak pointer would add a dependency between the lifetime of
MetricsService and VariationsService. :\

The other challenge is currently CreateEntropyProvider() takes the UMA
optin state as a param. Which comes from IsMetricsReportingEnabled() in
chrome_browser_main.cc. At the same time, metrics_service.cc has a
different MetricsServiceHelper::IsMetricsReportingEnabled() function with a
different implementation. :\ So this whole area is kind of a mess.

One thing we can do is make this new helper class (MetricsIdsManager) to
also encapsulate the Metrics opt-in state, which would make the APIs simple
(and we can consolidate the logic). Perhaps the class should be called
MetricsStateManager then? Though that's a bit of a confusing name due to
"State" also referring to the MetricsService state machine.

In terms of how that will work in a world with components, I'm thinking
variations_service component would depend on metrics_base component and the
new class being in metrics_base (whatever shapes those end up being - i.e.
metrics_base might just be 'metrics' and the metrics service would be in
'metrics_service' component or 'metrics_base' would actually be
'metrics/base').

WDYT about this general approach?


On Mon, Apr 28, 2014 at 3:40 PM, <isherman@chromium.org> wrote:

> On 2014/04/28 18:28:21, Alexei Svitkine wrote:
>
>> So actually, to make the simulation more correct in that case, I think
>> I'll
>> change this CL to instead use CreateEntropyProvider() from MetricsService
>> to
>> create a new entropy provider based on the current UMA prefs. It will add
>> a
>> slightly unfortunate dependency from VariationsService onto MetricsService
>>
> which
>
>> previously didn't exist, but hopefully we can live with that (and maybe
>>
> refactor
>
>> things differently in the future).
>>
>
> Sounds good.  How hard would it be to factor out CreateEntropyProvider()
> to some
> shared location as part of this CL, so that we avoid pushing technical
> debt into
> the future?  Actually, what's the intended dependency structure between the
> variations component and the metrics component (were these parts of the
> codebase
> to be fully componentized)?
>
> https://codereview.chromium.org/238443008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-04-28 20:57:08]

On 2014/04/28 20:16:58, Alexei Svitkine wrote:
> WDYT about this general approach?

Generally sounds good.  IMO it probably makes sense for the same object that
owns the MetricsService and VariationsService instances to also own the new
object.

[Alexei Svitkine (slow), 2014-04-28 21:01:25]

Good point. Maybe we should have a MetricsServicesManager which will own
VariationsService, MetricsService and RapporService.

Anyway, I'll start with refactoring the above into a class initially just
fully owned by MetricsService (in a separate CL). Then I'll change this CL
to make VariationsService be able to access it (either by creating
MetricsServicesManager or by just putting the MetricsStateManager on
BrowserProcess) and use it.


On Mon, Apr 28, 2014 at 4:57 PM, <isherman@chromium.org> wrote:

> On 2014/04/28 20:16:58, Alexei Svitkine wrote:
>
>> WDYT about this general approach?
>>
>
> Generally sounds good.  IMO it probably makes sense for the same object
> that
> owns the MetricsService and VariationsService instances to also own the new
> object.
>
> https://codereview.chromium.org/238443008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2014-05-09 15:24:29]

Reviewers, please have another look.

I've now rebased this on my latest changes and make VariationsService take a
MetricsStateManager param which is used for creating the entropy provider used
for simulation.

[Ilya Sherman, 2014-05-09 23:50:28]

LGTM, thanks.  Note: I only skimmed the variations changes.

https://codereview.chromium.org/238443008/diff/140001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/238443008/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:415: // manager may be
NULL for some unit tests.
Up to you, but I'd recommend passing a simple stub or mock object rather than
null, so that your production code doesn't need to worry about test-only cases.

[jwd, 2014-05-12 14:57:31]

lgtm

[Alexei Svitkine (slow), 2014-05-12 15:00:43]

https://codereview.chromium.org/238443008/diff/140001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/238443008/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:415: // manager may be
NULL for some unit tests.
On 2014/05/09 23:50:29, Ilya Sherman wrote:
> Up to you, but I'd recommend passing a simple stub or mock object rather than
> null, so that your production code doesn't need to worry about test-only
cases.

I'll keep it like this for now since otherwise a bit more refactoring / code is
needed compared to this which keeps things simpler. Can re-visit this in the
future.

[Alexei Svitkine (slow), 2014-05-12 15:00:48]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-12 15:00:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/238443008/140001

[Alexei Svitkine (slow), 2014-05-12 15:01:12]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-12 15:09:31]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-12 15:09:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/238443008/180001

[commit-bot: I haz the power, 2014-05-12 18:20:04]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-12 19:31:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-12 19:31:28]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Alexei Svitkine (slow), 2014-05-12 19:40:10]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-12 19:41:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/238443008/180001

[commit-bot: I haz the power, 2014-05-12 21:04:42]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-12 22:49:42]

Change committed as 269899