Create a histogram serialization mechanism in components/metrics

base/metrics/chromeos_metrics.cc

Index: base/metrics/chromeos_metrics.cc
diff --git a/base/metrics/chromeos_metrics.cc b/base/metrics/chromeos_metrics.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dc4eb928bb14ca74b493ea33f94ac294328f9cf6
--- /dev/null
+++ b/base/metrics/chromeos_metrics.cc
@@ -0,0 +1,153 @@
+// Copyright (c) 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 <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/file.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include <list>
+#include <memory>
+
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/memory/scoped_vector.h"
+#include "base/metrics/chromeos_metrics.h"
+#include "base/metrics/crash_sample.h"
+#include "base/metrics/histogram_sample.h"
+#include "base/metrics/linearhistogram_sample.h"
+#include "base/metrics/metric_sample.h"
+#include "base/metrics/sparsehistogram_sample.h"
+#include "base/metrics/useraction_sample.h"
+
+namespace base {
+MetricSample* ChromeOSMetrics::ReadSample(int32 message_size, uint8* buffer) {

[achaulk, 2014/04/07 22:16:00]

You're doing an awful lot of reinterpret_casts in this code. You also don't seem
to actually need uint8 here, char* would be fine.

Maybe pass in std::string (which can hold arbitrary binary data since it is
sized)? Maybe use ReadFileToString & Tokenize to split your key-values?

Since none of the types seem to need a arbitrary-contents TLV system, you could
get away with newlines as separators too

+    // The buffer should now contain a pair of null-terminated strings.
+    uint8* p = reinterpret_cast<uint8*>(memchr(buffer, '\0', message_size));
+    uint8* q = NULL;
+    if (p != NULL) {
+      q = reinterpret_cast<uint8*>(
+          memchr(p + 1, '\0', message_size - (p + 1 - buffer)));
+    }
+    if (q == NULL) {
+      DLOG(ERROR) << "bad name-value pair for metrics";
+      return NULL;
+    }
+    char* name = reinterpret_cast<char*>(buffer);
+    char* value = reinterpret_cast<char*>(p + 1);
+    if (strcmp(name, "crash") == 0) {
+      return CrashSample::ReadCrash(value);
+    } else if (strcmp(name, "histogram") == 0) {
+      return HistogramSample::ReadHistogram(value);
+    } else if (strcmp(name, "linearhistogram") == 0) {
+      return LinearHistogramSample::ReadLinearHistogram(value);
+    } else if (strcmp(name, "sparsehistogram") == 0) {
+      return SparseHistogramSample::ReadSparseHistogram(value);
+    } else if (strcmp(name, "useraction") == 0) {
+      return new UserActionSample(value);
+    } else {
+      DLOG(ERROR) << "invalid event type: " << name << value << message_size;
+    }
+    return NULL;
+}
+
+int32_t ChromeOSMetrics::FormatSample(int32_t buffer_size, char* buffer,

[achaulk, 2014/04/07 22:16:00]

This doesn't seem very useful since it just calls vsnprintf and you still need
to check the return

+                                            const char* format, ...) {
+  int32_t message_length;
+
+  // Format the non-LENGTH contents in the buffer by leaving space for
+  // LENGTH at the start of the buffer.
+  va_list args;
+  va_start(args, format);
+  message_length = vsnprintf(buffer, buffer_size,
+                             format, args);
+  va_end(args);
+
+  if (message_length < 0) {

[achaulk, 2014/04/07 22:16:00]

I don't think negative values are possible with the s variants since they refer
to actual output errors

+    return -1;
+  }
+
+  // +1 to account for the trailing \0.
+  message_length += 1;
+  if (message_length > buffer_size) {
+    return -1;
+  }
+
+  return message_length;
+}
+
+int32_t ChromeOSMetrics::WriteChromeMessage(MetricSample* sample,
+                                           int32_t buffer_size, char* buffer) {
+  int32_t message_length;
+  size_t len_size = sizeof(message_length);
+
+  // Format the non-LENGTH contents in the buffer by leaving space for
+  // LENGTH at the start of the buffer.
+  message_length = sample->Write(buffer_size-len_size, &buffer[len_size]);

[achaulk, 2014/04/07 22:16:00]

Should have the write functions return a std::string so you don't have to mess
around with buffer sizes

+
+  if (message_length < 0) {
+    return -1;
+  }
+
+  // +1 to account for the trailing \0.
+  message_length += len_size;
+  if (message_length > buffer_size) {
+    return -1;
+  }
+
+  // Prepend LENGTH to the message.
+  memcpy(buffer, &message_length, len_size);
+  return message_length;
+}
+
+bool ChromeOSMetrics::WriteMetricToFile(MetricSample* sample,
+                                        const std::string& filename) {
+  File file = File(FilePath(filename), File::FLAG_OPEN_ALWAYS
+                                        | File::FLAG_APPEND);
+  File::Error err = file.Lock();
+  if (err != File::FILE_OK) {
+    DLOG(ERROR) << "could not lock the file";
+    return false;
+  }
+
+  char buffer[kMessageMaxLength];
+  int32_t length =
+      ChromeOSMetrics::WriteChromeMessage(sample, kMessageMaxLength, buffer);
+  file.WriteAtCurrentPos(buffer, length);
+
+  return true;
+}
+
+void ChromeOSMetrics::ReadMetricsFromFile(const std::string& filename,
+    ScopedVector<MetricSample>* metrics) {
+  File file = File(FilePath(filename), File::FLAG_OPEN |
+                                       File::FLAG_READ | File::FLAG_WRITE);

[achaulk, 2014/04/07 22:16:00]

Why have write here? If you want to truncate, wouldn't it make sense to do that
just before writing? And most people don't expect a ReadX function to really be
ReadXAndTruncate

[bsimonnet, 2014/04/08 23:00:09]

The way it works it that the reading and writing would be done by two separate
processes that dont communicate so we need to read and truncate while locking
the file (and keep that duration as small as possible).
My goal was to keep only one copy of each metric at all time (in memory or on
the disk) to simplify the logic.

+  File::Error err = file.Lock();
+  if (err != File::FILE_OK) {
+    DLOG(ERROR) << "could not lock the file";
+    return;
+  }
+  for (;;) {
+    int32 message_length;
+    int read = file.ReadAtCurrentPos(reinterpret_cast<char*>(&message_length),

[achaulk, 2014/04/07 22:16:00]

Possible endian issues? Do these files ever leave the device?

[bsimonnet, 2014/04/08 23:00:09]

This file will always stay on the device. It is only used to communicate with
the running chrome instance.

+                                     sizeof(message_length));
+    if (read != sizeof(message_length)) {
+      break;
+    }
+    message_length -= sizeof(message_length);
+    char serialized_sample[message_length];

[achaulk, 2014/04/07 22:16:00]

I think we are avoiding this GCC extension

+    read = file.ReadAtCurrentPos(serialized_sample, message_length);
+    if (read != message_length) {
+      DLOG(ERROR) << "could not read message" << read;
+      break;
+    }
+    metrics->push_back(ReadSample(message_length,
+                                 reinterpret_cast<uint8*>(serialized_sample)));
+  }
+  file.SetLength(0);
+}
+}  // namespace base