Add histograms to track synced pref changes.

Add histograms to track synced pref changes.

Certain prefs will now elicit stat tracking
whenever they are changed locally and pushed to
sync, or when changes from sync server are pulled
down to update the local pref store.

New histogram names take the form:

Settings.$prefname.Changed{FromSync,Local}

The logged value is the new pref value imposed by
the change.

BUG=265627
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216813

[Ken Rockot(use gerrit already), 2013-08-01 17:05:21]

PTAL

[bbudge, 2013-08-01 17:48:08]

LGTM for PrefMetricsService part, and histograms.xml.

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.cc:199: from_sync ? ".PulledFromSync"
: "PushedToSync");
Why is one prefixed with '.' and not the other?

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.h:77: // Callback to for a boolean
pref change histogram.
s/to for/for?

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.h:82: void LogIntegerPrefChange(int
boundary_value,
ditto

[Ken Rockot(use gerrit already), 2013-08-01 17:55:14]

Thanks

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.cc:199: from_sync ? ".PulledFromSync"
: "PushedToSync");
On 2013/08/01 17:48:08, bbudge1 wrote:
> Why is one prefixed with '.' and not the other?

Good catch. Fixed.

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.h:77: // Callback to for a boolean
pref change histogram.
On 2013/08/01 17:48:08, bbudge1 wrote:
> s/to for/for?

