UMA metrics for cloud policies.

UMA metrics for cloud policies.

BUG=chromium-os:14002
TEST=Set cloud policies using testserver, corrupt some files, check about:histograms


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

[Joao da Silva, 2011-06-24 17:06:10]

Here's an initial implementation of UMA stats for cloud policy and device
enrollment. I've manually verified the enrollment events, and the "normal case"
token and policy fetch events. about:histograms is quite helpful for this :-)

Please have a look, thanks!

[Glenn Wilson, 2011-06-27 19:18:26]

On 2011/06/24 17:06:10, joaodasilva wrote:
> Here's an initial implementation of UMA stats for cloud policy and device
> enrollment. I've manually verified the enrollment events, and the "normal
case"
> token and policy fetch events. about:histograms is quite helpful for this :-)
> 
> Please have a look, thanks!

Adding Ken as a reviewer, since he's much better equipped to review than I.

[kmixter1, 2011-06-28 01:06:02]

So I probably cannot comment on if this is complete (probably Mattias should),
but here are my comments on what I can gather from the CL.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:229: case
GoogleServiceAuthError::SERVICE_UNAVAILABLE:
is SERVICE_UNAVAILABLE a network/service transient error or something dmserver
is responding because it will never be able to enroll with this user.  If the
former, maybe this should be in the kEntrollmentNetworkFailed?

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:301:
em::LogEnrollmentOperation(em::kEnrollmentShowScreen);
how is this different from kEnrollmentStarted?  Is this just saying they hit
ctrl-alt-e vs hitting "submit" on that page?  Not sure how to use this data.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:116:
em::LogPolicyOperation(em::kPolicyFetchBadResponse);
Have we considered/investigated a paging/escalation system for these if we see
spikes in errors?

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:260:
em::LogPolicyOperation(em::kPolicyFetchInvalidPolicy);
Is there a user policy equivalent to this or are user policies not sent as
protobufs to be parsed?

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics.h (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics.h:16: kTokenLoadFailed,
If not here, somewhere, it would be good to comment on exactly what this count
signifies.  Some are self-explanatory - though mentioning it applies to both
device and user policies could be helpful.  Probably the most discoverable place
to do it would be the histogram description xml file - though there's only one
comment for each of these three histograms... so maybe the specific comments on
the enumerated values should live here.

[gfeher, 2011-06-28 09:58:58]

Mattias is on vacation, I step in instead of him to review.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics.cc:15: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
Are you sure this is necessary? I haven't seen this pattern elsewhere in the
code for UMA. (Although I may have missed it.) histogram.h talks something about
thread safety, so you may want to check it more carefully or ask its authors.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics.h (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics.h:9: namespace enterprise_management {
This namespace is so far only used for protobuf generated classes, and I think
it would be nice to keep it so.

I suggest using the policy namespace and one the following two (your choice):
1. adding a sub-namespace, like in policy_constants.h
2. adding a common prefix to each enum item, e.g. kMetricTokenLoadFailed, etc.
like in out/Debug/.../configuration_policy_type.h

[Joao da Silva, 2011-06-30 12:57:00]

Thanks for reviewing! Please have another look.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:229: case
GoogleServiceAuthError::SERVICE_UNAVAILABLE:
On 2011/06/28 01:06:02, kmixter1 wrote:
> is SERVICE_UNAVAILABLE a network/service transient error or something dmserver
> is responding because it will never be able to enroll with this user.  If the
> former, maybe this should be in the kEntrollmentNetworkFailed?
It's an error that can be returned from gaia, and is also the default for
unknown errors on the gaia_auth_fetcher. So this is an AuthError (login
failed,try again) instead of an AccountError (your account can't be used for
enrollment). It's been updated as such, wdyt?

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:301:
em::LogEnrollmentOperation(em::kEnrollmentShowScreen);
On 2011/06/28 01:06:02, kmixter1 wrote:
> how is this different from kEnrollmentStarted?  Is this just saying they hit
> ctrl-alt-e vs hitting "submit" on that page?  Not sure how to use this data.
As you mention, this meant the enrollment screen was shown, while
kEnrollmentStarted means that "submit" was pressed. This metric has now been
removed.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:116:
em::LogPolicyOperation(em::kPolicyFetchBadResponse);
On 2011/06/28 01:06:02, kmixter1 wrote:
> Have we considered/investigated a paging/escalation system for these if we see
> spikes in errors?
AFAIK that hasn't been discussed yet. It would be great to have it for
BadResponses and for spikes in other events too. Let's discuss it in the next
meeting.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:260:
em::LogPolicyOperation(em::kPolicyFetchInvalidPolicy);
On 2011/06/28 01:06:02, kmixter1 wrote:
> Is there a user policy equivalent to this or are user policies not sent as
> protobufs to be parsed?
There is an equivalent. Actually both invalid user policies and invalid device
policies are handled in cloud_policy_cache_base, which checks the return value
of DecodePolicyData(). This call here would actually log the same error twice,
and has been removed.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics.cc:15: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2011/06/28 09:58:58, gfeher wrote:
> Are you sure this is necessary? I haven't seen this pattern elsewhere in the
> code for UMA. (Although I may have missed it.) histogram.h talks something
about
> thread safety, so you may want to check it more carefully or ask its authors.
After checking histogram.h it seems to me that initialization of the counter is
thread safe, but adding samples is not.

Meanwhile this has been removed, and the call sites use
UMA_HISTOGRAM_ENUMERATION directly now, except the few that log from the FILE
thread.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics.h (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics.h:9: namespace enterprise_management {
On 2011/06/28 09:58:58, gfeher wrote:
> This namespace is so far only used for protobuf generated classes, and I think
> it would be nice to keep it so.
> 
> I suggest using the policy namespace and one the following two (your choice):
> 1. adding a sub-namespace, like in policy_constants.h
> 2. adding a common prefix to each enum item, e.g. kMetricTokenLoadFailed, etc.
> like in out/Debug/.../configuration_policy_type.h
Agree, refactored as per your second suggestion.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics.h:16: kTokenLoadFailed,
On 2011/06/28 01:06:02, kmixter1 wrote:
> If not here, somewhere, it would be good to comment on exactly what this count
> signifies.  Some are self-explanatory - though mentioning it applies to both
> device and user policies could be helpful.  Probably the most discoverable
place
> to do it would be the histogram description xml file - though there's only one
> comment for each of these three histograms... so maybe the specific comments
on
> the enumerated values should live here.
I've added a comment to each count, explaining when it is triggered.
histograms.xml has a section after the histograms mapping names to each enum
value, so we can add descriptive names there too.

[kmixter1, 2011-06-30 23:28:59]

LGTM on my parts

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:229: case
GoogleServiceAuthError::SERVICE_UNAVAILABLE:
On 2011/06/30 12:57:00, joaodasilva wrote:
> On 2011/06/28 01:06:02, kmixter1 wrote:
> > is SERVICE_UNAVAILABLE a network/service transient error or something
dmserver
> > is responding because it will never be able to enroll with this user.  If
the
> > former, maybe this should be in the kEntrollmentNetworkFailed?
> It's an error that can be returned from gaia, and is also the default for
> unknown errors on the gaia_auth_fetcher. So this is an AuthError (login
> failed,try again) instead of an AccountError (your account can't be used for
> enrollment). It's been updated as such, wdyt?

Agree - from your description it sounds like it is more like a problem related
to this particular login, not the particular user or network.

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller.cc (right):

http://codereview.chromium.org/7105018/diff/13001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller.cc:116:
em::LogPolicyOperation(em::kPolicyFetchBadResponse);
On 2011/06/30 12:57:00, joaodasilva wrote:
> On 2011/06/28 01:06:02, kmixter1 wrote:
> > Have we considered/investigated a paging/escalation system for these if we
see
> > spikes in errors?
> AFAIK that hasn't been discussed yet. It would be great to have it for
> BadResponses and for spikes in other events too. Let's discuss it in the next
> meeting.

Once this hits stable, we should present these in one of the meetings and start
considering how they could be turned into inputs for alert systems.

http://codereview.chromium.org/7105018/diff/26002/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics.h (right):

http://codereview.chromium.org/7105018/diff/26002/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics.h:17: kMetricTokenLoadRequested = 0,
Do you want to instead have loadsuccess?  Otherwise you can't compute % of loads
that failed. (it's not quite just the difference due to incomplete requests)
Same with TokenSave, PolicyLoad/Save.

[gfeher, 2011-07-01 09:41:17]

LGTM

http://codereview.chromium.org/7105018/diff/26002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/7105018/diff/26002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:318:
view()->RequestFocus();
Would it make sense to count the number of times the enrollment screen was
shown?

[Joao da Silva, 2011-07-07 15:30:13]

Thanks for reviewing! Addressed the last comments. Let's see if the bots agree
with the changes.

http://codereview.chromium.org/7105018/diff/26002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/7105018/diff/26002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:318:
view()->RequestFocus();
On 2011/07/01 09:41:17, gfeher wrote:
> Would it make sense to count the number of times the enrollment screen was
> shown?
That was considered but removed. Do you see a use for that data?

http://codereview.chromium.org/7105018/diff/26002/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics.h (right):

http://codereview.chromium.org/7105018/diff/26002/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics.h:17: kMetricTokenLoadRequested = 0,
On 2011/06/30 23:28:59, kmixter1 wrote:
> Do you want to instead have loadsuccess?  Otherwise you can't compute % of
loads
> that failed. (it's not quite just the difference due to incomplete requests)
> Same with TokenSave, PolicyLoad/Save.
Agree. The last CL is now logging successful loads/stores, instead of requests
to load/store.

[commit-bot: I haz the power, 2011-07-08 15:44:42]

Change committed as 91857