feat:Statistics ticker count #2769
Conversation
WalkthroughThese changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Pika
participant Config
participant RocksDB
participant Redis
User->>+Pika: Update pika.conf (enable-db-statistics)
Pika->>+Config: Load configuration
Config->>Config: Set enable_db_statistics_
Config-->>-Pika: Configuration loaded
User->>+Pika: Request RocksDB Info
Pika->>+Config: Check enable_db_statistics()
Config-->>-Pika: Return boolean value
Pika->>+Redis: Call GetRocksDBInfo with boolean parameter
Redis->>RocksDB: Fetch info with or without ticker based on parameter
RocksDB-->>Redis: Return info details
Redis-->>Pika: Return formatted info
Pika-->>User: Display RocksDB Info
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- conf/pika.conf (1 hunks)
- include/pika_conf.h (2 hunks)
- src/pika_admin.cc (1 hunks)
- src/storage/include/storage/storage.h (1 hunks)
- src/storage/src/redis.cc (2 hunks)
- src/storage/src/redis.h (2 hunks)
- src/storage/src/storage.cc (1 hunks)
Additional comments not posted (9)
src/storage/src/redis.h (2)
247-247: Added method for RocksDB information retrieval with a new parameter.The method
GetRocksDBInfohas been appropriately updated to include the newbool open_tickerparameter, which aligns with the PR's objective to conditionally provide detailed statistics based on configuration. This change should be cross-verified in the implementation to ensure it handles the parameter correctly.
473-473: Initialization of new member variable.The member variable
db_statistics_has been initialized tonullptr. This is a standard practice for pointer safety. However, ensure that this variable is properly managed throughout its lifecycle to avoid memory leaks or dangling pointers, especially in the destructor or when copying instances of this class.src/storage/src/redis.cc (1)
269-269: Comprehensive update toGetRocksDBInfoto include detailed statistics.The method has been extensively updated to gather and format a wide range of database statistics. The use of lambdas for property and ticker data retrieval is efficient and makes the code cleaner. However, ensure that all used properties and tickers are supported by your version of RocksDB to avoid runtime errors. Additionally, the conditional checks for
open_tickerare crucial and must be thoroughly tested to ensure that enabling/disabling statistics works as expected.Also applies to: 269-458
conf/pika.conf (1)
353-353: Configuration setting added correctly for RocksDB statistics tickers.The new configuration option
open_rocksdb_statistics_tickersis added with a default value of "no", which aligns with the PR's objectives. This option is well-documented and follows the conventions used throughout the configuration file.include/pika_conf.h (2)
177-179: New methodopen_rocksdb_statistics_tickers()added correctly.The method
open_rocksdb_statistics_tickers()has been added to allow access to the new configuration setting. It is implemented correctly, using thread-safe access patterns withstd::shared_lock, consistent with other getters in the class.
893-893: New member variableopen_rocksdb_statistics_tickers_initialized correctly.The boolean member variable
open_rocksdb_statistics_tickers_is initialized tofalse, aligning with the default configuration specified inpika.conf. This is a good practice as it ensures the default behavior is consistent across the application.src/storage/include/storage/storage.h (1)
1094-1094: Updated method signature to includeopen_tickerparameter.The method
GetRocksDBInfonow accepts a boolean parameter to conditionally fetch RocksDB statistics. This is a critical change that affects all parts of the system where this method is called.Verification successful
Updated method signature to include
open_tickerparameter.The method
GetRocksDBInfonow accepts a boolean parameter to conditionally fetch RocksDB statistics. This is a critical change that affects all parts of the system where this method is called. The following instances have been verified to include the new parameter:
src/pika_admin.cc
- Line:
db_item.second->storage()->GetRocksDBInfo(rocksdb_info, true);- Line:
db_item.second->storage()->GetRocksDBInfo(rocksdb_info, false);src/storage/src/storage.cc
- Line:
inst->GetRocksDBInfo(info, temp, open_ticker);src/storage/src/redis.cc
- Line:
void Redis::GetRocksDBInfo(std::string& info, const char* prefix, bool open_ticker)These updates confirm that the method calls have been correctly modified to include the new
open_tickerparameter.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetRocksDBInfo` include the new boolean parameter. # Test: Search for the function usage. Expect: Only occurances of the new signature with the boolean parameter. rg --type cpp 'GetRocksDBInfo'Length of output: 691
src/storage/src/storage.cc (1)
1924-1928: Review ofStorage::GetRocksDBInfofunction changes:The addition of the
open_tickerparameter is well-handled in the implementation. The function effectively delegates the responsibility of gathering statistics to theinst->GetRocksDBInfomethod of individual instances, which is a good design choice as it maintains separation of concerns.Ensure that all instances of
Redisclass (or similar) that implement theGetRocksDBInfomethod are updated to handle this new parameter appropriately to avoid runtime errors.src/pika_admin.cc (1)
1309-1313: Conditional RocksDB statistics collection based on configurationThe implementation correctly toggles the collection of RocksDB statistics based on the
open_rocksdb_statistics_tickersconfiguration setting. This change allows for performance optimizations by disabling detailed statistics when not needed.Ensure that the
GetRocksDBInfomethod appropriately handles theopen_tickerparameter in all scenarios to prevent any potential issues when statistics collection is toggled.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- conf/pika.conf (1 hunks)
- src/storage/src/redis.cc (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- conf/pika.conf
- src/storage/src/redis.cc
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- conf/pika.conf (1 hunks)
- src/pika_admin.cc (2 hunks)
- src/pika_conf.cc (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- conf/pika.conf
- src/pika_admin.cc
Additional comments not posted (2)
src/pika_conf.cc (2)
697-701: Proper handling of theopen-rocksdb-statistics-tickersconfiguration.The code correctly checks the configuration string and sets the boolean value based on it. The default behavior of treating any value other than "yes" as "no" is a safe approach to configuration flags.
805-805: Correct implementation of configuration rewriting foropen-rocksdb-statistics-tickers.The method effectively ensures that the current state of the
open_rocksdb_statistics_tickers_is accurately represented in the configuration file, using a clear and concise approach.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- include/pika_conf.h (3 hunks)
- src/pika_admin.cc (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- include/pika_conf.h
- src/pika_admin.cc
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- include/pika_conf.h (3 hunks)
- src/pika_admin.cc (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- include/pika_conf.h
- src/pika_admin.cc
|
@coderabbitai review |
|
@baixin01: I will review the changes and provide feedback. Actions performedReview triggered.
|
|
@coderabbitai review |
Actions performedReview triggered.
|
|
@CodiumAI-Agent /review |
|
@CodiumAI-Agent /improve |
PR Reviewer Guide 🔍
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- include/pika_conf.h (2 hunks)
- src/pika_admin.cc (2 hunks)
- src/storage/src/redis.cc (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- include/pika_conf.h
- src/pika_admin.cc
- src/storage/src/redis.cc
| @@ -470,6 +470,7 @@ class Redis { | |||
| Storage* const storage_; | |||
| std::shared_ptr<LockMgr> lock_mgr_; | |||
| rocksdb::DB* db_ = nullptr; | |||
| std::shared_ptr<rocksdb::Statistics> db_statistics_ = nullptr; | |||
include/pika_conf.h
Outdated
| @@ -887,6 +890,7 @@ class PikaConf : public pstd::BaseConf { | |||
| int64_t thread_migrate_keys_num_ = 0; | |||
| int64_t max_write_buffer_size_ = 0; | |||
| int64_t max_total_wal_size_ = 0; | |||
| bool open_rocksdb_statistics_tickers_ = false; | |||
There was a problem hiding this comment.
这个命令最好是支持可以动态修改不用重启进程，线上用来定位问题的时候更方便。
There was a problem hiding this comment.
rocksdb statics统计 不支持动态
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- conf/pika.conf (1 hunks)
- include/pika_conf.h (2 hunks)
- include/pika_repl_client.h (1 hunks)
- src/pika_admin.cc (1 hunks)
- src/pika_conf.cc (2 hunks)
- src/storage/src/redis.cc (4 hunks)
- src/storage/src/redis.h (1 hunks)
Additional comments not posted (18)
src/storage/src/redis.h (1)
473-473: LGTM! Ensure consistent initialization and usage.The new member variable
db_statistics_enhances the class's capability to manage RocksDB statistics. Verify that its initialization and usage are consistent across the codebase.src/storage/src/redis.cc (6)
70-74: LGTM! Ensure consistent initialization and usage.The new configuration option for enabling database statistics is correctly integrated. Verify that the initialization and usage of
db_statistics_are consistent across the codebase.
278-282: LGTM! The new lambda function improves code clarity.The
write_aggregated_int_propertylambda function standardizes the method of reporting integer properties from RocksDB, improving code clarity and consistency.
284-296: LGTM! The new lambda function enhances property reporting.The
write_propertylambda function allows for more flexible reporting, accommodating cases where thehandles_vector may be empty or populated. This enhances the robustness of the code when retrieving properties from different column families.
298-303: LGTM! The new lambda function for ticker count reporting is a valuable addition.The
write_ticker_countlambda function tracks various statistics related to database operations, significantly broadening the scope of metrics being reported.
315-372: LGTM! The refactored metrics reporting improves consistency.The refactored metrics reporting using the new lambda functions ensures consistency in how metrics are handled across the codebase.
375-465: LGTM! The new metrics for blob files enhance monitoring capabilities.The new metrics for blob files improve the monitoring capabilities of the Redis storage implementation.
conf/pika.conf (2)
349-350: Configuration Addition:enable-db-statisticsThe new configuration option
enable-db-statisticsis correctly added with a default value ofno. This allows users to enable or disable database statistics.
351-353: Configuration Addition:db-statistics-levelThe new configuration option
db-statistics-levelis correctly added with a default value of2. This allows users to set the level of detail for database statistics.src/pika_conf.cc (3)
697-701: Configuration Loading:open-rocksdb-statistics-tickersThe
Loadmethod correctly retrieves the configuration value foropen-rocksdb-statistics-tickersand sets theenable_db_statistics_flag.
702-706: Configuration Loading:db-statistics-levelThe
Loadmethod correctly retrieves the configuration value fordb-statistics-leveland sets thedb_statistics_level_variable, ensuring it is not set below zero.
811-812: Configuration Saving:enable-db-statisticsanddb-statistics-levelThe
ConfigRewritemethod correctly saves the new configuration parametersenable-db-statisticsanddb-statistics-level.include/pika_conf.h (4)
177-179: LGTM!The method
enable_db_statisticscorrectly returns the value ofenable_db_statistics_.
180-183: LGTM!The method
db_statistics_levelcorrectly returns the value ofdb_statistics_level_with appropriate thread safety.
897-897: LGTM!The private member variable
enable_db_statistics_is correctly defined and initialized tofalse.
898-898: LGTM!The private member variable
db_statistics_level_is correctly defined and initialized to0.src/pika_admin.cc (2)
2150-2154: Ensure Consistent Naming for Configuration ParametersThe configuration parameter
enable-db-statisticsis checked and added correctly. Verify that this parameter is consistently named and used across the entire codebase.
2156-2160: Ensure Consistent Naming for Configuration ParametersThe configuration parameter
db-statistics-levelis checked and added correctly. Verify that this parameter is consistently named and used across the entire codebase.
include/pika_repl_client.h
Outdated
| @@ -21,7 +21,7 @@ | |||
| #include "include/pika_repl_client_thread.h" | |||
|
|
|||
| #include "net/include/thread_pool.h" | |||
| #include "pika_inner_message.pb.h" | |||
| #include "pika_inner_message.pb. h" | |||
There was a problem hiding this comment.
Fix the include directive.
The include directive has an unintended space before the file extension, which could cause issues with file inclusion.
- #include "pika_inner_message.pb. h"
+ #include "pika_inner_message.pb.h"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include "pika_inner_message.pb. h" | |
| #include "pika_inner_message.pb.h" |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/pika_conf.cc (2 hunks)
- src/pika_server.cc (1 hunks)
- src/storage/include/storage/storage.h (1 hunks)
- src/storage/src/redis.cc (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_conf.cc
Additional comments not posted (4)
src/storage/src/redis.cc (2)
314-317: LGTM!The usage of the
write_aggregated_int_propertyandwrite_propertylambda functions for reporting various RocksDB properties is consistent and clear. The refactoring improves readability and maintainability.Also applies to: 320-321, 324-324, 327-329, 332-332, 335-335, 338-338, 341-342, 345-345, 348-354, 355-361, 362-363, 366-366, 369-371, 374-377, 379-381
383-465: LGTM!The addition of the
write_ticker_countlambda function for reporting various RocksDB ticker counts is well-structured and enhances the monitoring capabilities. The usage is consistent and clear.src/storage/include/storage/storage.h (1)
72-73: LGTM!The new members
db_statistics_levelandenable_db_statisticsare correctly added and initialized in theStorageOptionsstruct. This enhances the configuration capabilities as intended.src/pika_server.cc (1)
1419-1421: Ensure consistency with other configuration options.The changes to enable and set the level of database statistics are correctly implemented. However, ensure that these options are consistently applied across the codebase and do not conflict with other configuration settings.
Verification successful
Changes are consistent and correctly implemented.
The configuration options
enable_db_statisticsanddb_statistics_levelare consistently applied across the codebase without any conflicts or inconsistencies.
src/pika_conf.ccsrc/pika_admin.ccsrc/pika_server.ccsrc/storage/include/storage/storage.hsrc/storage/src/redis.ccinclude/pika_conf.hScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of the `enable_db_statistics` and `db_statistics_level` options across the codebase. # Test: Search for the usage of these options. Expect: Consistent usage across the codebase. rg --type cpp -A 5 $'enable_db_statistics' rg --type cpp -A 5 $'db_statistics_level'Length of output: 6377
| auto write_aggregated_int_property=[&](const Slice& property, const char *metric) { | ||
| uint64_t value = 0; | ||
| db_->GetAggregatedIntProperty(property, &value); | ||
| string_stream << prefix << metric << ':' << value << "\r\n"; | ||
| }; |
There was a problem hiding this comment.
Ensure proper error handling for GetAggregatedIntProperty.
The lambda function write_aggregated_int_property does not handle potential errors from db_->GetAggregatedIntProperty. Consider adding error handling to ensure robustness.
- db_->GetAggregatedIntProperty(property, &value);
+ if (!db_->GetAggregatedIntProperty(property, &value)) {
+ // Handle error appropriately, e.g., log the error or set a default value
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto write_aggregated_int_property=[&](const Slice& property, const char *metric) { | |
| uint64_t value = 0; | |
| db_->GetAggregatedIntProperty(property, &value); | |
| string_stream << prefix << metric << ':' << value << "\r\n"; | |
| }; | |
| auto write_aggregated_int_property=[&](const Slice& property, const char *metric) { | |
| uint64_t value = 0; | |
| if (!db_->GetAggregatedIntProperty(property, &value)) { | |
| // Handle error appropriately, e.g., log the error or set a default value | |
| } | |
| string_stream << prefix << metric << ':' << value << "\r\n"; | |
| }; |
| auto write_ticker_count = [&](uint32_t tick_type, const char *metric) { | ||
| if (db_statistics_ == nullptr) { | ||
| return; | ||
| } | ||
| uint64_t count = db_statistics_->getTickerCount(tick_type); | ||
| string_stream << prefix << metric << ':' << count << "\r\n"; |
There was a problem hiding this comment.
Ensure proper error handling for getTickerCount.
The lambda function write_ticker_count does not handle potential errors from db_statistics_->getTickerCount. Consider adding error handling to ensure robustness.
- uint64_t count = db_statistics_->getTickerCount(tick_type);
+ uint64_t count = 0;
+ if (db_statistics_) {
+ count = db_statistics_->getTickerCount(tick_type);
+ } else {
+ // Handle error appropriately, e.g., log the error or set a default value
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto write_ticker_count = [&](uint32_t tick_type, const char *metric) { | |
| if (db_statistics_ == nullptr) { | |
| return; | |
| } | |
| uint64_t count = db_statistics_->getTickerCount(tick_type); | |
| string_stream << prefix << metric << ':' << count << "\r\n"; | |
| auto write_ticker_count = [&](uint32_t tick_type, const char *metric) { | |
| if (db_statistics_ == nullptr) { | |
| return; | |
| } | |
| uint64_t count = 0; | |
| if (db_statistics_) { | |
| count = db_statistics_->getTickerCount(tick_type); | |
| } else { | |
| // Handle error appropriately, e.g., log the error or set a default value | |
| } | |
| string_stream << prefix << metric << ':' << count << "\r\n"; |
| auto write_property=[&](const Slice& property, const char *metric) { | ||
| if (handles_.size() == 0) { | ||
| std::string value; | ||
| db_->GetProperty(db_->DefaultColumnFamily(), property, &value); | ||
| string_stream << prefix << metric << "_" << db_->DefaultColumnFamily()->GetName() << ':' << value << "\r\n"; | ||
| } else { | ||
| for (auto handle : handles_) { | ||
| std::string value; | ||
| db_->GetProperty(handle, property, &value); | ||
| string_stream << prefix << metric << "_" << handle->GetName() << ':' << value << "\r\n"; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Ensure proper error handling for GetProperty.
The lambda function write_property does not handle potential errors from db_->GetProperty. Consider adding error handling to ensure robustness.
- db_->GetProperty(handle, property, &value);
+ if (!db_->GetProperty(handle, property, &value).ok()) {
+ // Handle error appropriately, e.g., log the error or set a default value
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto write_property=[&](const Slice& property, const char *metric) { | |
| if (handles_.size() == 0) { | |
| std::string value; | |
| db_->GetProperty(db_->DefaultColumnFamily(), property, &value); | |
| string_stream << prefix << metric << "_" << db_->DefaultColumnFamily()->GetName() << ':' << value << "\r\n"; | |
| } else { | |
| for (auto handle : handles_) { | |
| std::string value; | |
| db_->GetProperty(handle, property, &value); | |
| string_stream << prefix << metric << "_" << handle->GetName() << ':' << value << "\r\n"; | |
| } | |
| } | |
| }; | |
| auto write_property=[&](const Slice& property, const char *metric) { | |
| if (handles_.size() == 0) { | |
| std::string value; | |
| if (!db_->GetProperty(db_->DefaultColumnFamily(), property, &value).ok()) { | |
| // Handle error appropriately, e.g., log the error or set a default value | |
| } else { | |
| string_stream << prefix << metric << "_" << db_->DefaultColumnFamily()->GetName() << ':' << value << "\r\n"; | |
| } | |
| } else { | |
| for (auto handle : handles_) { | |
| std::string value; | |
| if (!db_->GetProperty(handle, property, &value).ok()) { | |
| // Handle error appropriately, e.g., log the error or set a default value | |
| } else { | |
| string_stream << prefix << metric << "_" << handle->GetName() << ':' << value << "\r\n"; | |
| } | |
| } | |
| } | |
| }; |
related issue #2685
1.配置中增加 open_rocksdb_statistics_tickers ，默认为no。开启会损耗+1.5%
2.info rocksdb统计 statistics统计
Summary by CodeRabbit
New Features
Improvements
Bug Fixes