Add UMA histograms for policy loading.

Add UMA histogram for policy loading.

The desktop platform policy loaders now report basic UMA stats on policy load
status, such as missing policy, policy read errors etc.

BUG=chromium:186445
TEST=Perform a couple policy loads, look at about:histograms to check the results.
TBR=jochen@chromium.org

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

[Mattias Nissler (ping if slow), 2013-04-16 15:34:07]

Joao: please review.

Jim: I'm not sure whether generating histogram names like I do here in
policy_load_status.cc is the right approach, and whether and what changes are
needed in histograms.xml, can you advise? Thanks.

[Joao da Silva, 2013-04-16 17:31:04]

I have some questions inline but overall looks sane, so lgtm.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
File chrome/browser/policy/config_dir_policy_loader.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:34: case
JSONFileValueSerializer::JSON_ACCESS_DENIED:
POLICY_LOAD_STATUS_INACCESSIBLE?

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:40: case
base::JSONReader::JSON_NO_ERROR:
I'm assuming this is the non-DictionaryValue case, so it's fine.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:125: PolicyLoadStatusSample
status(scope_, level);
Shouldn't this be inside the for loop? Otherwise we sample "started" once but we
may have N errors.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:127:
status.Add(POLICY_LOAD_STATUS_NO_POLICY);
Why do we sample this case?

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_load_status.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_load_status.cc:48: }
//  namespace

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_load_status.h (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_load_status.h:8: #include <string>
Not used

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:525: ReadRegistry(kScopes[i].hive,
chrome_policy_key_, &gpo_dict);
No sampling for this case?

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
File chrome/browser/policy/preg_parser_win.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
chrome/browser/policy/preg_parser_win.cc:288: data.resize(size);
This escaped the first code review: my concern back then was that |size| is a
wrong value, and resizing |data| blows up the memory consumption. I wouldn't
enforce a file size limit (that's up to the administrator), but I'd enforce a
buffer size limit here.

A broken Registry.pol file with a huge size field here would always crash chrome
on startup.

[Mattias Nissler (ping if slow), 2013-04-16 18:51:14]

Code updated, PTAL.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
File chrome/browser/policy/config_dir_policy_loader.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:34: case
JSONFileValueSerializer::JSON_ACCESS_DENIED:
On 2013/04/16 17:31:04, Joao da Silva wrote:
> POLICY_LOAD_STATUS_INACCESSIBLE?

Well, INACCESSIBLE was meant as "we decided we can't get to it", and these are
all conditions where we actually were OK with reading the file and got an error
code.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:40: case
base::JSONReader::JSON_NO_ERROR:
On 2013/04/16 17:31:04, Joao da Silva wrote:
> I'm assuming this is the non-DictionaryValue case, so it's fine.

Exactly. I've made this more obvious now by adding a NOTREACHED().

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:125: PolicyLoadStatusSample
status(scope_, level);
On 2013/04/16 17:31:04, Joao da Silva wrote:
> Shouldn't this be inside the for loop? Otherwise we sample "started" once but
we
> may have N errors.

True. Now I realize why I had PolicyLoadStatus done in a different way
originally. Reverted to that :)

I don't think sampling per file is a good idea since it'll skew the results to
favor people that have many policy files.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:127:
status.Add(POLICY_LOAD_STATUS_NO_POLICY);
On 2013/04/16 17:31:04, Joao da Silva wrote:
> Why do we sample this case?

The idea was to get a rough idea on the number of clients with policy present so
the error fractions make more sense.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_load_status.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_load_status.cc:48: }
On 2013/04/16 17:31:04, Joao da Silva wrote:
> //  namespace

