add GetStatsHistory to retrieve stats snapshots #4748

miasantreble · 2018-12-05T01:02:17Z

This PR adds public GetStatsHistory API to retrieve stats history in the form of an std map. The key of the map is the timestamp in microseconds when the stats snapshot is taken, the value is another std map from stats name to stats value (stored in std string). Two DBOptions are introduced: stats_persist_period_sec (default 10 minutes) controls the intervals between two snapshots are taken; max_stats_history_count (default 10) controls the max number of history snapshots to keep in memory. RocksDB will stop collecting stats snapshots if stats_persist_period_sec is set to 0.

(This PR is the in-memory part of #4535)

siying · 2018-12-05T22:22:22Z

Where is the public API? I don't see a change to db.h

siying · 2018-12-05T22:25:15Z

db/db_impl.cc

+DBImpl::FindStatsBetween(uint64_t start_time, uint64_t end_time) {
+  std::map<uint64_t, std::map<std::string, std::string> > stats_history;
+  // first dump whatever is in-memory that satisfies the time range
+  for (const auto& stats : stats_history_) {


Is stats_history_ thread safe? Is it safe to access it while another thread might be updating it?

siying · 2018-12-05T22:31:14Z

db/db_impl.cc

+      if (cfd->initialized()) {
+        cfd->internal_stats()->GetMapProperty(
+            *cf_property_info, DB::Properties::kCFStatsNoFileHistogram,
+            &stats_map);


I'm confused. Will stats from different CFs overwrite the same entry in the map?

siying · 2018-12-06T22:53:23Z

db/db_impl.h

@@ -1565,6 +1579,9 @@ class DBImpl : public DB {
    return Env::WLTH_SHORT;
  }

+  std::map<uint64_t, std::map<std::string, std::string>>
+  FindStatsBetween(uint64_t start_time, uint64_t end_time);


I think this API needs more people to review. I am concerned to it. It assumes that all results need to be copied to memory. This makes memory budget hard to control. Yes we can add one more argument about maximum records to report, but this makes the function harder to understand. Another possible way is to make it an iterator interface, so that we only need to page in the records people are currently reading.
It's worth asking MyRocks people about how they plan to turn this interface into a virtual SQL table and allow arbitrary SQL queries, and we should enable them to do it through our API.

Right now we have discussed two general approaches to storing and retrieving stats history.

in-memory: implemented in this PR. Basic idea is to periodically store stats into a map and query through a new API.

pros: no backward compatibility issue

cons: increased memory consumption/limited length of stats history (unable to query history before the last DB restart)

persist to disk: implemented in Persistent stats: periodically persist RocksDB stats #4535. Basic idea is to implicitly create a new column family to store stats history, and have the new
GetStatsHistory API create iterator to query history

pros: limited memory cost, supports queries over lifetime of the DB

cons: additional downgrade operational cost (to manage the new column family), complication with managing additional column family

The original plan was to first roll out in-memory solution to get user engagement with the new API, and then roll out persist to disk when user are comfortable with the potential operational cost of downgrading or find a better solution for smooth downgrade.
The same set of API will be used throughout the process. In the first stage the API will only query in-memory data structure and in the second stage it will create iterators to query on disk data.
@yoshinorim do you mind taking a look and let us know your preference? especially with MyRocks integration?

I think the roll out plan makes sense. From high level point of view, memory consumption is probably the biggest concern.

Capping memory usage if Stats History consumes too much (capping the configurable number of entries or memory size. The latter is easier to tune for users)

Reporting rough memory size used by this feature

@miasantreble I am lost in your comment. I've never suggested any "new API". Both in-memory and persistent-to-disk solution should use the same API. The benefit of having the in-memory version at all is that people can adapt to it, and then later naturally migrate to the persistence version without any API change.

@siying thanks for pointing out the ambiguity, I've updated the comment above. Also how do you like the idea of adding an option to control the amount of memory stats history consumes? I can replace max_stats_history_count with something like stats_history_memory_budget and default to 128K(~15 stat history entries) maybe?

If holding the stats in-memory is a temporary feature I'd be careful about adding an option especially for it.

In the persistent version of the stats, will there be any API to control how much storage is used for stats? Or how much time we retain stats? If so, perhaps we can use that same API to control memory usage when we have the in-memory implementation.

@ajkr maybe we can add another API StartCollectStatsHistory(GetStatsOptions&) to start the background stats collection threads where we can specify the memory limit in GetStatsOptions, and to retrieve most recent stats history, still use GetStatsHistory. Basically we can put all new options in GetStatsOptions to avoid polluting the general options space. How does that sound?

@siying I gave some thought about the iterator interface. My main concern is that the stats history data structure (current a std::map) will be constantly updated (inserted and erased), and iterator interface will need to either 1) delay purging expired data to ensure iterator will have a consistent view of the data, or 2) create possible race condition with iterator reads and data purging. Yoshi pointed to me a similar case in MyRocks where we need to create virtual table using RocksDB data structure: https://github.com/facebook/mysql-5.6/blob/60ab18528d63709cc82364d5b9b81ecbfe8d86e8/storage/rocksdb/rdb_i_s.cc#L1148
MyRocks calls GetPropertiesOfAllTables which takes a pointer to a std::unordered_map<std::string, std::shared_ptr<const TableProperties>>. We can do similar things here to save some parameter copying cost

@siying I gave some thought about the iterator interface. My main concern is that the stats history data structure (current a std::map) will be constantly updated (inserted and erased), and iterator interface will need to either 1) delay purging expired data to ensure iterator will have a consistent view of the data, or 2) create possible race condition with iterator reads and data purging. Yoshi pointed to me a similar case in MyRocks where we need to create virtual table using RocksDB data structure: https://github.com/facebook/mysql-5.6/blob/60ab18528d63709cc82364d5b9b81ecbfe8d86e8/storage/rocksdb/rdb_i_s.cc#L1148

