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

chrome/browser/performance_monitor/database_unittest.cc

Index: chrome/browser/performance_monitor/database_unittest.cc
diff --git a/chrome/browser/performance_monitor/database_unittest.cc b/chrome/browser/performance_monitor/database_unittest.cc
index 4fef765c9ddde1f19e95f9bde17f7466049ec4b9..79c2add16f63c33cbc7153b9cfacc98bb8af2b02 100644
--- a/chrome/browser/performance_monitor/database_unittest.cc
+++ b/chrome/browser/performance_monitor/database_unittest.cc
@@ -12,15 +12,72 @@
 #include "base/scoped_temp_dir.h"
 #include "base/time.h"
 #include "chrome/browser/performance_monitor/database.h"
+#include "chrome/browser/performance_monitor/key_builder.h"
 #include "chrome/browser/performance_monitor/metric.h"
 #include "chrome/browser/performance_monitor/performance_monitor_util.h"
 #include "chrome/common/extensions/extension.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
+#include "third_party/leveldatabase/src/include/leveldb/iterator.h"
 
 using extensions::Extension;
 
 namespace performance_monitor {
 
+// A class which is friended by Database, in order to hold the private methods
+// and avoid friending all the different test classes.
+class DatabaseTestHelper {
+ public:
+  explicit DatabaseTestHelper(Database* database) : database_(database) { };
+  ~DatabaseTestHelper() { };
+
+  bool Close() { return database_->Close(); }
+
+  // Override the check for a metric's validity and insert it in the database.
+  // Note: This does not do extraneous updates, like updating the recent_db or
+  // the max_value_map.
+  bool AddInvalidMetric(std::string activity, Metric metric) {
+    std::string metric_key =
+        database_->key_builder_->CreateMetricKey(database_->clock_->GetTime(),
+                                                 metric.type,
+                                                 activity);
+    leveldb::Status status =
+        database_->metric_db_->Put(database_->write_options_,
+                                   metric_key,
+                                   metric.ValueAsString());
+    return status.ok();
+  }
+
+  // Writes an invalid event to the database; since events are stored as JSON
+  // strings, this is equivalent to writing a garbage string.
+  bool AddInvalidEvent(base::Time time, EventType type) {
+    std::string key = database_->key_builder_->CreateEventKey(time, type);
+    leveldb::Status status =
+        database_->event_db_->Put(database_->write_options_, key, "fake_event");
+    return status.ok();
+  }
+
+  size_t GetNumberOfMetricEntries() {
+    return GetNumberOfEntries(database_->metric_db_.get());
+  }
+  size_t GetNumberOfEventEntries() {
+    return GetNumberOfEntries(database_->event_db_.get());
+  }
+
+ private:
+  // Returns the number of entries in a given database.
+  size_t GetNumberOfEntries(leveldb::DB* level_db) {
+    size_t number_of_entries = 0;
+    scoped_ptr<leveldb::Iterator> iter(
+        level_db->NewIterator(database_->read_options_));
+    for (iter->SeekToFirst(); iter->Valid(); iter->Next())
+      ++number_of_entries;
+    return number_of_entries;
+  }
+
+  Database* database_;
+};
+
 // A clock that increments every access. Great for testing.
 class TestingClock : public Database::Clock {
  public:
@@ -107,14 +164,18 @@ class PerformanceMonitorDatabaseMetricTest : public ::testing::Test {
   }
 
   void PopulateDB() {
-    db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE,
-                   std::string("50.5"));
-    db_->AddMetric(activity_, METRIC_CPU_USAGE, std::string("13.1"));
-
-    db_->AddMetric(kProcessChromeAggregate, METRIC_PRIVATE_MEMORY_USAGE,
-                   std::string("1000000"));
-    db_->AddMetric(activity_, METRIC_PRIVATE_MEMORY_USAGE,
-                   std::string("3000000"));
+    db_->AddMetric(kProcessChromeAggregate,
+                   Metric(METRIC_CPU_USAGE, clock_->GetTime(), 50.5));
+    db_->AddMetric(activity_,
+                   Metric(METRIC_CPU_USAGE, clock_->GetTime(), 13.1));
+    db_->AddMetric(kProcessChromeAggregate,
+                   Metric(METRIC_PRIVATE_MEMORY_USAGE,
+                          clock_->GetTime(),
+                          1000000.0));
+    db_->AddMetric(activity_,
+                   Metric(METRIC_PRIVATE_MEMORY_USAGE,
+                          clock_->GetTime(),
+                          3000000.0));
   }
 
   scoped_ptr<Database> db_;
@@ -129,7 +190,8 @@ TEST(PerformanceMonitorDatabaseSetupTest, OpenClose) {
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   scoped_ptr<Database> db = Database::Create(temp_dir.path());
   ASSERT_TRUE(db.get());
-  ASSERT_TRUE(db->Close());
+  DatabaseTestHelper helper(db.get());
+  ASSERT_TRUE(helper.Close());
 }
 
 TEST(PerformanceMonitorDatabaseSetupTest, ActiveInterval) {
@@ -141,17 +203,19 @@ TEST(PerformanceMonitorDatabaseSetupTest, ActiveInterval) {
 
   scoped_ptr<Database> db_1 = Database::Create(temp_dir.path());
   db_1->set_clock(scoped_ptr<Database::Clock>(clock_1));
+  DatabaseTestHelper helper1(db_1.get());
   db_1->AddStateValue("test", "test");
   ASSERT_TRUE(db_1.get());
-  ASSERT_TRUE(db_1->Close());
+  ASSERT_TRUE(helper1.Close());
 
   TestingClock* clock_2 = new TestingClock(*clock_1);
   base::Time mid_time = clock_2->GetTime();
   scoped_ptr<Database> db_2 = Database::Create(temp_dir.path());
   db_2->set_clock(scoped_ptr<Database::Clock>(clock_2));
+  DatabaseTestHelper helper2(db_2.get());
   db_2->AddStateValue("test", "test");
   ASSERT_TRUE(db_2.get());
-  ASSERT_TRUE(db_2->Close());
+  ASSERT_TRUE(helper2.Close());
 
   TestingClock* clock_3 = new TestingClock(*clock_2);
   base::Time end_time = clock_3->GetTime();
@@ -225,13 +289,15 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetMaxMetric) {
   EXPECT_EQ(1000000,
             db_->GetMaxStatsForActivityAndMetric(METRIC_PRIVATE_MEMORY_USAGE));
 
-  db_->AddMetric(kProcessChromeAggregate, METRIC_PRIVATE_MEMORY_USAGE,
-                 std::string("99"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_PRIVATE_MEMORY_USAGE, clock_->GetTime(), 99.0));
   EXPECT_EQ(1000000,
             db_->GetMaxStatsForActivityAndMetric(METRIC_PRIVATE_MEMORY_USAGE));
 
-  db_->AddMetric(kProcessChromeAggregate, METRIC_PRIVATE_MEMORY_USAGE,
-                 std::string("6000000"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_PRIVATE_MEMORY_USAGE,
+                        clock_->GetTime(),
+                        6000000.0));
   EXPECT_EQ(6000000,
             db_->GetMaxStatsForActivityAndMetric(METRIC_PRIVATE_MEMORY_USAGE));
 }
@@ -260,6 +326,23 @@ TEST_F(PerformanceMonitorDatabaseEventTest, GetInstallEvents) {
   EXPECT_TRUE(events[1]->data()->Equals(install_event_2_->data()));
 }
 
+TEST_F(PerformanceMonitorDatabaseEventTest, InvalidEvents) {
+  DatabaseTestHelper helper(db_.get());
+
+  // Insert an invalid event into the database; verify it is inserted.
+  size_t original_number_of_entries = helper.GetNumberOfEventEntries();
+  ASSERT_TRUE(helper.AddInvalidEvent(
+      clock_->GetTime(), EVENT_EXTENSION_INSTALL));
+  ASSERT_EQ(original_number_of_entries + 1u, helper.GetNumberOfEventEntries());
+
+  // Should not retrieve the invalid event.
+  Database::EventVector events = db_->GetEvents();
+  ASSERT_EQ(original_number_of_entries, events.size());
+
+  // Invalid event should have been deleted.
+  ASSERT_EQ(original_number_of_entries, helper.GetNumberOfEventEntries());
+}
+
 TEST_F(PerformanceMonitorDatabaseEventTest, GetUnusedEventType) {
   Database::EventVector events = db_->GetEvents(EVENT_EXTENSION_DISABLE);
   ASSERT_TRUE(events.empty());
@@ -343,7 +426,8 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetStatsForActivityAndMetric) {
   ASSERT_EQ(1u, stats.size());
   EXPECT_EQ(13.1, stats[0].value);
   base::Time before = clock_->GetTime();
-  db_->AddMetric(activity_, METRIC_CPU_USAGE, std::string("18"));
+  db_->AddMetric(activity_,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 18.0));
   stats = db_->GetStatsForActivityAndMetric(activity_, METRIC_CPU_USAGE,
                                             before, clock_->GetTime());
   ASSERT_EQ(1u, stats.size());
@@ -375,7 +459,8 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetStatsForMetricByActivity) {
       METRIC_CPU_USAGE, clock_->GetTime(), clock_->GetTime());
   EXPECT_EQ(0u, stats_map.size());
   base::Time before = clock_->GetTime();
-  db_->AddMetric(activity_, METRIC_CPU_USAGE, std::string("18"));
+  db_->AddMetric(activity_,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 18.0));
   stats_map = db_->GetStatsForMetricByActivity(METRIC_CPU_USAGE, before,
                                                clock_->GetTime());
   ASSERT_EQ(1u, stats_map.size());
