Performance Monitor Database

chrome/browser/performance_monitor/database.h

+// Copyright (c) 2012 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.
+
+#ifndef CHROME_BROWSER_PERFORMANCE_MONITOR_DATABASE_H_
+#define CHROME_BROWSER_PERFORMANCE_MONITOR_DATABASE_H_
+#pragma once
+
+#include <vector>
+#include <map>

[Aaron Boodman, 2012/06/11 22:52:13]

I think this is no longer needed.

[eaugusti, 2012/06/12 00:12:57]

Done.

+#include <string>
+
+#include "base/file_path.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/linked_ptr.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "base/time.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
+
+namespace performance_monitor {
+
+struct TimeRange {
+  TimeRange();
+  TimeRange(base::Time start_time, base::Time end_time);
+  ~TimeRange();
+
+  base::Time start;
+  base::Time end;
+};
+
+// The class supporting all performance monitor storage. This class wraps
+// multiple leveldb::DB objects. Most methods must be called on a background
+// thread and are named to reflect that. Callers should use
+// BrowserThread::PostBlockingPoolSequencedTask using
+// performance_monitor::kDBSequenceToken as the sequence token.
+//
+// Different schemas are used for the different leveldb::DB's based off of the
+// structure of the data and the common ways that it will need to be accessed.
+// The following specifies the schema of each type of leveldb::DB. Delimiters
+// are denoted with a '-'.
+//
+// State DB:
+// Stores information about the configuration or 'state' of the browser. Things
+// like browser version go in here.
+// Key: Unique Identifier
+// Value: State Value
+//
+// Active Interval DB:
+// Stores information about when there is data in the database. When the
+// database is constructed, the time is noted as the start of the active
+// interval. Then, every write operation the current time is marked as the end
+// of the current active interval. If the database has no write operations for
+// a certian amount of time, then the database is considered inactive for that
+// time period and a new start time is noted. Having the key be the beginning
+// of the active interval allows for efficient upserts to the current active
+// interval. If the end of the active interval was in the key, then every update
+// to the active interval would have to remove a key and insert a new one.
+// Key: Beginning of ActiveInterval
+// Value: End of ActiveInterval
+//
+// Event DB:
+// Stores all events. A time and type is enough to uniquely identify an event.
+// Using the time that the event took place as the beginning of the key allows
+// us to efficiently answer the question: "What are all the events that took
+// place in this time range?".
+// Key: Time - Type
+// Value: Event in JSON
+//
+// Recent DB:
+// Stores the most recent metric statistics to go into the database. This
+// database becomes useful when it is necessary to find all the active metrics
+// within a timerange. Without it, all the metric databases would need to be
+// searched to see if that metric is active.
+// Key: Time - Metric - Activity
+// Value: Statistic
+//
+// Metric DB:
+// Stores the statistics for different metrics. Having the activity before the
+// time in the key esures that all statistics for a specific acticity are
+// spatially local.
+// Key: Metric - Activity - Time
+// Value: Statistic
+class Database : public base::RefCountedThreadSafe<Database> {
+ public:
+  // The class that the database will use to infer time. Abstracting out the
+  // time mechanism allows for easy testing and mock data insetion.
+  class Clock {
+   public:
+    Clock() {}
+    virtual ~Clock() {}
+    virtual base::Time GetTime() = 0;
+  };
+
+  // A "state" value is anything that can only have one value at a time, and
+  // usually describes the state of the browser eg. version.
+  bool AddStateValueOnBackgroundThread(const std::string& key,

[Aaron Boodman, 2012/06/11 22:52:13]

It looks to me like this class is meant to be used entirely on background
threads. In that case, there is no need to label every method
'OnBackgroundThread'. Sorry for leading you down that path ... I only meant it
to apply to classes that are used across threads.

[eaugusti, 2012/06/12 00:12:57]

Done.

+                                       const std::string& value);
+
+  std::string GetStateValueOnBackgroundThread(const std::string& key);
+
+  // Erase everything in the database. Warning - No undo!
+  void ClearOnBackgroundThread();
+
+  // Returns the times for which there is data in the database.
+  std::vector<TimeRange> GetActiveIntervalOnBackgroundThread(
+      const base::Time& start, const base::Time& end);
+
+  FilePath path() const { return path_; }
+
+  void set_clock(linked_ptr<Clock> clock) {
+    clock_ = clock;
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<Database>;
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest,
+                           OpenCloseDefaultTest);
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest,
+                           OpenCloseAlternatePathTest);
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest,
+                           ActiveIntervalTest);
+
+  // By default, the database uses a clock that simply returns the current time.
+  class SystemClock : public Clock {
+   public:
+    SystemClock() {}
+    virtual ~SystemClock() {}
+    virtual base::Time GetTime() OVERRIDE;
+  };
+
+  Database(const FilePath& path, linked_ptr<Clock> clock);
+  virtual ~Database();
+
+  static scoped_refptr<Database> InitOnBackgroundThread(
+      FilePath path, linked_ptr<Clock> clock);

