Added more policy UMA metrics.

components/policy/core/browser/browser_policy_connector.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/policy/core/browser/browser_policy_connector.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/metrics/histogram.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "components/policy/core/common/configuration_policy_provider.h"
 #include "components/policy/core/common/policy_namespace.h"
 #include "components/policy/core/common/policy_pref_names.h"
 #include "components/policy/core/common/policy_service_impl.h"
 #include "components/policy/core/common/policy_statistics_collector.h"
 #include "components/policy/core/common/policy_switches.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "policy/policy_constants.h"
 #include "third_party/icu/source/i18n/unicode/regex.h"
 
 namespace policy {
 
 namespace {
 
 // The URL for the device management server.
 const char kDefaultDeviceManagementServerUrl[] =
     "https://m.google.com/devicemanagement/data/api";
 
 // Used in BrowserPolicyConnector::SetPolicyProviderForTesting.
 bool g_created_policy_service = false;
 ConfigurationPolicyProvider* g_testing_provider = NULL;
 
 // Returns true if |domain| matches the regex |pattern|.
-bool MatchDomain(const base::string16& domain, const base::string16& pattern) {
+bool MatchDomain(const base::string16& domain, const base::string16& pattern,
+                 size_t index, size_t max) {
   UErrorCode status = U_ZERO_ERROR;
   const icu::UnicodeString icu_pattern(pattern.data(), pattern.length());
   icu::RegexMatcher matcher(icu_pattern, UREGEX_CASE_INSENSITIVE, status);
   if (!U_SUCCESS(status)) {
     // http://crbug.com/365351 - if for some reason the matcher creation fails
     // just return that the pattern doesn't match the domain. This is safe
     // because the calling method (IsNonEnterpriseUser()) is just used to enable
     // an optimization for non-enterprise users - better to skip the
     // optimization than crash.
     DLOG(ERROR) << "Possible invalid domain pattern: " << pattern
                 << " - Error: " << status;
+    UMA_HISTOGRAM_BOOLEAN("Enterprise.DomainWhitelistRegexSuccess", false);
+    UMA_HISTOGRAM_ENUMERATION("Enterprise.DomainWhitelistRegexFailureIndex",
+                              index, max);

[Joao da Silva, 2014/07/11 15:13:47]

WDYT of sampling the UErrorCode in "status" with UMA_HISTOGRAM_SPARSE_SLOWLY?
That would tell us what is wrong with the pattern too (given that we can't repro
this locally... it may point at a strange locale or whatever)

[Andrew T Wilson (Slow), 2014/07/11 15:46:21]

I considered that, but I don't know what UMA_HISTOGRAM_SPARSE_SLOWLY is because
it's totally undocumented, so I'm uncomfortable using it. I can drive this in a
separate CL once someone responds to my email to chromium-dev about it.

[Ilya Sherman, 2014/07/11 22:54:12]

UMA_HISTOGRAM_SPARSE_SLOWLY just uses a map rather than a vector.

[Ilya Sherman, 2014/07/11 22:54:12]

Hmm, I'm concerned that |max| is being passed in as a parameter -- is it not a
compile-time constant?  UMA_HISTOGRAM_ENUMERATION defines a fixed-size vector
for the number of buckets specified by the third parameter (in this case, max),
so it's important that this be a constant.

[Andrew T Wilson (Slow), 2014/07/24 15:25:56]

So, what's the right way to use UMA_HISTOGRAM_SPARSE_SLOWLY() in my case? The
value I'm logging varies from -128 through around 0x10501 (from
third_party/icu/source/common/unicode/utypes.h). Do I need to try to add an enum
to histograms.xml for every single expected value, or is it possible to just
have it track this as an unlabeled int? What's the right way to do this? Use a
"units" attribute?

[Andrew T Wilson (Slow), 2014/07/24 15:25:56]

Done.

[Ilya Sherman, 2014/07/24 17:29:32]

It's up to you what values you list in histograms.xml -- if you prefer not to
include labels for any of the logged values, that's ok.  However, my
recommendation would be to include all of the constants from utypes.h as labels.
 You could probably write a quick script to generate these labels, and then you
would have an easier time making sense of the histogram from the dashboard.

     return false;
   }
+  UMA_HISTOGRAM_BOOLEAN("Enterprise.DomainWhitelistRegexSuccess", true);

[Ilya Sherman, 2014/07/11 22:54:12]

Please define a wrapper method around emitting to this histogram, so that
there's less chance of a typo sneaking into just one of the code paths.

[Andrew T Wilson (Slow), 2014/07/24 15:25:56]

Done.

   icu::UnicodeString icu_input(domain.data(), domain.length());
   matcher.reset(icu_input);
   status = U_ZERO_ERROR;
   UBool match = matcher.matches(status);
   DCHECK(U_SUCCESS(status));
   return !!match;  // !! == convert from UBool to bool.
 }
 
 }  // namespace
 
@@ skipping 137 matching lines @@
     L"mail\\.ru",
     L"msn\\.com",
     L"qq\\.com",
     L"yahoo(\\.co|\\.com|)\\.[^.]+", // yahoo.com, yahoo.co.uk, yahoo.com.tw
     L"yandex\\.ru",
   };
   const base::string16 domain = base::UTF8ToUTF16(
       gaia::ExtractDomainName(gaia::CanonicalizeEmail(username)));
   for (size_t i = 0; i < arraysize(kNonManagedDomainPatterns); i++) {
     base::string16 pattern = base::WideToUTF16(kNonManagedDomainPatterns[i]);
-    if (MatchDomain(domain, pattern))
+    if (MatchDomain(domain, pattern, i, arraysize(kNonManagedDomainPatterns)))

[Ilya Sherman, 2014/07/11 22:54:12]

Hmm, UMA is not intended to be used to track user navigation history.  Please
check with the privacy team whether this is an appropriate metric to record via
UMA.  Note that if you are really quite interested in this metric, you could
also use RAPPOR, which has higher privacy guarantees, and hence is more
appropriate for measuring data in aggregate that would be PII at the individual
level.

[Andrew T Wilson (Slow), 2014/07/24 15:25:56]

I'm not tracking user navigation. During signin, we generate a set of
ICUMatchers that are used to check if the signin domain is known to be
enterprise or not. The creation of the ICUMatcher is failing, and we want to
know which one - we're just returning the index of a hard-coded regex, not
anything related to the user data (it has nothing to do with the actual user's
domain, so no PII is being transmitted).

[Ilya Sherman, 2014/07/24 17:29:32]

I'm confused -- the failure is independent of the user input?  If so, why not
just debug locally, rather than via UMA data?

       return true;
   }
   return false;
 }
 
 // static
 std::string BrowserPolicyConnector::GetDeviceManagementUrl() {
   CommandLine* command_line = CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kDeviceManagementUrl))
     return command_line->GetSwitchValueASCII(switches::kDeviceManagementUrl);
@@ skipping 14 matching lines @@
 }
 
 void BrowserPolicyConnector::SetPlatformPolicyProvider(
     scoped_ptr<ConfigurationPolicyProvider> provider) {
   CHECK(!platform_policy_provider_);
   platform_policy_provider_ = provider.get();
   AddPolicyProvider(provider.Pass());
 }
 
 }  // namespace policy