Add UMA histograms for policy loading.

chrome/browser/policy/config_dir_policy_loader.cc

 // Copyright (c) 2012 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 "chrome/browser/policy/config_dir_policy_loader.h"
 
 #include <algorithm>
 #include <set>
 #include <string>
 
 #include "base/bind.h"
 #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 {
 
 namespace {
 
 // Subdirectories that contain the mandatory and recommended policies.
 const base::FilePath::CharType kMandatoryConfigDir[] =
     FILE_PATH_LITERAL("managed");
 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,
                                              PolicyScope scope)
     : config_dir_(config_dir),
       scope_(scope) {}
 
 ConfigDirPolicyLoader::~ConfigDirPolicyLoader() {}
 
 void ConfigDirPolicyLoader::InitOnFile() {
@@ skipping 50 matching lines @@
                                          PolicyLevel level,
                                          PolicyBundle* bundle) {
   // Enumerate the files and sort them lexicographically.
   std::set<base::FilePath> files;
   file_util::FileEnumerator file_enumerator(path, false,
                                             file_util::FileEnumerator::FILES);
   for (base::FilePath config_file_path = file_enumerator.Next();
        !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
   // last file in lexicographic order.
   for (std::set<base::FilePath>::reverse_iterator config_file_iter =
            files.rbegin(); config_file_iter != files.rend();
        ++config_file_iter) {
     JSONFileValueSerializer deserializer(*config_file_iter);
     deserializer.set_allow_trailing_comma(true);
     int error_code = 0;
     std::string error_msg;
     scoped_ptr<base::Value> value(
         deserializer.Deserialize(&error_code, &error_msg));
     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;
     }
 
     // Detach the "3rdparty" node.
     base::Value* third_party = NULL;
     if (dictionary_value->Remove("3rdparty", &third_party)) {
       Merge3rdPartyPolicy(third_party, level, bundle);
       delete third_party;
     }
 
@@ skipping 56 matching lines @@
   }
 }
 
 void ConfigDirPolicyLoader::OnFileUpdated(const base::FilePath& path,
                                           bool error) {
   if (!error)
     Reload(false);
 }
 
 }  // namespace policy