Add guards to metric values; erase bad events/metrics from db

chrome/browser/performance_monitor/metric.cc

Index: chrome/browser/performance_monitor/metric.cc
diff --git a/chrome/browser/performance_monitor/metric.cc b/chrome/browser/performance_monitor/metric.cc
index 4d105f768c70d30b6e143b2bad73e2eb16a17f0c..388e587f1241166f2a44b4b432c79b1ae26beb53 100644
--- a/chrome/browser/performance_monitor/metric.cc
+++ b/chrome/browser/performance_monitor/metric.cc
@@ -4,21 +4,76 @@
 
 #include "chrome/browser/performance_monitor/metric.h"
 
+#include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 
 namespace performance_monitor {
 
+namespace {
+
+// These constants are designed to keep metrics reasonable. However, due to the
+// variety of system configurations which can run chrome, these values may not
+// catch *all* erroneous values. For instance, on a one-CPU machine, any CPU
+// usage > 100 is erroneous, but on a 16-CPU machine, it's perfectly normal.
+// These are "best-guesses" in order to weed out obviously-false values.
+const double kMinUndefined = 0.0;
+const double kMaxUndefined = 0.0;  // No undefined metric is valid.
+const double kMinCpuUsage = 0.0;
+const double kMaxCpuUsage = 100000.0;  // 100% on a 1000-CPU machine.
+const double kMinPrivateMemoryUsage = 0.0;
+const double kMaxPrivateMemoryUsage =
+    1024.0 * 1024.0 * 1024.0 * 1024.0;  // 1 TB
+const double kMinSharedMemoryUsage = 0.0;
+const double kMaxSharedMemoryUsage = 1024.0 * 1024.0 * 1024.0 * 1024.0;  // 1 TB

[Yoyo Zhou, 2012/09/10 23:52:44]

Can you use the constants from the unit-conversion changelist?

[Devlin, 2012/09/11 18:52:17]

Done. Moved the constants from the ui file to c/b/p_m/constants file, so that we
still only include in one direction (i.e., ui includes c/b/p_m, but not vice
versa). 

Is the unit conversion (or at least, the set of unit constants) something that
we should think about putting at a higher level (perhaps in base)? I know there
are other places in the codebase where some of these values are used, but it's
impractical to include a dozen different files for a dozen different
constants...

[Yoyo Zhou, 2012/09/13 00:09:20]

If there's demand for it.

+const double kMinStartupTime = 0.0;
+const double kMaxStartupTime = 1000000 * 60 * 15;  // 15 minutes

[Yoyo Zhou, 2012/09/10 23:52:44]

Be consistent about using foo or foo.0.

[Devlin, 2012/09/11 18:52:17]

Done.

+const double kMinTestStartupTime = 0.0;
+const double kMaxTestStartupTime = 1000000 * 60 * 15;  // 15 minutes
+const double kMinSessionRestoreTime = 0.0;
+const double kMaxSessionRestoreTime = 1000000 * 60 * 15;  // 15 minutes
+const double kMinPageLoadTime = 0.0;
+const double kMaxPageLoadTime = 1000000 * 60 * 15;  // 15 minutes
+const double kMinNetworkBytesRead = 0.0;
+// There really isn't a feasible "max" for network bytes read (even numbers in
+// high TB are potentially possible).
+const double kMaxNetworkBytesRead = static_cast<double>(kint64max);

[Yoyo Zhou, 2012/09/10 23:52:44]

This is kind of hackish. It'd be better to change the comparison in Validate so
that e.g. it passes if the max is 0 (but this is merely a better hack and still
must be documented).

[Devlin, 2012/09/11 18:52:17]

Done.

+
+struct MetricBound {
+  double min;
+  double max;
+};
+
+const MetricBound kMetricBounds[] = {

[Yoyo Zhou, 2012/09/10 23:52:44]

If we still had metric_details I would suggest that this goes there.  As it is,
having max/min information here is a little weird.

[Devlin, 2012/09/11 18:52:17]

I'm not sure I agree that this is weird. It seems to me that since this is used
by the metrics and is directly related to them (and is only used for this
function), that this is the logical place for it. However, if you think this
would go better somewhere else (perhaps as a function, ValidateMetric(), in
performance_monitor_util?), I would be happy to do so :)

[Yoyo Zhou, 2012/09/13 00:09:20]

Oops, I think I had a half-formed thought there. Feel free to ignore it.

+ { kMinUndefined, kMaxUndefined },
+ { kMinCpuUsage, kMaxCpuUsage },
+ { kMinPrivateMemoryUsage, kMaxPrivateMemoryUsage },
+ { kMinSharedMemoryUsage, kMaxSharedMemoryUsage },
+ { kMinStartupTime, kMaxStartupTime },
+ { kMinTestStartupTime, kMaxTestStartupTime },
+ { kMinSessionRestoreTime, kMaxSessionRestoreTime },
+ { kMinPageLoadTime, kMaxPageLoadTime },
+ { kMinNetworkBytesRead, kMaxNetworkBytesRead },
+};
+
+COMPILE_ASSERT(ARRAYSIZE_UNSAFE(kMetricBounds) == METRIC_NUMBER_OF_METRICS,
+               metric_bounds_size_doesnt_match_metric_count);
+
+}  // namespace
+
 Metric::Metric() {
   value = 0.0;
 }
 
-Metric::Metric(const base::Time& metric_time, const double metric_value)
-    : time(metric_time), value(metric_value) {
+Metric::Metric(MetricType metric_type,
+               const base::Time& metric_time,
+               const double metric_value)
+    : type(metric_type), time(metric_time), value(metric_value) {
 }
 
-Metric::Metric(const std::string& metric_time,
-               const std::string& metric_value) {
+Metric::Metric(MetricType metric_type,
+               const std::string& metric_time,
+               const std::string& metric_value) : type(metric_type) {
   int64 conversion = 0;
   base::StringToInt64(metric_time, &conversion);
   time = base::Time::FromInternalValue(conversion);
@@ -28,4 +83,13 @@ Metric::Metric(const std::string& metric_time,
 Metric::~Metric() {
 }
 
+bool Metric::Validate() const {
+  return type < METRIC_NUMBER_OF_METRICS &&
+      value < kMetricBounds[type].max && value > kMetricBounds[type].min;

[Yoyo Zhou, 2012/09/10 23:52:44]

So 0 is going to be an invalid value for all of these metrics?

[Devlin, 2012/09/11 18:52:17]

It's difficult to say for sure. The only time I think we could really have 0 for
one of the (current) metrics is if we had a really powerful 1-cpu machine that
had chrome idling, and it was close enough to 0 to be reported as such. Changed
it to be safe, though.

+}
+
+std::string Metric::ValueAsString() const {
+  return base::DoubleToString(value);
+}
+
 }  // namespace performance_monitor