metrics cleanup and fixes.
- value is int now.
- add seconds to milliseconds option to metrics_client and use it
- chmod chronos/root fix
- style fixes
Review URL: http://codereview.chromium.org/1649007
diff --git a/metrics/metrics_client.cc b/metrics/metrics_client.cc
index cdea012..17f933c 100644
--- a/metrics/metrics_client.cc
+++ b/metrics/metrics_client.cc
@@ -2,22 +2,16 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include <errno.h>
-#include <sys/file.h>
-#include <string.h>
-#include <stdio.h>
-
+#include <cstdio>
#include <cstdlib>
-#include <iostream>
#include "metrics_library.h"
-using namespace std;
-
// Usage: metrics_client [-ab] metric_name metric_value
int main(int argc, char** argv) {
bool send_to_autotest = false;
bool send_to_chrome = true;
+ bool secs_to_msecs = false;
int metric_name_index = 1;
int metric_value_index = 2;
bool print_usage = false;
@@ -25,7 +19,7 @@
if (argc >= 3) {
// Parse arguments
int flag;
- while ((flag = getopt(argc, argv, "ab")) != -1) {
+ while ((flag = getopt(argc, argv, "abt")) != -1) {
switch (flag) {
case 'a':
send_to_autotest = true;
@@ -35,6 +29,9 @@
send_to_chrome = true;
send_to_autotest = true;
break;
+ case 't':
+ secs_to_msecs = true;
+ break;
default:
print_usage = true;
break;
@@ -52,22 +49,31 @@
}
if (print_usage) {
- cerr << "Usage: metrics_client [-ab] name value" << endl;
- cerr << endl;
- cerr << " default: send metric to chrome only" << endl;
- cerr << " -a: send metric to autotest only" << endl;
- cerr << " -b: send metric to both chrome and autotest" << endl;
+ fprintf(stderr,
+ "Usage: metrics_client [-abt] name value\n"
+ "\n"
+ " default: send metric with integer value to chrome only\n"
+ " -a: send metric to autotest only\n"
+ " -b: send metric to both chrome and autotest\n"
+ " -t: convert value from float seconds to int milliseconds\n");
return 1;
}
+ const char* name = argv[metric_name_index];
+ int value;
+ if (secs_to_msecs) {
+ float secs = strtof(argv[metric_value_index], NULL);
+ value = static_cast<int>(secs * 1000.0f);
+ } else {
+ value = atoi(argv[metric_value_index]);
+ }
+
// Send metrics
if (send_to_autotest) {
- MetricsLibrary::SendToAutotest(argv[metric_name_index],
- argv[metric_value_index]);
+ MetricsLibrary::SendToAutotest(name, value);
}
if (send_to_chrome) {
- MetricsLibrary::SendToChrome(argv[metric_name_index],
- argv[metric_value_index]);
+ MetricsLibrary::SendToChrome(name, value);
}
return 0;
}
diff --git a/metrics/metrics_daemon.cc b/metrics/metrics_daemon.cc
index a924b8a..9bb9c20 100644
--- a/metrics/metrics_daemon.cc
+++ b/metrics/metrics_daemon.cc
@@ -113,12 +113,10 @@
if (diff.tv_sec >= INT_MAX / 1000) {
diff_ms = INT_MAX;
}
- char buffer[100];
- snprintf(buffer, sizeof(buffer), "%d", diff_ms);
if (testing_) {
- TestPublishMetric(network_states_[old_id].stat_name, buffer);
+ TestPublishMetric(network_states_[old_id].stat_name, diff_ms);
} else {
- ChromePublishMetric(network_states_[old_id].stat_name, buffer);
+ ChromePublishMetric(network_states_[old_id].stat_name, diff_ms);
}
}
network_state_id_ = new_id;
@@ -135,10 +133,10 @@
return static_cast<NetworkStateId>(-1);
}
-void MetricsDaemon::ChromePublishMetric(const char* name, const char* value) {
+void MetricsDaemon::ChromePublishMetric(const char* name, int value) {
MetricsLibrary::SendToChrome(name, value);
}
-void MetricsDaemon::TestPublishMetric(const char* name, const char* value) {
+void MetricsDaemon::TestPublishMetric(const char* name, int value) {
LOG(INFO) << "received metric: " << name << " " << value;
}
diff --git a/metrics/metrics_daemon.h b/metrics/metrics_daemon.h
index 2ac1ea0..dc097b5 100644
--- a/metrics/metrics_daemon.h
+++ b/metrics/metrics_daemon.h
@@ -64,10 +64,10 @@
NetworkStateId GetNetworkStateId(const char* state_name);
// Sends a stat to Chrome for transport to UMA.
- void ChromePublishMetric(const char* name, const char* value);
+ void ChromePublishMetric(const char* name, int value);
// Prints a stat for testing.
- void TestPublishMetric(const char* name, const char* value);
+ void TestPublishMetric(const char* name, int value);
#if 0
// Fetches a name-value hash table from DBus.
diff --git a/metrics/metrics_library.cc b/metrics/metrics_library.cc
index 99ad616..f482145 100644
--- a/metrics/metrics_library.cc
+++ b/metrics/metrics_library.cc
@@ -13,21 +13,23 @@
#include <errno.h>
#include <sys/file.h>
-#include <string.h>
-#include <stdio.h>
+
+#include <cstdarg>
+#include <cstdio>
+#include <cstring>
#define READ_WRITE_ALL_FILE_FLAGS \
(S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH)
static const char kAutotestPath[] = "/tmp/.chromeos-metrics-autotest";
static const char kChromePath[] = "/tmp/.chromeos-metrics";
-static const int kBufferSize = 4096;
+static const int32_t kBufferSize = 1024;
using namespace std;
// TODO(sosa@chromium.org) - use Chromium logger instead of stderr
-void MetricsLibrary::PrintError(const char *message, const char *file,
- int code) {
+static void PrintError(const char *message, const char *file,
+ int code) {
const char *kProgramName = "metrics_library";
if (code == 0) {
fprintf(stderr, "%s: %s\n", kProgramName, message);
@@ -40,61 +42,105 @@
}
}
-void MetricsLibrary::SendToAutotest(string name, string value) {
- FILE *autotest_file = fopen(kAutotestPath, "a+");
- if (autotest_file == NULL) {
- PrintError("fopen", kAutotestPath, errno);
- return;
- }
-
- fprintf(autotest_file, "%s=%s\n", name.c_str(), value.c_str());
- fclose(autotest_file);
-}
-
-void MetricsLibrary::SendToChrome(string name, string value) {
+// Sends message of size length to Chrome and returns true on success.
+static bool SendMessageToChrome(int32_t length, const char *message) {
int chrome_fd = open(kChromePath,
O_WRONLY | O_APPEND | O_CREAT,
READ_WRITE_ALL_FILE_FLAGS);
- // If we failed to open it, return
+ // If we failed to open it, return.
if (chrome_fd < 0) {
PrintError("open", kChromePath, errno);
- return;
+ return false;
}
- // Need to chmod because open flags are anded with umask.
- if (fchmod(chrome_fd, READ_WRITE_ALL_FILE_FLAGS) < 0) {
- PrintError("fchmod", kChromePath, errno);
- close(chrome_fd);
- return;
- }
+ // Need to chmod because open flags are anded with umask. Ignore the
+ // exit code -- a chronos process may fail chmoding because the file
+ // has been created by a root process but that should be OK.
+ fchmod(chrome_fd, READ_WRITE_ALL_FILE_FLAGS);
- // Grab an exclusive lock to protect Chrome from truncating underneath us
+ // Grab an exclusive lock to protect Chrome from truncating
+ // underneath us. Keep the file locked as briefly as possible.
if (flock(chrome_fd, LOCK_EX) < 0) {
PrintError("flock", kChromePath, errno);
close(chrome_fd);
- return;
+ return false;
}
- // Message format is: LENGTH (binary), NAME, VALUE
- char message[kBufferSize];
- char *curr_ptr = message;
- int32_t message_length =
- name.length() + value.length() + 2 + sizeof(message_length);
- if (message_length > static_cast<int32_t>(sizeof(message)))
- PrintError("name/value too long", NULL, 0);
-
- // Make sure buffer is blanked
- memset(message, 0, sizeof(message));
- memcpy(curr_ptr, &message_length, sizeof(message_length));
- curr_ptr += sizeof(message_length);
- strncpy(curr_ptr, name.c_str(), name.length());
- curr_ptr += name.length() + 1;
- strncpy(curr_ptr, value.c_str(), value.length());
- if (write(chrome_fd, message, message_length) != message_length)
+ bool success = true;
+ if (write(chrome_fd, message, length) != length) {
PrintError("write", kChromePath, errno);
+ success = false;
+ }
- // Release the file lock and close file
- if (flock(chrome_fd, LOCK_UN) < 0)
+ // Release the file lock and close file.
+ if (flock(chrome_fd, LOCK_UN) < 0) {
PrintError("unlock", kChromePath, errno);
+ success = false;
+ }
close(chrome_fd);
+ return success;
+}
+
+// Formats a name/value message for Chrome in buffer and returns the
+// length of the message or a negative value on error.
+//
+// Message format is: | LENGTH(binary) | NAME | \0 | VALUE | \0 |
+//
+// The arbitrary format argument covers the non-LENGTH portion of the
+// message. The caller is responsible to store the \0 character
+// between NAME and VALUE (e.g. "%s%c%d", name, '\0', value).
+static int32_t FormatChromeMessage(int32_t buffer_size, char *buffer,
+ const char *format, ...) {
+ 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.
+ va_list args;
+ va_start(args, format);
+ message_length = vsnprintf(&buffer[len_size], buffer_size - len_size,
+ format, args);
+ va_end(args);
+
+ if (message_length < 0) {
+ PrintError("chrome message format error", NULL, 0);
+ return -1;
+ }
+
+ // +1 to account for the trailing \0.
+ message_length += len_size + 1;
+ if (message_length > buffer_size) {
+ PrintError("chrome message too long", NULL, 0);
+ return -1;
+ }
+
+ // Prepend LENGTH to the message.
+ memcpy(buffer, &message_length, len_size);
+ return message_length;
+}
+
+bool MetricsLibrary::SendToAutotest(string name, int value) {
+ FILE *autotest_file = fopen(kAutotestPath, "a+");
+ if (autotest_file == NULL) {
+ PrintError("fopen", kAutotestPath, errno);
+ return false;
+ }
+
+ fprintf(autotest_file, "%s=%d\n", name.c_str(), value);
+ fclose(autotest_file);
+ return true;
+}
+
+bool MetricsLibrary::SendToChrome(string name, int value) {
+ // Format the message.
+ char message[kBufferSize];
+ int32_t message_length =
+ FormatChromeMessage(kBufferSize, message,
+ "%s%c%d", name.c_str(), '\0', value);
+
+ if (message_length < 0)
+ return false;
+
+ // Send the message.
+ return SendMessageToChrome(message_length, message);
}
diff --git a/metrics/metrics_library.h b/metrics/metrics_library.h
index f1268c9..ebc972c 100644
--- a/metrics/metrics_library.h
+++ b/metrics/metrics_library.h
@@ -12,7 +12,6 @@
#ifndef METRICS_LIBRARY_H_
#define METRICS_LIBRARY_H_
-#include <stdio.h>
#include <string>
// TODO(sosa@chromium.org): Add testing for send methods
@@ -20,14 +19,10 @@
// Library used to send metrics both Autotest and Chrome
class MetricsLibrary {
public:
- // Sends histogram data to Chrome.
- static void SendToChrome(std::string name, std::string value);
- // Sends to Autotest.
- static void SendToAutotest(std::string name, std::string value);
-
- private:
- // Prints message to stderr
- static void PrintError(const char *message, const char *file, int code);
+ // Sends histogram data to Chrome and returns true on success.
+ static bool SendToChrome(std::string name, int value);
+ // Sends to Autotest and returns true on success.
+ static bool SendToAutotest(std::string name, int value);
};
#endif /* METRICS_LIBRARY_H_ */