Add additional histograms with suffixes to ImportantFileWriter.

base/files/important_file_writer.cc

 // Copyright (c) 2011 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 "base/files/important_file_writer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/critical_closure.h"
 #include "base/debug/alias.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 
@@ skipping 10 matching lines @@
 enum TempFileFailure {
   FAILED_CREATING,
   FAILED_OPENING,
   FAILED_CLOSING,  // Unused.
   FAILED_WRITING,
   FAILED_RENAMING,
   FAILED_FLUSHING,
   TEMP_FILE_FAILURE_MAX
 };
 
-void LogFailure(const FilePath& path, TempFileFailure failure_code,
+// Helper function to write samples to a histogram with a dynamically assigned
+// histogram name.  Works with different error code types convertible to int
+// which is the actual argument type of UmaHistogramExactLinear.
+template <typename SampleType>
+typename std::enable_if<std::is_convertible<SampleType, int>::value>::type
+UmaHistogramExactLinearWithSuffix(const char* histogram_name,
+                                  const char* histogram_suffix,
+                                  SampleType add_sample,
+                                  SampleType max_sample) {
+  DCHECK(histogram_name);
+  auto histogram_full_name_length = strlen(histogram_name);
+  if (histogram_suffix && *histogram_suffix)
+    histogram_full_name_length +=
+        strlen(histogram_suffix) + 1;  // Also account for a dot.

[dcheng, 2017/06/07 22:56:39]

To be honest, I don't know that it's worth the effort to calculate the length to
reserve, but maybe someone will have an alternate opinion. I think it'd be
easier to read if it was just += / append()

[xaerox, 2017/06/08 05:52:53]

I agree that one additional reallocation is worth the better readability of the
code. Hopefully it doesn't look like premature pessimization. Fixed.

+  std::string histogram_full_name;
+  histogram_full_name.reserve(histogram_full_name_length);
+  histogram_full_name.append(histogram_name);
+  if (histogram_suffix && *histogram_suffix) {
+    histogram_full_name.append(".");
+    histogram_full_name.append(histogram_suffix);
+  }
+  UmaHistogramExactLinear(histogram_full_name, static_cast<int>(add_sample),
+                          static_cast<int>(max_sample));
+}
+
+void LogFailure(const FilePath& path,
+                StringPiece histogram_suffix,
+                TempFileFailure failure_code,
                 StringPiece message) {
-  UMA_HISTOGRAM_ENUMERATION("ImportantFile.TempFileFailures", failure_code,
-                            TEMP_FILE_FAILURE_MAX);
+  UmaHistogramExactLinearWithSuffix("ImportantFile.TempFileFailures",
+                                    histogram_suffix.data(), failure_code,
+                                    TEMP_FILE_FAILURE_MAX);
   DPLOG(WARNING) << "temp file failure: " << path.value() << " : " << message;
 }
 
 // Helper function to call WriteFileAtomically() with a
 // std::unique_ptr<std::string>.
 void WriteScopedStringToFileAtomically(
     const FilePath& path,
     std::unique_ptr<std::string> data,
     Closure before_write_callback,
-    Callback<void(bool success)> after_write_callback) {
+    Callback<void(bool success)> after_write_callback,
+    const std::string& histogram_suffix) {
   if (!before_write_callback.is_null())
     before_write_callback.Run();
 
-  bool result = ImportantFileWriter::WriteFileAtomically(path, *data);
+  bool result = ImportantFileWriter::WriteFileAtomically(
+      path, *data, histogram_suffix.c_str());

[dcheng, 2017/06/07 22:56:39]

No need to call c_str() for this

[xaerox, 2017/06/08 05:52:53]

Done.

 
   if (!after_write_callback.is_null())
     after_write_callback.Run(result);
 }
 
+base::File::Error GetLastFileError() {
+#if defined(OS_WIN)
+  return base::File::OSErrorToFileError(::GetLastError());
+#elif defined(OS_POSIX)
+  return base::File::OSErrorToFileError(errno);
+#else
+  return base::File::FILE_OK;
+#endif
+}
+
+void DeleteTmpFile(const FilePath& tmp_file_path,
+                   StringPiece histogram_suffix) {
+  if (!DeleteFile(tmp_file_path, false)) {
+    UmaHistogramExactLinearWithSuffix(
+        "ImportantFile.FileDeleteError", histogram_suffix.data(),
+        -GetLastFileError(), -base::File::FILE_ERROR_MAX);
+  }
+}
+
 }  // namespace
 
 // static
 bool ImportantFileWriter::WriteFileAtomically(const FilePath& path,
-                                              StringPiece data) {
+                                              StringPiece data,
+                                              StringPiece histogram_suffix) {
 #if defined(OS_CHROMEOS)
   // On Chrome OS, chrome gets killed when it cannot finish shutdown quickly,
   // and this function seems to be one of the slowest shutdown steps.
   // Include some info to the report for investigation. crbug.com/418627
   // TODO(hashimoto): Remove this.
   struct {
     size_t data_size;
     char path[128];
   } file_info;
   file_info.data_size = data.size();
   strlcpy(file_info.path, path.value().c_str(), arraysize(file_info.path));
   debug::Alias(&file_info);
 #endif
 
   // Write the data to a temp file then rename to avoid data loss if we crash
   // while writing the file. Ensure that the temp file is on the same volume
   // as target file, so it can be moved in one step, and that the temp file
   // is securely created.
   FilePath tmp_file_path;
   if (!CreateTemporaryFileInDir(path.DirName(), &tmp_file_path)) {
-    LogFailure(path, FAILED_CREATING, "could not create temporary file");
+    UmaHistogramExactLinearWithSuffix(
+        "ImportantFile.FileCreateError", histogram_suffix.data(),
+        -GetLastFileError(), -base::File::FILE_ERROR_MAX);
+    LogFailure(path, histogram_suffix, FAILED_CREATING,
+               "could not create temporary file");
     return false;
   }
 
   File tmp_file(tmp_file_path, File::FLAG_OPEN | File::FLAG_WRITE);
   if (!tmp_file.IsValid()) {
-    LogFailure(path, FAILED_OPENING, "could not open temporary file");
+    UmaHistogramExactLinearWithSuffix(
+        "ImportantFile.FileOpenError", histogram_suffix.data(),
+        -tmp_file.error_details(), -base::File::FILE_ERROR_MAX);
+    LogFailure(path, histogram_suffix, FAILED_OPENING,
+               "could not open temporary file");
     DeleteFile(tmp_file_path, false);
     return false;
   }
 
   // If this fails in the wild, something really bad is going on.
   const int data_length = checked_cast<int32_t>(data.length());
   int bytes_written = tmp_file.Write(0, data.data(), data_length);
+  if (bytes_written < data_length) {
+    UmaHistogramExactLinearWithSuffix(
+        "ImportantFile.FileWriteError", histogram_suffix.data(),
+        -GetLastFileError(), -base::File::FILE_ERROR_MAX);
+  }
   bool flush_success = tmp_file.Flush();
   tmp_file.Close();
 
   if (bytes_written < data_length) {
-    LogFailure(path, FAILED_WRITING, "error writing, bytes_written=" +
-               IntToString(bytes_written));
-    DeleteFile(tmp_file_path, false);
+    LogFailure(path, histogram_suffix, FAILED_WRITING,
+               "error writing, bytes_written=" + IntToString(bytes_written));
+    DeleteTmpFile(tmp_file_path, histogram_suffix.data());
     return false;
   }
 
   if (!flush_success) {
-    LogFailure(path, FAILED_FLUSHING, "error flushing");
-    DeleteFile(tmp_file_path, false);
+    LogFailure(path, histogram_suffix, FAILED_FLUSHING, "error flushing");
+    DeleteTmpFile(tmp_file_path, histogram_suffix);
     return false;
   }
 
-  if (!ReplaceFile(tmp_file_path, path, nullptr)) {
-    LogFailure(path, FAILED_RENAMING, "could not rename temporary file");
-    DeleteFile(tmp_file_path, false);
+  base::File::Error replace_file_error = base::File::FILE_OK;
+  if (!ReplaceFile(tmp_file_path, path, &replace_file_error)) {
+    UmaHistogramExactLinearWithSuffix(
+        "ImportantFile.FileRenameError", histogram_suffix.data(),
+        -replace_file_error, -base::File::FILE_ERROR_MAX);
+    LogFailure(path, histogram_suffix, FAILED_RENAMING,
+               "could not rename temporary file");
+    DeleteTmpFile(tmp_file_path, histogram_suffix);
     return false;
   }
 
   return true;
 }
 
 ImportantFileWriter::ImportantFileWriter(
     const FilePath& path,
-    scoped_refptr<SequencedTaskRunner> task_runner)
-    : ImportantFileWriter(path,
-                          std::move(task_runner),
-                          kDefaultCommitInterval) {}
-
-ImportantFileWriter::ImportantFileWriter(
-    const FilePath& path,
     scoped_refptr<SequencedTaskRunner> task_runner,
-    TimeDelta interval)
+    StringPiece histogram_suffix)
+    : ImportantFileWriter(path,
+                          std::move(task_runner),
+                          kDefaultCommitInterval,
+                          histogram_suffix) {}
+
+ImportantFileWriter::ImportantFileWriter(
+    const FilePath& path,
+    scoped_refptr<SequencedTaskRunner> task_runner,
+    TimeDelta interval,
+    StringPiece histogram_suffix)
     : path_(path),
       task_runner_(std::move(task_runner)),
       serializer_(nullptr),
       commit_interval_(interval),