Is the in-memory history stats data a permanent feature? If not, I think we could perhaps prioritize API design over consideration of specific (in-memory) data structures, e.g. std::map. Once we have an interface, we can find a in-memory data structure that meets the requirement of the API.

@miasantreble Let's put the in-memory case aside for just a moment. If we only consider the persistent case, what kind of function will you use?

miasantreble · 2018-12-07T22:33:11Z

sample CF and DB stats returned by GetStatsHistory with commit 088b8d9:

time = 1544221786796536, key = pikachu.compaction.L0.AvgSec, value = 0.000971
time = 1544221786796536, key = pikachu.compaction.L0.CompCount, value = 1.000000
time = 1544221786796536, key = pikachu.compaction.L0.CompSec, value = 0.000971
time = 1544221786796536, key = pikachu.compaction.L0.CompactedFiles, value = 0.000000
time = 1544221786796536, key = default.io_stalls.stop_for_pending_compaction_bytes, value = 0
time = 1544221786796536, key = default.io_stalls.total_slowdown, value = 0
time = 1544221786796536, key = default.io_stalls.total_stop, value = 0
time = 1544221786796536, key = interval_num_keys_written, value = 0
time = 1544221786796536, key = interval_seconds_up, value = 0.000211
time = 1544221786796536, key = interval_stall_micros_readable, value = 00:00:0.000 H:M:S

riversand963 · 2018-12-19T23:52:42Z

db/db_impl.cc

  }
 }