@@ -384,9 +469,38 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetStatsForMetricByActivity) {
   EXPECT_EQ(18, stats->at(0).value);
 }
 
+TEST_F(PerformanceMonitorDatabaseMetricTest, InvalidMetrics) {
+  DatabaseTestHelper helper(db_.get());
+  Metric invalid_metric(METRIC_CPU_USAGE, clock_->GetTime(), -5.0);
+  ASSERT_FALSE(invalid_metric.Validate());
+
+  // Check that the database normally refuses to insert bad metrics.
+  EXPECT_FALSE(db_->AddMetric(invalid_metric));
+
+  // Verify that it was not inserted into the database.
+  Database::MetricVector stats = db_->GetStatsForActivityAndMetric(
+      activity_, METRIC_CPU_USAGE, base::Time(), clock_->GetTime());
+  ASSERT_EQ(1u, stats.size());

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

Should this 1u be the original_number_of_entries?

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

Done.

+
+  // Forcefully insert it into the database.
+  size_t original_number_of_entries = helper.GetNumberOfMetricEntries();
+  ASSERT_TRUE(helper.AddInvalidMetric(activity_, invalid_metric));
+  ASSERT_EQ(original_number_of_entries + 1u, helper.GetNumberOfMetricEntries());
+
+  // Try to retrieve it; should only get one result.
+  stats = db_->GetStatsForActivityAndMetric(
+      activity_, METRIC_CPU_USAGE, base::Time(), clock_->GetTime());
+  ASSERT_EQ(1u, stats.size());
+
+  // Entry should have been deleted in the database.
+  ASSERT_EQ(original_number_of_entries, helper.GetNumberOfMetricEntries());
+}
+
 TEST_F(PerformanceMonitorDatabaseMetricTest, GetFullRange) {
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("3.4"));
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("21"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 3.4));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 21.0));
   Database::MetricVector stats =
       db_->GetStatsForActivityAndMetric(METRIC_CPU_USAGE);
   ASSERT_EQ(3u, stats.size());
@@ -397,10 +511,13 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetFullRange) {
 
 TEST_F(PerformanceMonitorDatabaseMetricTest, GetRange) {
   base::Time start = clock_->GetTime();
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("3"));
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("9"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 3.0));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 9.0));
   base::Time end = clock_->GetTime();
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("21"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 21.0));
   Database::MetricVector stats =
       db_->GetStatsForActivityAndMetric(METRIC_CPU_USAGE, start, end);
   ASSERT_EQ(2u, stats.size());