Support `GetMapProperty()` with "rocksdb.dbstats" #9057

ajkr · 2021-10-19T22:56:17Z

This PR supports querying GetMapProperty() with "rocksdb.dbstats" to get the DB-level stats in a map format. It only reports cumulative stats over the DB lifetime and, as such, does not update the baseline for interval stats. Like other map properties, the string keys are not (yet) exposed in the public API.

Test Plan: new unit test

facebook-github-bot · 2021-10-20T00:07:51Z

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

zhichao-cao · 2021-10-20T00:11:43Z

db/internal_stats.cc

+    (*db_stats)[db_stats_type_to_info.at(type).property_name] =
+        std::to_string(GetDBStats(type));
+  }
+  double seconds_up = (clock_->NowMicros() - started_at_) / kMicrosInSec;


We may use the same calculation, either remove the "1" in DumpDBStats or add "1" here.....

OK, I removed the + 1 from the old calculation (I didn't want to bring the + 1 to the new code because it would make testing difficult). But let me know if you can think of a reason why it was there in the first place -- I have no idea.

Oh it's because seconds_up is used as a denominator later. Oddly other code to handle this situation uses std::max(interval_seconds_up, 0.001). Let me use that instead for handling zero uptime without dividing by zero.

zhichao-cao

LGTM

facebook-github-bot · 2021-10-20T00:29:05Z

@ajkr has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-10-20T00:33:50Z

@ajkr has updated the pull request. You must reimport the pull request before landing.

ajkr

Thanks for the review! BTW I also increased test coverage a bit.

ajkr · 2021-10-20T00:31:28Z

db/internal_stats.cc

+    (*db_stats)[db_stats_type_to_info.at(type).property_name] =
+        std::to_string(GetDBStats(type));
+  }
+  double seconds_up = (clock_->NowMicros() - started_at_) / kMicrosInSec;