+void DBImpl::PersistStats() {
+  TEST_SYNC_POINT("DBImpl::PersistStats:1");


Is it possible to use a informative name for test sync point?

riversand963 · 2018-12-19T23:55:44Z

db/db_impl.cc

+  }
+  {
+    InstrumentedMutexLock l(&mutex_);
+    default_cf_internal_stats_->GetMapProperty(


We collect different stats for default cf?

riversand963 · 2018-12-19T23:57:14Z

db/db_impl.h

@@ -713,6 +714,10 @@ class DBImpl : public DB {
  static Status CreateAndNewDirectory(Env* env, const std::string& dirname,
                                      std::unique_ptr<Directory>* directory);

+  // return a map of DBStats and CFstats, specify time window etc in stats_opts
+  virtual Status GetStatsHistory(GetStatsOptions& stats_opts,
+    std::map<uint64_t, std::map<std::string, std::string>>& stats_history) override;


Pointer type for out arg?

riversand963 · 2018-12-20T00:16:21Z

util/repeatable_thread.h

+  // allow caller to pass a mutex and temporarily unlock the mutex during
+  // the process of cancel. Useful when function_ also need to lock the same
+  // mutex
+  void cancel(InstrumentedMutex* mutex = nullptr) {


The cancel function looks a little strange with an extra mutex parameter. Is it necessary?

I agree with @riversand963 . Passing in a mutex, which just to be unlocked is a little bit strange.

siying · 2019-01-26T00:48:22Z

db/db_impl.cc

+  }
+}
+
+Status DBImpl::GetStatsHistory(GetStatsOptions& stats_opts,


I would argue that start and end time should be argument, not an option. It's just like in Get(), key is not an option.

siying · 2019-01-26T00:48:59Z

db/stats_history.h

+  AggregationOps aggr_ops;
+};
+
+class StatsHistoryIterator {


This needs to stay in the public headers.

siying · 2019-01-26T00:49:31Z

db/stats_history.h

+
+  // Position at the first key in the source.  The iterator is Valid()
+  // after this call iff the source is not empty.
+  void SeekToFirst();


Do we need this?

siying · 2019-01-26T00:50:34Z

db/stats_history.h

+  // the returned slice is valid only until the next modification of
+  // the iterator.
+  // REQUIRES: Valid()
+  uint64_t key(); // stats history creation timestamp


key(), value() is confusing to me. Why not return StatsRecord, StatSlice or something like that, which is a struct and contain timestamp and stats?

siying · 2019-01-26T00:50:47Z

db/stats_history.h

+  Status status();
+
+  // Advance the iterator to any valid timestamp >= time_start
+  void AdvanceIteratorByTime(uint64_t start_time, uint64_t end_time);


Do we need it at all?

siying · 2019-01-26T00:51:50Z

db/stats_history.h

+  // No copying allowed
+  StatsHistoryIterator(const StatsHistoryIterator&);
+  void operator=(const StatsHistoryIterator&);
+};


Do you want to have the same class to serve both of persistent stats or stats in memory? If you want to do separate ones, maybe it's should be an interface and the in memory one is one implementation.

siying · 2019-01-26T00:52:24Z

db/stats_history.h

+  // list of counters to query, call GetSupportedStatsCounters to get full list
+  std::vector<Tickers> tickers;
+  // aggregation operation like avg, sum, min, max, count, percentiles, etc
+  AggregationOps aggr_ops;


What is it for?

this is for user to specify what kind of aggregation operation to apply on the data, for example, average, sum, min/max, p90, etc. it's not implemented or used anywhere in the initial version.
One thing that's not clear is whether to have two separate accessor for raw StatsRecord and aggregated stats. For example, GetStatsHistory is called for 1 hour interval, certain counters, aggregation operation of average and p90. What is the best way to return the data? I think we can have a StatsRecord() to return the raw data together with timestamp and a AggregatedStats() to return a single aggregated result? Do we need to support aggregation operations that returns more than a single floating point value? How about aggregation operations that takes multiple counters?

If it is not implemented or used. I suggest we remove it for now. We can always add it back later.

@siying got a question for tickers: what form of ticker should users provide?
Currently each ticker have two forms, one is of type enum and another is std::string. For example, Tickers::BLOCK_CACHE_MISS or "rocksdb.block.cache.miss", and the mapping is maintained in TickersNameMap. Is it more user friendly to ask users to provide the latter and add lookup functions to return the enum type to do the filtering before returning the results?

siying · 2019-01-30T21:08:17Z

db/db_impl.cc

+            *cf_property_info, DB::Properties::kCFFileHistogram, &cf_stats_map);
+        // add column family name to the key string
+        for (const auto& cf_stats : cf_stats_map) {
+          std::string key_with_cf_name = cfd->GetName() + "." + cf_stats.first;


"." is not a good delimiter for column family. There are a lot of "." in the name of stats.

siying · 2019-01-30T21:09:12Z

db/db_impl.cc

+        // add column family name to the key string
+        for (const auto& cf_stats : cf_stats_map) {
+          std::string key_with_cf_name = cfd->GetName() + "." + cf_stats.first;
+          stats_map[key_with_cf_name] = cf_stats.second;


Am I right that the stats are accumulative counters? I believe we want the counters for the current window.

stats_map is a single level std::map that stores the stats kv pairs. In this loop we augment the stats name to include the column family name. Once stats_map is populated, it will be added to stats_history_ (two level std::map) as the value.

InternalStats->GetMapProperty returns all cumulative counters in the format of map<string, string>. Of all the counters, only one is not in floating point form: write_stall_micros_readable and there is a related write_stall_micros so skipping it will not cause loss of information. Shall convert the map to map<string, dobule> to make it easier to calculate the delta?

siying · 2019-02-01T22:32:21Z

db/db_impl.cc

@@ -646,8 +646,21 @@ void DBImpl::StartTimedTasks() {
  }
 }

+size_t DBImpl::GetStatsHistorySize() const {


Call it "Estiamte", not "Get".

siying · 2019-02-01T22:33:48Z

db/db_impl.cc

+    size_total += sizeof(stats.first) + sizeof(stats.second);
+    if (stats.second.size() != 0) {
+      auto s = stats.second.begin();
+      size_total += (sizeof(s->first) + sizeof(s->second)) * stats.second.size();


Here we go though every time slice, and for each, estimating the memory by using the first one in the map, times map size. I think a much more accurate estimation will be the other around: do a more accurate estimate of the memory consumption for one time slice, and times number of time slices.

siying · 2019-02-01T22:35:25Z

db/db_impl.cc

-          it = stats_history_.erase(it);
-        }
-      }
+    bool GC_needed = GetStatsHistorySize() >


GC => "gc"

siying · 2019-02-01T22:36:04Z

db/db_impl.cc

-          it = stats_history_.erase(it);
-        }
-      }
+    bool GC_needed = GetStatsHistorySize() >