Done.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_load_status.h (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_load_status.h:8: #include <string>
On 2013/04/16 17:31:04, Joao da Silva wrote:
> Not used

Done.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:525: ReadRegistry(kScopes[i].hive,
chrome_policy_key_, &gpo_dict);
On 2013/04/16 17:31:04, Joao da Silva wrote:
> No sampling for this case?

It's all good! :)

Assuming ReadPolicyFromGPO samples all its branches, we don't need anything
here.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
File chrome/browser/policy/preg_parser_win.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
chrome/browser/policy/preg_parser_win.cc:288: data.resize(size);
On 2013/04/16 17:31:04, Joao da Silva wrote:
> This escaped the first code review: my concern back then was that |size| is a
> wrong value, and resizing |data| blows up the memory consumption. I wouldn't
> enforce a file size limit (that's up to the administrator), but I'd enforce a
> buffer size limit here.
> 
> A broken Registry.pol file with a huge size field here would always crash
chrome
> on startup.

The kMaxPRegFileSize check two lines above should catch this situation? Or are
you worried about the 16M buffer?

[jar (doing other things), 2013-04-16 19:32:11]

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_load_status.cc (right):

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:50:
PolicyLoadStatusSample::PolicyLoadStatusSample(int scope, int level)
Just to be sure I'm following.... you'll potentially make 9 distinct histograms,
each with about 9 buckets, eh?

I don't like to see too many distinct histograms... but I'm not sure I see a
nice (easy to write) way to do this. You *could* create one histogram with 81
buckets... but you might actually like having it broken down as you currently
set it up.

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:61: LOG(ERROR) << "sampling " << i;
nit: use DLOG, as this probably won't be helpful to users, and will just clog
most logs.

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:62: histogram_->Add(i);
I was curious about why you put this into a bit vector, and then into the
histogram.
Are there cases where the same bit is set multiple times during the life of the
PolicyLoadStatusSample?

[Joao da Silva, 2013-04-16 19:32:57]

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
File chrome/browser/policy/config_dir_policy_loader.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:125: PolicyLoadStatusSample
status(scope_, level);
On 2013/04/16 18:51:14, Mattias Nissler wrote:
> On 2013/04/16 17:31:04, Joao da Silva wrote:
> > Shouldn't this be inside the for loop? Otherwise we sample "started" once
but
> we
> > may have N errors.
> 
> True. Now I realize why I had PolicyLoadStatus done in a different way
> originally. Reverted to that :)
> 
> I don't think sampling per file is a good idea since it'll skew the results to
> favor people that have many policy files.

This skews the errors to reads ratio. 10 files out of 20 may fail to read (50%
error rate), but if those 10 are in the same deployment then we only sample 11
reads and 10 errors (91% error rate!).

In practice the skewing won't be as significant. Up to you to choose which
sampling error you prefer :-)

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
File chrome/browser/policy/preg_parser_win.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
chrome/browser/policy/preg_parser_win.cc:29: { 'P', 'R', 'e', 'g', '\x01',
'\x00', '\x00', '\x00' };
Why did you revert this in the 2nd patch set?

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
chrome/browser/policy/preg_parser_win.cc:288: data.resize(size);
On 2013/04/16 18:51:14, Mattias Nissler wrote:
> On 2013/04/16 17:31:04, Joao da Silva wrote:
> > This escaped the first code review: my concern back then was that |size| is
a
> > wrong value, and resizing |data| blows up the memory consumption. I wouldn't
> > enforce a file size limit (that's up to the administrator), but I'd enforce
a
> > buffer size limit here.
> > 
> > A broken Registry.pol file with a huge size field here would always crash
> chrome
> > on startup.
> 
> The kMaxPRegFileSize check two lines above should catch this situation? Or are
> you worried about the 16M buffer?

Looking at this again I now see the size check in line 286, that somehow escaped
my attention earlier. Please ignore.

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/con...
File chrome/browser/policy/config_dir_policy_loader.cc (right):

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/con...
chrome/browser/policy/config_dir_policy_loader.cc:51: return
POLICY_LOAD_STATUS_STARTED;
Now I'm confused :-) When this is called because the top-level object is not a
dictionary (e.g. it's a list, or a string; this is the 2nd call from line 156,
where it's always JSON_NO_ERROR), then that's not a NOTREACHED(), I think that's
a parse error.

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_load_status.cc (right):

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:61: LOG(ERROR) << "sampling " << i;
Please remove this, or make it a VLOG.

[Mattias Nissler (ping if slow), 2013-04-17 10:16:35]

Thanks for the comments, new version is up for another round.

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
File chrome/browser/policy/config_dir_policy_loader.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/config_...
chrome/browser/policy/config_dir_policy_loader.cc:125: PolicyLoadStatusSample
status(scope_, level);
On 2013/04/16 19:32:57, Joao da Silva wrote:
> On 2013/04/16 18:51:14, Mattias Nissler wrote:
> > On 2013/04/16 17:31:04, Joao da Silva wrote:
> > > Shouldn't this be inside the for loop? Otherwise we sample "started" once
> but
> > we
> > > may have N errors.
> > 
> > True. Now I realize why I had PolicyLoadStatus done in a different way
> > originally. Reverted to that :)
> > 
> > I don't think sampling per file is a good idea since it'll skew the results
to
> > favor people that have many policy files.
> 
> This skews the errors to reads ratio. 10 files out of 20 may fail to read (50%
> error rate), but if those 10 are in the same deployment then we only sample 11
> reads and 10 errors (91% error rate!).

I'm not following. If we have 10 files in the mandatory directory, we sample one
read and any type of error we encounter in any of the files only once. That's
intentional - even if we fail only on one file I think it's fair to say the
entire policy load operation failed.

> 
> In practice the skewing won't be as significant. Up to you to choose which
> sampling error you prefer :-)

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
File chrome/browser/policy/preg_parser_win.cc (right):

