Enrollment recovery: Add UMA stats.

Enrollment recovery: Add UMA stats.

Add UMA stats to track frequency and success rate of enrollment recovery.

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

[Thiemo Nagel, 2014-07-11 19:17:00]

Hi Julian,

this is how I intend to add UMA stats for enrollment recovery.  I still need to
test the code, but if you like, feel free to take a look already now.

Best,
Thiemo

[Thiemo Nagel, 2014-07-14 08:38:42]

Hi Joao,

could you please take a look at chrome/browser/chromeos/login/enrollment/* ?

Thank you!
Thiemo

[Thiemo Nagel, 2014-07-14 08:41:31]

Hi Pavel,

may I ask you to take a look at WizardController, EnrollmentScreenHandler and
oobe_screen_oauth_enrollment.css?

Thank you and best regards,
Thiemo

[Thiemo Nagel, 2014-07-14 08:43:41]

Hi Alexei,

may I ask you to take a look at my addition to histograms.xml?

Thank you and best regards,
Thiemo

[pastarmovj, 2014-07-14 08:46:06]

a couple of comments.

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:142:
enrollment_mode_ == EnrollmentScreenActor::ENROLLMENT_MODE_RECOVERY) {
Is this change part of simply adding the UMAs? It seems to do more than that.

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:316: if
(is_auto_enrollment())
Since auto enrollment is going away anyway can you instead merge this in a
single UMA function. It seems to me there is no need to have two.

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:55: return
kModeStrings[mode];
This is potentially an issue. You should not rely only on the compiler to guard
valid values of the enum. If for some reason this code should work against some
other lib/compilation unit which sees different definition of the enum you can
easily end up with out of bounds error.

I would add a CHECK on the mode here.

[Joao da Silva, 2014-07-14 10:15:50]

lgtm bar remaining comments

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:61:
actor_->Show();
Nothing is sampled in this case?

[Thiemo Nagel, 2014-07-14 10:30:35]

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:142:
enrollment_mode_ == EnrollmentScreenActor::ENROLLMENT_MODE_RECOVERY) {
On 2014/07/14 08:46:06, pastarmovj wrote:
> Is this change part of simply adding the UMAs? It seems to do more than that.

This CL introduces ENROLLMENT_MODE_RECOVERY which didn't previously exist.  (I
used to piggy-back on ENROLLMENT_MODE_FORCED but this turns out to not work very
well when recovery-specific logging comes into play.)  The control flow should
be unchanged.

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:316: if
(is_auto_enrollment())
On 2014/07/14 08:46:06, pastarmovj wrote:
> Since auto enrollment is going away anyway can you instead merge this in a
> single UMA function. It seems to me there is no need to have two.

I fully agree, but I'd rather do that in a separate CL (which will not be
back-merged).

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:55: return
kModeStrings[mode];
On 2014/07/14 08:46:06, pastarmovj wrote:
> This is potentially an issue. You should not rely only on the compiler to
guard
> valid values of the enum. If for some reason this code should work against
some
> other lib/compilation unit which sees different definition of the enum you can
> easily end up with out of bounds error.
> 
> I would add a CHECK on the mode here.

Done.

[Thiemo Nagel, 2014-07-14 11:04:30]

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/385983007/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:61:
actor_->Show();
On 2014/07/14 10:15:50, Joao da Silva wrote:
> Nothing is sampled in this case?

This code path is just for auto-enrollment which I don't care about and which
will go away soon, anyways.

[pastarmovj, 2014-07-14 13:05:42]

lgtm

[Alexei Svitkine (slow), 2014-07-14 13:58:58]

https://codereview.chromium.org/385983007/diff/110001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/385983007/diff/110001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:310: histogram =
policy::kMetricEnrollment;
This is incorrect. The UMA_HISTOGRAM_*() macros must take a constant string,
since the first invocation of the macro caches the Histogram object. Please use
two different macros here.

[Thiemo Nagel, 2014-07-14 14:20:08]

Thank you very much!  Could you please take another look?

Best,
Thiemo

https://codereview.chromium.org/385983007/diff/110001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/385983007/diff/110001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:310: histogram =
policy::kMetricEnrollment;
On 2014/07/14 13:58:58, Alexei Svitkine wrote:
> This is incorrect. The UMA_HISTOGRAM_*() macros must take a constant string,
> since the first invocation of the macro caches the Histogram object. Please
use
> two different macros here.

Thank you for catching this!  I wasn't aware of that property of UMA histograms.
 Definitely good to know...

[Alexei Svitkine (slow), 2014-07-14 15:21:29]

lgtm

[Thiemo Nagel, 2014-07-14 15:32:48]

On 2014/07/14 15:21:29, Alexei Svitkine wrote:
> lgtm

Thank you!

[dzhioev (left Google), 2014-07-15 10:08:58]

LGTM

[Thiemo Nagel, 2014-07-15 18:58:03]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-07-15 19:01:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/385983007/130001

[commit-bot: I haz the power, 2014-07-16 04:54:35]

Change committed as 283342