Add UMA histograms for policy loading.

chrome/browser/policy/config_dir_policy_loader.cc

Index: chrome/browser/policy/config_dir_policy_loader.cc
diff --git a/chrome/browser/policy/config_dir_policy_loader.cc b/chrome/browser/policy/config_dir_policy_loader.cc
index 5e6505833105530c1fc74b8835abebf31c341b6d..3c34f0326c9562b15c4dd07d165c63b6c9492922 100644
--- a/chrome/browser/policy/config_dir_policy_loader.cc
+++ b/chrome/browser/policy/config_dir_policy_loader.cc
@@ -12,10 +12,12 @@
 #include "base/bind_helpers.h"
 #include "base/file_util.h"
 #include "base/json/json_file_value_serializer.h"
+#include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/platform_file.h"
 #include "base/stl_util.h"
 #include "chrome/browser/policy/policy_bundle.h"
+#include "chrome/browser/policy/policy_load_status.h"
 
 namespace policy {
 
@@ -27,6 +29,29 @@ const base::FilePath::CharType kMandatoryConfigDir[] =
 const base::FilePath::CharType kRecommendedConfigDir[] =
     FILE_PATH_LITERAL("recommended");
 
+PolicyLoadStatus JsonErrorToPolicyLoadStatus(int status) {
+  switch (status) {
+    case JSONFileValueSerializer::JSON_ACCESS_DENIED:

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

POLICY_LOAD_STATUS_INACCESSIBLE?

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

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.

+    case JSONFileValueSerializer::JSON_CANNOT_READ_FILE:
+    case JSONFileValueSerializer::JSON_FILE_LOCKED:
+      return POLICY_LOAD_STATUS_READ_ERROR;
+    case JSONFileValueSerializer::JSON_NO_SUCH_FILE:
+      return POLICY_LOAD_STATUS_MISSING;
+    case base::JSONReader::JSON_NO_ERROR:

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

I'm assuming this is the non-DictionaryValue case, so it's fine.

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

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

+    case base::JSONReader::JSON_INVALID_ESCAPE:
+    case base::JSONReader::JSON_SYNTAX_ERROR:
+    case base::JSONReader::JSON_UNEXPECTED_TOKEN:
+    case base::JSONReader::JSON_TRAILING_COMMA:
+    case base::JSONReader::JSON_TOO_MUCH_NESTING:
+    case base::JSONReader::JSON_UNEXPECTED_DATA_AFTER_ROOT:
+    case base::JSONReader::JSON_UNSUPPORTED_ENCODING:
+    case base::JSONReader::JSON_UNQUOTED_DICTIONARY_KEY:
+      return POLICY_LOAD_STATUS_PARSE_ERROR;
+  }
+  NOTREACHED() << "Invalid status " << status;
+  return POLICY_LOAD_STATUS_PARSE_ERROR;
+}
+
 }  // namespace
 
 ConfigDirPolicyLoader::ConfigDirPolicyLoader(const base::FilePath& config_dir,
@@ -97,6 +122,12 @@ void ConfigDirPolicyLoader::LoadFromPath(const base::FilePath& path,
        !config_file_path.empty(); config_file_path = file_enumerator.Next())
     files.insert(config_file_path);
 
+  PolicyLoadStatusSample status(scope_, level);

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

Shouldn't this be inside the for loop? Otherwise we sample "started" once but we
may have N errors.

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

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.

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

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 :-)

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

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.

+  if (files.empty()) {
+    status.Add(POLICY_LOAD_STATUS_NO_POLICY);

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

Why do we sample this case?

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

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

+    return;
+  }
+
   // Start with an empty dictionary and merge the files' contents.
   // The files are processed in reverse order because |MergeFrom| gives priority
   // to existing keys, but the ConfigDirPolicyProvider gives priority to the
@@ -113,12 +144,14 @@ void ConfigDirPolicyLoader::LoadFromPath(const base::FilePath& path,
     if (!value.get()) {
       LOG(WARNING) << "Failed to read configuration file "
                    << config_file_iter->value() << ": " << error_msg;
+      status.Add(JsonErrorToPolicyLoadStatus(error_code));
       continue;
     }
     base::DictionaryValue* dictionary_value = NULL;
     if (!value->GetAsDictionary(&dictionary_value)) {
       LOG(WARNING) << "Expected JSON dictionary in configuration file "
                    << config_file_iter->value();
+      status.Add(JsonErrorToPolicyLoadStatus(error_code));
       continue;
     }