https://codereview.chromium.org/14294008/diff/1/chrome/browser/policy/preg_pa...
chrome/browser/policy/preg_parser_win.cc:29: { 'P', 'R', 'e', 'g', '\x01',
'\x00', '\x00', '\x00' };
On 2013/04/16 19:32:57, Joao da Silva wrote:
> Why did you revert this in the 2nd patch set?

I didn't revert. The second patch set is rebased onto ToT which doesn't have the
other commit that adds this yet.

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/con...
File chrome/browser/policy/config_dir_policy_loader.cc (right):

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/con...
chrome/browser/policy/config_dir_policy_loader.cc:51: return
POLICY_LOAD_STATUS_STARTED;
On 2013/04/16 19:32:57, Joao da Silva wrote:
> Now I'm confused :-) When this is called because the top-level object is not a
> dictionary (e.g. it's a list, or a string; this is the 2nd call from line 156,
> where it's always JSON_NO_ERROR), then that's not a NOTREACHED(), I think
that's
> a parse error.

Oh, correct. I misread your earlier comment. Fixed.

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_load_status.cc (right):

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:50:
PolicyLoadStatusSample::PolicyLoadStatusSample(int scope, int level)
On 2013/04/16 19:32:11, jar wrote:
> Just to be sure I'm following.... you'll potentially make 9 distinct
histograms,
> each with about 9 buckets, eh?

Correct, the intent was to distinguish load operations in different contexts.
Note that on a given platform, we only fill 4 distinct histograms at max - the
platform either does loads for a specific scope or level, or the loader loads
all data at once for all scopes and/or levels.

> 
> I don't like to see too many distinct histograms... but I'm not sure I see a
> nice (easy to write) way to do this. You *could* create one histogram with 81
> buckets... but you might actually like having it broken down as you currently
> set it up.

Thinking more about it, there's probably not much reason to have distinct
histograms, at least not for getting basic health stats from the entire
population. I've now cut this down to use only a single histogram; I guess we
can add more complicated sampling if we have specific debugging needs in the
future. Joao, are you cool with that?

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:61: LOG(ERROR) << "sampling " << i;
On 2013/04/16 19:32:57, Joao da Silva wrote:
> Please remove this, or make it a VLOG.

Doh, debugging artifact. Fixed.

https://codereview.chromium.org/14294008/diff/14001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:62: histogram_->Add(i);
On 2013/04/16 19:32:11, jar wrote:
> I was curious about why you put this into a bit vector, and then into the
> histogram.
> Are there cases where the same bit is set multiple times during the life of
the
> PolicyLoadStatusSample?

Yes, that's exactly the reason. We may loop over multiple files and I want to
record only one sample even if we encounter an error in more than one file. That
way, we get statistics that indicate the frequency of specific error types
w.r.t. the total number of policy load attempts. For the load attempts, I record
a POLICY_LOAD_STATUS_STARTED sample unconditionally.

[Joao da Silva, 2013-04-17 10:48:47]

lgtm

https://codereview.chromium.org/14294008/diff/37001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_load_status.cc (right):

https://codereview.chromium.org/14294008/diff/37001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:20: :
histogram_(base::LinearHistogram::FactoryGet(
nit: indent

[Mattias Nissler (ping if slow), 2013-04-17 13:41:36]

https://codereview.chromium.org/14294008/diff/37001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_load_status.cc (right):

https://codereview.chromium.org/14294008/diff/37001/chrome/browser/policy/pol...
chrome/browser/policy/policy_load_status.cc:20: :
histogram_(base::LinearHistogram::FactoryGet(
On 2013/04/17 10:48:47, Joao da Silva wrote:
> nit: indent

Done.

[Mattias Nissler (ping if slow), 2013-04-18 12:33:23]

TBR jochen@ for the gyp changes. Ticking the box now.

[commit-bot: I haz the power, 2013-04-18 12:33:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/14294008/51001

[jochen (gone - plz use gerrit), 2013-04-18 12:35:24]

gypi lgtm

[commit-bot: I haz the power, 2013-04-18 14:32:07]

Change committed as 194919