Done.

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.h:82: void LogIntegerPrefChange(int
boundary_value,
On 2013/08/01 17:48:08, bbudge1 wrote:
> ditto

Done.

https://codereview.chromium.org/21580002/diff/7001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_metrics_service.h:82: void LogIntegerPrefChange(int
boundary_value,
On 2013/08/01 17:48:08, bbudge1 wrote:
> ditto

Done.

[Alexei Svitkine (slow), 2013-08-01 18:02:51]

histograms.xml lgtm

[meacer, 2013-08-01 20:48:03]

LGTM

[Ken Rockot(use gerrit already), 2013-08-05 15:35:42]

ping battre@

[battre, 2013-08-05 17:17:26]

Sorry, I did not get through it today.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:403:
NotifySyncedPrefObservers(name, true);
nit: true /*from_sync*/

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:493:
NotifySyncedPrefObservers(name, false);
nit: true /*from_false*/

[battre, 2013-08-06 15:54:38]

I think I have found two bugs. How about some unit tests?

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:123: startupPrefValueMax));
nit: inline startupPrefValueMax?

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:133: const std::string&
histogram_base,
nit: histogram_name_prefix?

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:179: int domain_id) {
nit: align parameters.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:201: callback.Run(histogram_name,
pref->GetValue());
This may be a bug. pref->GetValue() does not necessary return the value we got
from sync. It may return the value that is configured by a policy or extension.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:246: }
nit: no {}

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:51: ValueHistogramCallback;
nit: LogHistogramValueCallback?

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:493:
NotifySyncedPrefObservers(name, false);
I think this is a bug: You notify the observers her before executing the change.
But the observers query the current value and will get the old one.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:523: bool from_sync) {
nit: align parameters.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.h (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.h:8: #include <list>
nit: not used.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.h:180: void
NotifySyncedPrefObservers(const std::string& path, bool from_sync);
can you make this function const?

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:151: SyncedPrefObserver* observer)
{
nit: align parameters, also below

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
File chrome/browser/prefs/synced_pref_change_registrar.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar.cc:13: const std::string&
path, bool from_sync) {
nit: each parameter into a new line.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar.cc:28: void
SyncedPrefChangeRegistrar::Add(const char *path,
nit: align parameters

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar.cc:47: iter !=
observers_.end(); ++iter) {
nit: +1 space indent

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar.cc:57: void
SyncedPrefChangeRegistrar::OnSyncedPrefChanged(const std::string& path,
nit: align parameters.

[Ken Rockot(use gerrit already), 2013-08-06 16:27:46]

Thanks. Just some questions about the potential bugs for now

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:201: callback.Run(histogram_name,
pref->GetValue());
On 2013/08/06 15:54:39, battre wrote:
> This may be a bug. pref->GetValue() does not necessary return the value we got
> from sync. It may return the value that is configured by a policy or
extension.

I think it's correct as-is, but can you elaborate? A value changed by policy
should never invoke this callback, because
PrefModelAssociator::ProcessPrefChange is cut off before notifying observers in
the case where the pref !IsUserModifiable(). I'm not sure about values
configured by extension - is PrefModelAssociator aware of the distinction?

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:493:
NotifySyncedPrefObservers(name, false);
On 2013/08/06 15:54:39, battre wrote:
> I think this is a bug: You notify the observers her before executing the
change.
> But the observers query the current value and will get the old one.

The pref has its new value at this point. This method is invoked as a result of
a local (or managed) pref change. We're notifying observers before the change is
pushed up to sync.

Line 403 notifies observers of a change being pulled down from sync, and it
occurs after the local value is updated as well.

[Ken Rockot(use gerrit already), 2013-08-06 20:18:32]

Going to do some additional verification regarding the possible bugs you've
highlighted. In the meantime, addressed all the nits.

Also removed any domain-related histograms or code, since that stuff was based
on a reverted change.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:123: startupPrefValueMax));
On 2013/08/06 15:54:39, battre wrote:
> nit: inline startupPrefValueMax?

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:133: const std::string&
histogram_base,
On 2013/08/06 15:54:39, battre wrote:
> nit: histogram_name_prefix?

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:246: }
On 2013/08/06 15:54:39, battre wrote:
> nit: no {}

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:51: ValueHistogramCallback;
On 2013/08/06 15:54:39, battre wrote:
> nit: LogHistogramValueCallback?

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:403:
NotifySyncedPrefObservers(name, true);
On 2013/08/05 17:17:26, battre wrote:
> nit: true /*from_sync*/

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:493:
NotifySyncedPrefObservers(name, false);
On 2013/08/05 17:17:26, battre wrote:
> nit: true /*from_false*/

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:523: bool from_sync) {
On 2013/08/06 15:54:39, battre wrote:
> nit: align parameters.

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.h (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.h:8: #include <list>
On 2013/08/06 15:54:39, battre wrote:
> nit: not used.

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.h:180: void
NotifySyncedPrefObservers(const std::string& path, bool from_sync);
On 2013/08/06 15:54:39, battre wrote:
> can you make this function const?

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:151: SyncedPrefObserver* observer)
{
On 2013/08/06 15:54:39, battre wrote:
> nit: align parameters, also below

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
File chrome/browser/prefs/synced_pref_change_registrar.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar.cc:13: const std::string&
path, bool from_sync) {
On 2013/08/06 15:54:39, battre wrote:
> nit: each parameter into a new line.

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar.cc:28: void
SyncedPrefChangeRegistrar::Add(const char *path,
On 2013/08/06 15:54:39, battre wrote:
> nit: align parameters

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar.cc:47: iter !=
observers_.end(); ++iter) {
On 2013/08/06 15:54:39, battre wrote:
> nit: +1 space indent

Done.

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar.cc:57: void
SyncedPrefChangeRegistrar::OnSyncedPrefChanged(const std::string& path,
On 2013/08/06 15:54:39, battre wrote:
> nit: align parameters.

Done.

[battre, 2013-08-07 07:22:39]

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/21580002/diff/14002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:201: callback.Run(histogram_name,
pref->GetValue());
On 2013/08/06 16:27:46, rockot wrote:
> On 2013/08/06 15:54:39, battre wrote:
> > This may be a bug. pref->GetValue() does not necessary return the value we
got
> > from sync. It may return the value that is configured by a policy or
> extension.
> 
> I think it's correct as-is, but can you elaborate? A value changed by policy
> should never invoke this callback, because
> PrefModelAssociator::ProcessPrefChange is cut off before notifying observers
in
> the case where the pref !IsUserModifiable(). I'm not sure about values
> configured by extension - is PrefModelAssociator aware of the distinction?

This is what I had in mind.

A preference is controlled by policy. Now Sync updates the preference and
triggers the notification. We check the current value to report to UMA and won't
get the Sync value but the policy value.

[Ken Rockot(use gerrit already), 2013-08-07 23:45:05]

Ah, I understand now. I was confused by the comment being under
ProcessPrefChange, which only comes from local updates, not updates from sync.

The problem does exist with updates from sync. I've added some browser tests as
a starting point, ensuring that local and sync changes are differentiable and
ensuring that local changes to managed prefs do not notify observers.

Now I think we just need a strategy for dealing with sync changes to managed
prefs.

[Ken Rockot(use gerrit already), 2013-08-08 18:12:50]

What I've done for now is prevent SyncedPrefChangeRegistrar from notifying its
observers if the pref is managed.

A test was added to ensure no notification happens when a managed pref is
changed from sync.

[battre, 2013-08-09 12:04:00]

Great! LGTM

https://codereview.chromium.org/21580002/diff/44001/chrome/browser/prefs/sync...
File chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc (right):

https://codereview.chromium.org/21580002/diff/44001/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc:146:
ASSERT_TRUE(observer.has_been_notified);
this and the following should be EXPECT_*

https://codereview.chromium.org/21580002/diff/44001/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc:190:
ASSERT_FALSE(observer.has_been_notified);
this and the following should be EXPECT_*

https://codereview.chromium.org/21580002/diff/44001/chrome/browser/prefs/sync...
chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc:210:
ASSERT_FALSE(observer.has_been_notified);
as above

[commit-bot: I haz the power, 2013-08-09 15:00:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/21580002/61001

[commit-bot: I haz the power, 2013-08-09 18:48:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/21580002/87001

[commit-bot: I haz the power, 2013-08-10 07:07:13]

Change committed as 216813