Create a histogram serialization mechanism in components/metrics

components/metrics/chromeos/metrics_utils.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/metrics/chromeos/metrics_utils.h"
+
+#include <sys/file.h>
+
+#include <string>
+#include <vector>
+
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/files/scoped_file.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
+#include "components/metrics/chromeos/metric_sample.h"
+
+#define READ_WRITE_ALL_FILE_FLAGS \
+  (S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH)
+
+namespace metrics {
+
+scoped_ptr<MetricSample> MetricsUtils::ReadSample(const std::string& sample) {
+  std::vector<std::string> parts;
+  base::SplitString(sample, '\0', &parts);
+  // We should have two null terminated strings so split should produce
+  // three chunks.
+  if (parts.size() != 3) {
+    DLOG(ERROR) << "splitting message on \\0 produced " << parts.size()
+                << " parts (expected 3)";
+    return scoped_ptr<MetricSample>();
+  }
+  const std::string& name = parts[0];
+  const std::string& value = parts[1];
+
+  if (LowerCaseEqualsASCII(name, "crash")) {
+    return MetricSample::CrashSample(value);
+  } else if (LowerCaseEqualsASCII(name, "histogram")) {
+    return MetricSample::ReadHistogram(value);
+  } else if (LowerCaseEqualsASCII(name, "linearhistogram")) {
+    return MetricSample::ReadLinearHistogram(value);
+  } else if (LowerCaseEqualsASCII(name, "sparsehistogram")) {
+    return MetricSample::ReadSparseHistogram(value);
+  } else if (LowerCaseEqualsASCII(name, "useraction")) {
+    return MetricSample::UserActionSample(value);
+  } else {
+    DLOG(ERROR) << "invalid event type: " << name << ", value: " << value;
+  }
+  return scoped_ptr<MetricSample>();
+}
+
+bool MetricsUtils::WriteMetricToFile(const MetricSample* sample,

[Alexei Svitkine (slow), 2014/05/15 13:43:26]

Can this take a const& sample?

[bsimonnet, 2014/05/15 19:23:40]

Done.

+                                     const std::string& filename) {
+  if (!sample->IsValid())
+    return false;
+
+  base::ScopedFD file_descriptor(open(filename.c_str(),
+                                      O_WRONLY | O_APPEND | O_CREAT,
+                                      READ_WRITE_ALL_FILE_FLAGS));
+
+  if (file_descriptor.get() < 0) {
+    DLOG(ERROR) << "error openning the file";
+    return false;
+  }
+
+  fchmod(file_descriptor.get(), READ_WRITE_ALL_FILE_FLAGS);
+  // Grab a lock to avoid chrome truncating the file
+  // underneath us. Keep the file locked as briefly as possible.
+  // Freeing file_descriptor will close the file and and remove the lock.
+  if (HANDLE_EINTR(flock(file_descriptor.get(), LOCK_EX)) < 0) {
+    DLOG(ERROR) << "error locking" << filename << " : " << errno;
+    return false;
+  }
+
+  std::string msg = sample->ToString();
+  int32 size = msg.length() + sizeof(int32);
+  if (size > kMessageMaxLength) {
+    DLOG(ERROR) << "cannot write message: too long";
+    return false;
+  }
+
+  // The file containing the metrics samples will only be read by programs on
+  // the same device so we do not check endianness.
+  if (base::WriteFileDescriptor(file_descriptor.get(),
+                                reinterpret_cast<char*>(&size),
+                                sizeof(size)) != sizeof(size)) {
+    DLOG(ERROR) << "error writing message length " << errno;
+    return false;
+  }
+
+  if (base::WriteFileDescriptor(
+          file_descriptor.get(), msg.c_str(), msg.length()) !=
+      static_cast<int>(msg.length())) {
+    DLOG(ERROR) << "error writing message" << errno;
+    return false;
+  }
+
+  return true;
+}
+
+void MetricsUtils::ReadAndTruncateMetricsFromFile(
+    const std::string& filename,
+    ScopedVector<MetricSample>* metrics) {
+  struct stat stat_buf;
+  int result;
+
+  result = stat(filename.c_str(), &stat_buf);
+  if (result < 0) {
+    if (errno != ENOENT) {

[Alexei Svitkine (slow), 2014/05/15 13:43:26]

Nit: No {}'s

[bsimonnet, 2014/05/15 19:23:40]

Done.

+      DPLOG(ERROR) << filename << ": bad metrics file stat";
+    }
+    // Nothing to collect---try later.
+    return;
+  }
+  if (stat_buf.st_size == 0) {
+    // Also nothing to collect.
+    return;
+  }
+  base::ScopedFD fd(open(filename.c_str(), O_RDWR));
+  if (fd.get() < 0) {
+    DPLOG(ERROR) << filename << ": cannot open";
+    return;
+  }
+  result = flock(fd.get(), LOCK_EX);
+  if (result < 0) {
+    DPLOG(ERROR) << filename << ": cannot lock";
+    return;
+  }
+  // This processes all messages in the log.  Each message starts with a 4-byte

[Alexei Svitkine (slow), 2014/05/15 13:43:26]

Nit: Add an empty line before this.

[bsimonnet, 2014/05/15 19:23:40]

Done.

+  // field containing the length of the entire message.  The length is followed
+  // by a name-value pair of null-terminated strings.  When all messages are
+  // read and processed, or an error occurs, truncate the file to zero size.
+  for (;;) {
+    int32 message_size;
+    // The file containing the metrics do not leave the device so the writer and
+    // the reader will always have the same endianness.
+    result = HANDLE_EINTR(read(fd.get(), &message_size, sizeof(message_size)));
+    if (result < 0) {
+      DPLOG(ERROR) << "reading metrics message header";
+      break;
+    }
+    if (result == 0) {  // This indicates a normal EOF.

[Alexei Svitkine (slow), 2014/05/15 13:43:26]

Nit: No {}'s. Or Keep {}'s and move comment to the line below.

[bsimonnet, 2014/05/15 19:23:40]

Done.

+      break;
+    }
+    if (result < static_cast<int>(sizeof(message_size))) {
+      DLOG(ERROR) << "bad read size " << result << ", expecting "
+                  << sizeof(message_size);
+      break;
+    }
+    // kMessageMaxLength applies to the entire message: the 4-byte

[Alexei Svitkine (slow), 2014/05/15 13:43:26]

This comment seems misplaced since the check directly below it doesn't use
kMessageMaxLength.

[bsimonnet, 2014/05/15 19:23:40]

Done.

+    // length field and the two null-terminated strings.
+    if (message_size < 2 + static_cast<int>(sizeof(message_size))) {

[Alexei Svitkine (slow), 2014/05/15 13:43:26]

Do we really need the magic number 2 here? It seems bad to put the assumption
about the two null chars in this file.

It seems to me that ReadSample() should already fail if the part after the
message is < 2 chars in size, so maybe just remove the 2? (And the comment about
null terminated strings from this function.)

[bsimonnet, 2014/05/15 19:23:40]

Done.

+      DLOG(ERROR) << "bad message size " << message_size;
+      break;
+    }
+
+    if (message_size > kMessageMaxLength) {
+      DLOG(ERROR) << "message too long : " << message_size;
+      if (HANDLE_EINTR(lseek(fd.get(), message_size - 4, SEEK_CUR)) == -1) {
+        DLOG(ERROR) << "error while skipping message. abort";
+        break;
+      }
+      continue;
+    }
+
+    message_size -= sizeof(message_size);  // The message size includes itself.
+    char buffer[kMessageMaxLength];
+    result = HANDLE_EINTR(read(fd.get(), buffer, message_size));
+    if (result < 0) {
+      DPLOG(ERROR) << "reading metrics message body";
+      break;
+    }
+    if (result < message_size) {
+      DLOG(ERROR) << "message too short: length " << result << ", expected "
+                  << message_size;
+      break;
+    }

[Alexei Svitkine (slow), 2014/05/15 13:43:26]

Nit: Consider making a helper function for reading the message that will make
this function more concise.

i.e. the loop body can then become:

std::string message;
bool success = ReadMessage(fd.get(), &message);
if (!success)
  break;
scoped_ptr<MetricSample> sample(ParseSample(message));
if (!sample)
  break;
metrics->push_back(sample.release());

[bsimonnet, 2014/05/15 19:23:40]

Done.

+    scoped_ptr<MetricSample> sample =
+        ReadSample(std::string(buffer, message_size));
+    if (sample.get() == NULL) {

[Alexei Svitkine (slow), 2014/05/15 13:43:26]

Nit: just !sample and remove {}'s

[bsimonnet, 2014/05/15 19:23:40]

Done.

+      break;
+    }
+    metrics->push_back(sample.release());
+  }
+
+  result = ftruncate(fd.get(), 0);
+  if (result < 0) {
+    DPLOG(ERROR) << "truncate metrics log";
+  }
+  result = flock(fd.get(), LOCK_UN);
+  if (result < 0) {
+    DPLOG(ERROR) << "unlock metrics log";
+  }
+}
+
+}  // namespace metrics