+      histogram_suffix_(histogram_suffix),
       weak_factory_(this) {
   DCHECK(task_runner_);
 }
 
 ImportantFileWriter::~ImportantFileWriter() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // We're usually a member variable of some other object, which also tends
   // to be our serializer. It may not be safe to call back to the parent object
   // being destructed.
   DCHECK(!HasPendingWrite());
 }
 
 bool ImportantFileWriter::HasPendingWrite() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return timer().IsRunning();
 }
 
 void ImportantFileWriter::WriteNow(std::unique_ptr<std::string> data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!IsValueInRangeForNumericType<int32_t>(data->length())) {
     NOTREACHED();
     return;
   }
 
   Closure task = AdaptCallbackForRepeating(
       BindOnce(&WriteScopedStringToFileAtomically, path_, std::move(data),
                std::move(before_next_write_callback_),
-               std::move(after_next_write_callback_)));
+               std::move(after_next_write_callback_), histogram_suffix_));
 
   if (!task_runner_->PostTask(FROM_HERE, MakeCriticalClosure(task))) {
     // Posting the task to background message loop is not expected
     // to fail, but if it does, avoid losing data and just hit the disk
     // on the current thread.
     NOTREACHED();
 
     task.Run();
   }
   ClearPendingWrite();
@@ skipping 34 matching lines @@
 void ImportantFileWriter::ClearPendingWrite() {
   timer().Stop();
   serializer_ = nullptr;
 }
 
 void ImportantFileWriter::SetTimerForTesting(Timer* timer_override) {
   timer_override_ = timer_override;
 }
 
 }  // namespace base