Calling "GC" may be a little bit confusing. Maybe just call it "purge".

siying · 2019-02-01T22:38:42Z

db/in_memory_stats_history.cc

@@ -48,8 +44,8 @@ void InMemoryStatsHistoryIterator::AdvanceIteratorByTime(uint64_t start_time, ui
    found = db_impl_->FindStatsByTime(start_time, end_time, &new_time, &stats_map);
  }
  if (found) {
-    value_.swap(stats_map);
-    key_ = new_time;
+    stats_map_.swap(stats_map);


Why don't we directly pass &stats_map_ to FindStatsByTime()?

siying · 2019-02-01T22:40:24Z

db/version_set.cc

@@ -3834,10 +3833,7 @@ Status VersionSet::ReduceNumberOfLevels(const std::string& dbname,
  ColumnFamilyDescriptor dummy_descriptor(kDefaultColumnFamilyName,
                                          ColumnFamilyOptions(*options));
  dummy.push_back(dummy_descriptor);
-  {
-    InstrumentedMutexLock l(&dummy_mutex);


What's the change for?

to revert some old change that is no longer needed

siying · 2019-02-01T22:42:13Z

include/rocksdb/options.h

+  // if not zero, periodically take stats snapshots and store in memory, the
+  // memory size for stats snapshots is capped at stats_history_buffer_size
+  // Default: 1MB
+  unsigned int stats_history_buffer_size = 1024*1024;


1024 * 1024

Should be size_t.

siying · 2019-02-01T22:43:34Z

include/rocksdb/stats_history.h

-  std::vector<Tickers> tickers;
-  // aggregation operation like avg, sum, min, max, count, percentiles, etc
-  AggregationOps aggr_ops;
+  std::vector<std::string> tickers;


It's not used. Can we remove it?

siying · 2019-02-01T22:44:19Z

include/rocksdb/stats_history.h


-  // Return the value for the current entry.  The underlying storage for
-  // the returned slice is valid only until the next modification of
+  // Return the current stats history as an std::map. The underlying storage


We should mention it is map from what to what.

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-02-07T03:54:08Z

@miasantreble has updated the pull request. Re-import the pull request

miasantreble · 2019-02-07T03:59:32Z

GetStatsHistory output with the latest commit e428280
https://gist.github.com/miasantreble/465de00c2432f5c675d73978eefabea5
The test does two batches of operations (to populate statistics) and sleep for 4 seconds after each batch. stats_persist_period_sec (sample interval in seconds) is set to 1 second
At end of test, GetStatsHistory returns 8 stats slices and only two of them contains non-zero data, which is consistent with the expectation
The test can be found here

facebook-github-bot · 2019-02-07T05:57:07Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-02-07T21:24:47Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-02-12T22:39:28Z

@miasantreble has updated the pull request. Re-import the pull request

miasantreble · 2019-02-12T22:48:23Z

@riversand963 thanks for your review, I think all you comments have been addressed, do you mind taking another look to see if there is any other issues?

siying · 2019-02-12T22:53:57Z

db/db_options_test.cc

+  // Wait for stats persist to finish
+  for (; mock_time < 20; ++mock_time) {
+    dbfull()->TEST_WaitForPersistStatsRun(
+        [&] { mock_env->set_current_time(mock_time); });


And here TEST_WaitForPersistStatsRun() is called again. Did you say that it may hang?

the only occasion where hang is observed is when first turning off persistent stats thread through SetOptions and then reenable it, other than that it works fine.

siying · 2019-02-12T22:54:53Z

db/db_options_test.cc

+  db_->CompactRange(CompactRangeOptions(), nullptr, nullptr);
+  for (; mock_time < 10; ++mock_time) {
+    dbfull()->TEST_WaitForPersistStatsRun(
+        [&] { mock_env->set_current_time(mock_time); });


Did you say that you can't run TEST_WaitForPersistStatsRun() twice? Here it is 10 times.

siying · 2019-02-12T22:56:17Z

db/db_options_test.cc

+  size_t stats_history_size = dbfull()->TEST_EstiamteStatsHistorySize();
+  ASSERT_GE(slice_count, 9);
+  ASSERT_GE(stats_history_size, 12000);
+  // capping memory cost at 2000 bytes


Here it says 2000, but it set to be 12000.

siying · 2019-02-12T22:56:50Z

db/db_options_test.cc

+  }
+  delete stats_iter;
+  size_t stats_history_size_reopen = dbfull()->TEST_EstiamteStatsHistorySize();
+  ASSERT_LE(slice_count, 2);


Can you add a comment to explain why it is <=2 ? Is it that we know size of a slice will be at at least 4000?

facebook-github-bot · 2019-02-12T23:38:26Z

@miasantreble has updated the pull request. Re-import the pull request

riversand963

Thanks @miasantreble. I've taken another pass and left some comments.

riversand963 · 2019-02-13T17:12:21Z

HISTORY.md

@@ -7,10 +7,11 @@
 * For users of dictionary compression with ZSTD v0.7.0+, we now reuse the same digested dictionary when compressing each of an SST file's data blocks for faster compression speeds.
 * For all users of dictionary compression who set `cache_index_and_filter_blocks == true`, we now store dictionary data used for decompression in the block cache for better control over memory usage. For users of ZSTD v1.1.4+ who compile with -DZSTD_STATIC_LINKING_ONLY, this includes a digested dictionary, which is used to increase decompression speed.
 * Add support for block checksums verification for external SST files before ingestion.
+* Introduced stats history which periodically saves Statistics snapshots and added `GetStatsHistory` API to retrieve these snapshots.


Introduced -> Introduce

db/db_impl.cc

db/db_impl.h

riversand963 · 2019-02-13T17:46:19Z

db/db_impl.h

+      uint64_t start_time, uint64_t end_time,
+      StatsHistoryIterator** stats_iterator) override;
+
+  // return list of currently supported stats counters


Is there supposed to be a function here, or should the comment be removed?

will remove

monitoring/statistics.cc

db/db_impl.cc

facebook-github-bot · 2019-02-14T19:03:25Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

miasantreble · 2019-02-15T21:25:38Z

all review comments have been addressed, @siying @riversand963 PTAL?

db/db_impl.cc

riversand963 · 2019-02-15T21:41:12Z

db/db_impl.cc

-  return new_iterator->status();
+    std::unique_ptr<StatsHistoryIterator>* stats_iterator) {
+  if (!stats_iterator) {
+    return Status::InvalidArgument();


Pass a string to InvalidArgument to provide more info.

db/db_options_test.cc

monitoring/statistics.cc

facebook-github-bot · 2019-02-19T18:26:20Z

@miasantreble has updated the pull request. Re-import the pull request

riversand963 · 2019-02-20T00:06:44Z

db/db_impl.cc

+bool DBImpl::FindStatsByTime(uint64_t start_time, uint64_t end_time,
+                             uint64_t* new_time,
+                             std::map<std::string, uint64_t>* stats_map) {
+  assert(new_time && stats_map);


I usually use one assertion for each condition. but this is minor here, and will leave this up to you.

riversand963 · 2019-02-20T00:15:04Z

db/db_impl.cc

+      stats_history_[now_micros] = stats_delta;
+    }
+    stats_slice_initialized_ = true;
+    stats_slice_ = stats_map;


Maybe use std::vector's swap?

riversand963 · 2019-02-20T00:20:42Z

db/db_impl.h

@@ -724,6 +728,17 @@ class DBImpl : public DB {
  static Status CreateAndNewDirectory(Env* env, const std::string& dirname,
                                      std::unique_ptr<Directory>* directory);

+  // Given a time window, return an iterator for accessing stats history
+  virtual Status GetStatsHistory(


Nit: remove virtual. Use only override. Follow the updated, enforced code convention.

Disregard this

riversand963 · 2019-02-20T00:22:23Z

db/in_memory_stats_history.cc

+// found in the LICENSE file. See the AUTHORS file for names of contributors.
+
+#include "db/in_memory_stats_history.h"
+#include "db/db_impl.h"


Nit: sort the includes.

interestingly this was suggested by make format but I will sort by lexicographic order

riversand963 · 2019-02-20T00:27:48Z

db/in_memory_stats_history.h

+    AdvanceIteratorByTime(start_time_, end_time_);
+  }
+  virtual ~InMemoryStatsHistoryIterator() override;
+  virtual bool Valid() const override;


Nits: remove virtual and keep override to indicate that these functions do override virtual functions in their base class, i.e. StatsHistoryIterator with the same signature. https://stackoverflow.com/questions/39932391/virtual-override-or-both-c. This also helps avoid potential Phabricator warnings/errors.

riversand963

LGTM. Can you make sure you address the comments from @siying ?
Regarding the virtual vs. override thing, maybe you can first rebase your changes on top of the latest master, then see if this rule has been applied to neighboring functions. If so, then maybe a good idea to change in your PR as well. Otherwise it's OK to leave it there as is.

siying · 2019-02-20T01:14:04Z

I remember some lint tool will fail if "virtual ... override" is used. I'm surprised that we didn't see any.

facebook-github-bot · 2019-02-20T21:58:32Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: This PR continues the work in #4748 and #4535 by adding a new DBOption `persist_stats_to_disk` which instructs RocksDB to persist stats history to RocksDB itself. When statistics is enabled, and both options `stats_persist_period_sec` and `persist_stats_to_disk` are set, RocksDB will periodically write stats to a built-in column family in the following form: key -> (timestamp in microseconds)#(stats name), value -> stats value. The existing API `GetStatsHistory` will detect the current value of `persist_stats_to_disk` and either read from in-memory data structure or from the hidden column family on disk. Pull Request resolved: #5046 Differential Revision: D15863138 Pulled By: miasantreble fbshipit-source-id: bb82abdb3f2ca581aa42531734ac799f113e931b

Summary: This PR continues the work in facebook#4748 and facebook#4535 by adding a new DBOption `persist_stats_to_disk` which instructs RocksDB to persist stats history to RocksDB itself. When statistics is enabled, and both options `stats_persist_period_sec` and `persist_stats_to_disk` are set, RocksDB will periodically write stats to a built-in column family in the following form: key -> (timestamp in microseconds)#(stats name), value -> stats value. The existing API `GetStatsHistory` will detect the current value of `persist_stats_to_disk` and either read from in-memory data structure or from the hidden column family on disk. Pull Request resolved: facebook#5046 Differential Revision: D15863138 Pulled By: miasantreble fbshipit-source-id: bb82abdb3f2ca581aa42531734ac799f113e931b

miasantreble requested review from sagar0, ajkr and siying December 5, 2018 01:02

facebook-github-bot added the CLA Signed label Dec 5, 2018

miasantreble force-pushed the get-stats-history branch from 5cca3ac to a0767ca Compare December 5, 2018 06:55

siying reviewed Dec 5, 2018

View reviewed changes

siying reviewed Dec 6, 2018

View reviewed changes

miasantreble force-pushed the get-stats-history branch from 088b8d9 to 9c86d60 Compare December 7, 2018 22:49

riversand963 reviewed Dec 19, 2018

View reviewed changes

riversand963 reviewed Dec 20, 2018

View reviewed changes

miasantreble force-pushed the get-stats-history branch 2 times, most recently from 855328f to 634c071 Compare January 24, 2019 21:43

siying reviewed Jan 26, 2019

View reviewed changes

miasantreble force-pushed the get-stats-history branch from 634c071 to d583c4c Compare January 30, 2019 20:02

siying reviewed Jan 30, 2019

View reviewed changes

siying reviewed Feb 1, 2019

View reviewed changes

facebook-github-bot reviewed Feb 5, 2019

View reviewed changes

miasantreble force-pushed the get-stats-history branch from e428280 to 9fc2efc Compare February 7, 2019 05:56

facebook-github-bot reviewed Feb 7, 2019

View reviewed changes

miasantreble force-pushed the get-stats-history branch from 9fc2efc to b1562e1 Compare February 7, 2019 21:24

facebook-github-bot reviewed Feb 7, 2019

View reviewed changes

miasantreble force-pushed the get-stats-history branch from b1562e1 to bded764 Compare February 7, 2019 22:10

more test improvements and fix lint

209d6ce

siying reviewed Feb 12, 2019

View reviewed changes

comment update

dbc0bed

riversand963 reviewed Feb 13, 2019

View reviewed changes

address review comments from Yanqin

d838788

facebook-github-bot reviewed Feb 14, 2019

View reviewed changes

riversand963 reviewed Feb 15, 2019

View reviewed changes

fix more review comments

e07db0b

riversand963 reviewed Feb 20, 2019

View reviewed changes

riversand963 approved these changes Feb 20, 2019

View reviewed changes

address review comments

2782095

facebook-github-bot reviewed Feb 20, 2019

View reviewed changes

facebook-github-bot closed this in c4f5d0a Feb 20, 2019

miasantreble deleted the get-stats-history branch February 22, 2019 18:15

miasantreble mentioned this pull request Mar 7, 2019

Persistent Stats: persist stats history to disk #5046

Closed

add GetStatsHistory to retrieve stats snapshots #4748

add GetStatsHistory to retrieve stats snapshots #4748

Conversation

miasantreble commented Dec 5, 2018 • edited Loading

siying commented Dec 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miasantreble Dec 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miasantreble Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miasantreble commented Dec 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 7, 2019

miasantreble commented Feb 7, 2019 • edited Loading

facebook-github-bot commented Feb 7, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 7, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 12, 2019

miasantreble commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 12, 2019

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miasantreble Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Feb 14, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

miasantreble commented Feb 15, 2019

Choose a reason for hiding this comment

facebook-github-bot commented Feb 19, 2019

Choose a reason for hiding this comment

miasantreble commented Dec 5, 2018 •

edited

Loading

miasantreble Dec 7, 2018 •

edited

Loading

miasantreble Dec 10, 2018 •

edited

Loading

miasantreble commented Feb 7, 2019 •

edited

Loading

miasantreble Feb 13, 2019 •

edited

Loading