[Aaron Boodman, 2012/06/11 22:52:13]

Nit: I think it is cleaner to just let the tests use the setter. No need for a
constructor that takes a clock.

[eaugusti, 2012/06/12 00:12:57]

Done.

+
+  static scoped_refptr<Database> InitOnBackgroundThread(FilePath path) {

[Aaron Boodman, 2012/06/11 22:52:13]

I think this should become a public static factory function, like:

static scoped_refptr<Database> Create(const FilePath& path) {
  ...
}

There's no need to try and lock down clients to just the ones that we envision
at this moment. The class stands alone.

Also, I would probably say that the client should always pass a path. Let the
normal client pass in the typical path.

[eaugusti, 2012/06/12 00:12:57]

Done.

+    return InitOnBackgroundThread(path, linked_ptr<Clock>(new SystemClock()));
+  }
+
+  static scoped_refptr<Database> InitOnBackgroundThread() {
+    return InitOnBackgroundThread(FilePath(),
+                                  linked_ptr<Clock>(new SystemClock()));
+  }
+
+  bool CloseOnBackgroundThread();
+
+  // Mark the database as being active for the current time.
+  void UpdateActiveIntervalOnBackgroundThread();
+
+  // The directory where all the databases will reside.
+  FilePath path_;
+
+  // The key for the beginning of the active interval.
+  std::string start_time_key_;
+
+  // The last time the database had a transaction.
+  base::Time last_update_time_;
+
+  linked_ptr<Clock> clock_;

[Aaron Boodman, 2012/06/11 22:52:13]

I don't understand why this is not scoped_ptr.

[eaugusti, 2012/06/12 00:12:57]

When testing, the tests want direct access to the clock at any time. Like if
they are populating with some very precise data or need to read the clock.

[Aaron Boodman, 2012/06/12 03:57:59]

That's fine, you can just have the Database take ownership of the Clock, like
this:

class Database {
  ...
  set_clock(scoped_ptr<Clock> clock) {
    clock_ = clock.Pass();
  }
  ...
  scoped_ptr<Clock> clock_;
}

unittest:
scoped_refptr<Database> db = Database::Create(...);
TestClock* clock = new TestClock();
db->set_clock(scoped_ptr<Clock>(clock));
clock->DoStuff(42);

+
+  linked_ptr<leveldb::DB> recent_db_;

[Aaron Boodman, 2012/06/11 22:52:13]

I don't understand why these databases are linked_ptr. Why not scoped_ptr, or
even just normal members? Are you ever going to access these databases from any
place other than this class on the background thread?

[eaugusti, 2012/06/12 00:12:57]

You are right, they should be linked_ptr.

+
+  linked_ptr<leveldb::DB> state_db_;
+
+  linked_ptr<leveldb::DB> active_interval_db_;
+
+  linked_ptr<leveldb::DB> metric_db_;
+
+  leveldb::ReadOptions read_options_;
+  leveldb::WriteOptions write_options_;
+
+  DISALLOW_COPY_AND_ASSIGN(Database);
+};
+}  // namespace performance_monitor
+
+#endif  // CHROME_BROWSER_PERFORMANCE_MONITOR_DATABASE_H_