OK, I removed the + 1 from the old calculation (I didn't want to bring the + 1 to the new code because it would make testing difficult). But let me know if you can think of a reason why it was there in the first place -- I have no idea.

facebook-github-bot · 2021-10-20T00:53:00Z

@ajkr has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-10-20T02:15:34Z

@ajkr has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-10-20T02:18:00Z

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

zhichao-cao · 2021-10-20T06:15:10Z

db/internal_stats.cc

@@ -1324,7 +1362,7 @@ void InternalStats::DumpDBStats(std::string* value) {
           NumberToHumanString(write_with_wal).c_str(),
           NumberToHumanString(wal_synced).c_str(),
           write_with_wal / static_cast<double>(wal_synced + 1),


Should we also change "wal_synced + 1"?

Done here and a few more places, but not yet all + 1s in the file...

zhichao-cao · 2021-10-20T06:17:43Z

db/db_properties_test.cc

@@ -1895,7 +1915,80 @@ TEST_F(DBPropertiesTest, BlockCacheProperties) {
  ASSERT_EQ(0, value);
 }

+TEST_F(DBPropertiesTest, GetMapPropertyDbStats) {


The test looks good to me. I think we do not need to cover user_write_stall_micros in this test

zhichao-cao · 2021-10-20T06:19:44Z

HISTORY.md

@@ -18,6 +18,7 @@
 * Introduce an experimental feature to dump out the blocks from block cache and insert them to the secondary cache to reduce the cache warmup time (e.g., used while migrating DB instance). More information are in `class CacheDumper` and `CacheDumpedLoader` at `rocksdb/utilities/cache_dump_load.h` Note that, this feature is subject to the potential change in the future, it is still experimental.
 * Introduced a new BlobDB configuration option `blob_garbage_collection_force_threshold`, which can be used to trigger compactions targeting the SST files which reference the oldest blob files when the ratio of garbage in those blob files meets or exceeds the specified threshold. This can reduce space amplification with skewed workloads where the affected SST files might not otherwise get picked up for compaction.
 * Added EXPERIMENTAL support for table file (SST) unique identifiers that are stable and universally unique, available with new function `GetUniqueIdFromTableProperties`. Only SST files from RocksDB >= 6.24 support unique IDs.
+* Added `GetMapProperty()` support for "rocksdb.dbstats" (`DB::Properties::kDBStats`). As a map property, it only includes cumulative stats over the DB's lifetime.


it only includes cumulative stats (i.e., user write related) over.....

It includes everything in InternalDBStatsType (which, yes, happen to be all write related) and also uptime.

Done, added more description of the map's contents.

ajkr

Thanks for the review. BTW, needs Phabricator accept (assuming you think it's ready).

ajkr · 2021-10-20T17:47:10Z

HISTORY.md

@@ -18,6 +18,7 @@
 * Introduce an experimental feature to dump out the blocks from block cache and insert them to the secondary cache to reduce the cache warmup time (e.g., used while migrating DB instance). More information are in `class CacheDumper` and `CacheDumpedLoader` at `rocksdb/utilities/cache_dump_load.h` Note that, this feature is subject to the potential change in the future, it is still experimental.
 * Introduced a new BlobDB configuration option `blob_garbage_collection_force_threshold`, which can be used to trigger compactions targeting the SST files which reference the oldest blob files when the ratio of garbage in those blob files meets or exceeds the specified threshold. This can reduce space amplification with skewed workloads where the affected SST files might not otherwise get picked up for compaction.
 * Added EXPERIMENTAL support for table file (SST) unique identifiers that are stable and universally unique, available with new function `GetUniqueIdFromTableProperties`. Only SST files from RocksDB >= 6.24 support unique IDs.
+* Added `GetMapProperty()` support for "rocksdb.dbstats" (`DB::Properties::kDBStats`). As a map property, it only includes cumulative stats over the DB's lifetime.


It includes everything in InternalDBStatsType (which, yes, happen to be all write related) and also uptime.

ajkr · 2021-10-20T17:47:37Z

db/internal_stats.cc

@@ -1324,7 +1362,7 @@ void InternalStats::DumpDBStats(std::string* value) {
           NumberToHumanString(write_with_wal).c_str(),
           NumberToHumanString(wal_synced).c_str(),
           write_with_wal / static_cast<double>(wal_synced + 1),


facebook-github-bot · 2021-10-20T18:18:36Z

@ajkr has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-10-20T18:23:46Z

@ajkr has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-10-20T18:26:09Z

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

zhichao-cao · 2021-10-20T18:36:26Z

Thanks for the review. BTW, needs Phabricator accept (assuming you think it's ready).

Accepted in Phabricator

facebook-github-bot · 2021-10-20T20:17:09Z

@ajkr merged this pull request in 4217d1b.

Summary: This PR supports querying `GetMapProperty()` with "rocksdb.dbstats" to get the DB-level stats in a map format. It only reports cumulative stats over the DB lifetime and, as such, does not update the baseline for interval stats. Like other map properties, the string keys are not (yet) exposed in the public API. Pull Request resolved: facebook/rocksdb#9057 Test Plan: new unit test Reviewed By: zhichao-cao Differential Revision: D31781495 Pulled By: ajkr fbshipit-source-id: 6f77d3aee8b4b1a015061b8c260a123859ceaf9b

facebook-github-bot added the CLA Signed label Oct 19, 2021

ajkr force-pushed the getmapppt-rocksdb-dbstats branch from bc49ccd to 742f78c Compare October 19, 2021 23:53

ajkr marked this pull request as ready for review October 19, 2021 23:53

zhichao-cao reviewed Oct 20, 2021

View reviewed changes

zhichao-cao approved these changes Oct 20, 2021

View reviewed changes

ajkr commented Oct 20, 2021

View reviewed changes

zhichao-cao reviewed Oct 20, 2021

View reviewed changes

ajkr commented Oct 20, 2021

View reviewed changes

ajkr added 13 commits October 20, 2021 10:53

Support GetMapProperty() with "rocksdb.dbstats"

e105a1f

update rocksdb.dbstats API doc

fa5258e

update rocksdb.dbstats API doc

58f7276

remove extra microsecond; start test

dc43599

update HISTORY.md

b11767c

improve test

1acd0e4

remove extra microsecond in string property

4d1cffa

avoid dividing by zero uptime

133cb9e

make format

fad2e82

fix status checks

2a84827

explicit conversion to int for Windows

391e3fe

update HISTORY.md

2f06383

fix writes per commit group denominator

d911049

ajkr force-pushed the getmapppt-rocksdb-dbstats branch from 028d170 to d911049 Compare October 20, 2021 18:18

fix more unnecessary +1s

3f347e6

facebook-github-bot closed this in 4217d1b Oct 20, 2021

facebook-github-bot added the Merged label Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `GetMapProperty()` with "rocksdb.dbstats" #9057

Support `GetMapProperty()` with "rocksdb.dbstats" #9057

ajkr commented Oct 19, 2021 •

edited

Loading

facebook-github-bot commented Oct 20, 2021

zhichao-cao Oct 20, 2021

ajkr Oct 20, 2021 •

edited

Loading

ajkr Oct 20, 2021

zhichao-cao left a comment

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

ajkr left a comment

ajkr Oct 20, 2021 •

edited

Loading

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

zhichao-cao Oct 20, 2021

ajkr Oct 20, 2021

ajkr Oct 20, 2021 •

edited

Loading

zhichao-cao Oct 20, 2021 •

edited

Loading

zhichao-cao Oct 20, 2021

ajkr Oct 20, 2021

ajkr Oct 20, 2021 •

edited

Loading

ajkr left a comment

ajkr Oct 20, 2021

ajkr Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

zhichao-cao commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

Support GetMapProperty() with "rocksdb.dbstats" #9057

Support GetMapProperty() with "rocksdb.dbstats" #9057

Conversation

ajkr commented Oct 19, 2021 • edited Loading

facebook-github-bot commented Oct 20, 2021

Choose a reason for hiding this comment

ajkr Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhichao-cao left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

ajkr left a comment

Choose a reason for hiding this comment

ajkr Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkr Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

zhichao-cao Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkr Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

ajkr left a 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 Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

zhichao-cao commented Oct 20, 2021

facebook-github-bot commented Oct 20, 2021

Support `GetMapProperty()` with "rocksdb.dbstats" #9057

Support `GetMapProperty()` with "rocksdb.dbstats" #9057

ajkr commented Oct 19, 2021 •

edited

Loading

ajkr Oct 20, 2021 •

edited

Loading

ajkr Oct 20, 2021 •

edited

Loading

ajkr Oct 20, 2021 •

edited

Loading

zhichao-cao Oct 20, 2021 •

edited

Loading

ajkr Oct 20, 2021 •

edited

Loading