feat: add log-net-activities configuration #2964
Conversation
* fix authrequired bug * init pika.conf * fix InitUser * fix InitUser * fix no userblacklist * fix acl * delete annotation * fix bug
WalkthroughThe pull request introduces significant updates to the Pika configuration and logging system. A new parameter, Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
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: 3
🧹 Outside diff range and nitpick comments (11)
include/pika_dispatch_thread.h (1)
22-22: Add documentation for the new SetLogLevel method.Please add a documentation comment explaining:
- The purpose of this method
- The valid range of log level values
- The behavior at each log level
+ /** + * Sets the logging level for the dispatch thread. + * @param value The log level (0: minimal logging, 1: verbose logging) + */ void SetLogLevel(int32_t value);src/net/include/server_thread.h (3)
131-131: Add documentation for the SetLogLevel method.Please add a documentation comment explaining the method's purpose and valid values.
+ /** + * Sets the logging level for connection activities. + * @param value The log level (0: minimal logging, 1: verbose logging) + */ void SetLogLevel(int32_t value);
172-172: Consider grouping configuration-related members together.The log_level_ member would be better placed with other configuration-related members near the top of the protected section, rather than being isolated.
protected: /* * The event handler */ std::unique_ptr<NetMultiplexer> net_multiplexer_; + + // Configuration + std::atomic<int32_t> log_level_{0}; - - std::atomic<int32_t> log_level_{0};
172-172: Add documentation for the log_level_ member variable.Please document the purpose and valid values of the log_level_ member.
+ /** + * Controls the verbosity of connection activity logging. + * 0: Minimal logging (default) + * 1: Verbose logging of connection activities + */ std::atomic<int32_t> log_level_{0};src/net/src/server_thread.cc (1)
267-272: Consider using enum or constants for log levels.The implementation is thread-safe using atomic operations and has proper input validation. However, the magic numbers 0 and 1 could be replaced with named constants or an enum for better maintainability and self-documentation.
Consider this improvement:
+// At the top of the file or in a header +enum class LogLevel { + MINIMAL = 0, // No connection activity logs + VERBOSE = 1 // Log connection activities +}; void ServerThread::SetLogLevel(int32_t value) { - if (value != 0 && value != 1) { + if (value != static_cast<int32_t>(LogLevel::MINIMAL) && + value != static_cast<int32_t>(LogLevel::VERBOSE)) { return; } log_level_.store(value); }src/net/src/dispatch_thread.cc (2)
151-153: Consider using structured logging format.The conditional logging implementation is correct, but the log message could be more structured for better parsing.
Consider using key-value pairs in the log message:
- LOG(INFO) << "accept new conn " << ti.String(); + LOG(INFO) << "event=new_connection conn=" << ti.String();
161-163: Consider extracting repeated log level check.The log level check is duplicated. Consider extracting it to a helper method for better maintainability.
Consider this improvement:
+bool DispatchThread::ShouldLog() const { + return log_level_ == 1; +} - if (log_level_ == 1) { + if (ShouldLog()) { LOG(INFO) << "find worker(" << next_thread << "), refresh the last_thread_ to " << last_thread_; }include/pika_server.h (1)
495-495: Add method documentation.Consider adding documentation to describe the purpose of the method and the meaning of the parameter value.
Add documentation like this:
+ /* + * Sets the verbosity level for connection activity logging + * @param value: 0 for minimal logging (no connection activities) + * 1 for verbose logging (includes connection activities) + */ void SetLogLevel(int32_t value);conf/pika.conf (1)
77-81: Enhance documentation of the log-level parameterThe documentation clearly explains the basic usage, but consider adding:
- Whether the parameter can be changed at runtime using
CONFIG SET- Thread-safety considerations when changing the value
- Impact on performance and disk usage for each level
src/pika_admin.cc (1)
2481-2488: LGTM with a suggestion for improvement.The validation and error handling look good. Consider defining constants for the valid log levels (0 and 1) to improve maintainability.
+// At the top of the file or in a header +static const int32_t LOG_LEVEL_NONE = 0; +static const int32_t LOG_LEVEL_VERBOSE = 1; // In the code -if ((pstd::string2int(value.data(), value.size(), &ival) == 0) || (ival != 0 && ival != 1)) { +if ((pstd::string2int(value.data(), value.size(), &ival) == 0) || + (ival != LOG_LEVEL_NONE && ival != LOG_LEVEL_VERBOSE)) {src/acl.cc (1)
480-480: LGTM! Consider adding password validation.The password handling is secure, using SHA-256 hashing. However, consider adding input validation for the password string to enforce security requirements (e.g., minimum length, complexity).
Example validation that could be added before the current line:
void Acl::UpdateDefaultUserPassword(const std::string& pass) { std::unique_lock wl(mutex_); auto u = GetUser(DefaultUser); u->SetUser("resetpass"); if (pass.empty()) { u->SetUser("nopass"); } else { + if (pass.length() < 8) { + LOG(WARNING) << "Password is too short, recommended minimum length is 8 characters"; + } u->SetUser(">" + pass); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
conf/pika.conf(1 hunks)include/pika_conf.h(3 hunks)include/pika_dispatch_thread.h(1 hunks)include/pika_server.h(1 hunks)src/acl.cc(1 hunks)src/net/include/server_thread.h(2 hunks)src/net/src/dispatch_thread.cc(2 hunks)src/net/src/server_thread.cc(1 hunks)src/pika_admin.cc(4 hunks)src/pika_conf.cc(1 hunks)src/pika_dispatch_thread.cc(1 hunks)src/pika_server.cc(2 hunks)
🔇 Additional comments (6)
src/net/include/server_thread.h (1)
131-131: Verify the handling of log levels across the codebase.
The implementation assumes binary logging levels (0 or 1), but this constraint isn't explicitly enforced. Let's verify how log levels are used across the codebase.
Also applies to: 172-172
✅ Verification successful
Based on the results, I can now generate the final response:
Log level implementation is correctly using binary values (0/1) throughout the codebase
The verification shows that:
- The log level is consistently initialized to 0 in both
server_thread.handpika_conf.h - The only usage of log level comparison in
dispatch_thread.ccexplicitly checks for value 1 (if (log_level_ == 1)) - The implementation is thread-safe using
std::atomicfor storage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where log_level_ is used to ensure consistent handling
rg -A 3 "log_level_"
# Search for any hardcoded log level values
ast-grep --pattern 'SetLogLevel($value)'
Length of output: 1608
src/net/src/dispatch_thread.cc (1)
196-196: Consider using ERROR level judiciously.
The log level has been changed from WARNING to ERROR. Ensure this truly represents an error condition that requires immediate attention rather than just a warning.
Let's verify the impact of this error condition:
✅ Verification successful
Based on the context, I can now generate the final response:
ERROR level is appropriate for this blocking operation failure
The log message indicates a critical failure in the blocking operation mechanism of blpop/brpop commands. This is correctly logged as ERROR because:
- It represents a state corruption in the blocking operation tracking system (blocked_conn_to_keys_ map)
- The error causes the blocking functionality to "not work correctly" as stated in the message
- This impacts core Redis-compatible functionality where clients depend on reliable blocking operations
- The code immediately returns after this error, abandoning the cleanup operation
The ERROR level is justified as this represents a system-level failure that affects functionality correctness rather than just a warning condition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related error handling code
rg -A 5 "blocking info of blpop/brpop went wrong"
Length of output: 517
Script:
#!/bin/bash
# Let's get more context around this code to understand the error condition
rg -B 10 -A 10 "blocking info of blpop/brpop went wrong"
# Also search for other usages of blpop/brpop to understand the criticality
rg "blpop|brpop" -A 2
Length of output: 53460
include/pika_conf.h (1)
1094-1094: LGTM!
The atomic variable is correctly declared and initialized with the default value matching the configuration.
src/pika_server.cc (1)
1923-1923: LGTM!
Simple and clean implementation of the log level setter that delegates to the dispatch thread.
src/pika_admin.cc (2)
1593-1598: LGTM!
Clean implementation of log-level config retrieval that follows the existing pattern for other parameters.
3329-3331: LGTM!
Good addition of conditional logging that helps reduce log noise while still providing useful connection info when needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/pika_conf.cc (1)
135-143: LGTM! Consider adding documentation for log levels.The implementation correctly loads and validates the log-level configuration, with proper thread-safe storage using atomic variables. The validation ensures the value is either 0 or 1, with appropriate error logging and fallback to 0 for invalid values.
Consider adding a comment to document the meaning of each log level:
+ // log-level controls connection activity logging in the dispatcher thread: + // 0: Disable connection activity logging + // 1: Enable logging of new client connections int32_t log_level = 0; GetConfInt("log-level", &log_level);
# Conflicts: # src/pika_admin.cc
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
include/pika_dispatch_thread.h (1)
22-22: Add documentation for the new SetLogLevel methodPlease add documentation to describe:
- Purpose of the method
- Parameter description and valid values
- Thread-safety guarantees
+ /** + * Sets the logging level for the dispatch thread. + * @param new_mode The new logging mode (DEBUG=0 or NORMAL=1) + * @thread-safety This method is thread-safe + */ void SetLogLevel(net::LogMode new_mode);src/net/include/server_thread.h (1)
131-131: Add documentation for the SetLogLevel methodPlease add documentation to describe the method's purpose and thread-safety guarantees.
+ /** + * Sets the logging level for the server thread. + * @param new_mode The new logging mode (DEBUG=0 or NORMAL=1) + * @thread-safety This method is thread-safe + */ void SetLogLevel(net::LogMode new_mode);src/pika_admin.cc (1)
3325-3327: Consider enhancing the log messageThe debug log message could be more descriptive.
- LOG(INFO) << "QutCmd will close connection " << GetConn()->String(); + LOG(INFO) << "QuitCmd closing connection: " << GetConn()->String();include/pika_conf.h (1)
89-91: Consider adding documentation for logging modesAdd documentation comments to describe:
- Purpose and behavior of each logging mode
- Impact on system performance
- Typical use cases for each mode
+ /** + * Get the current logging mode. + * DEBUG: Logs all connection activities in dispatcher thread + * NORMAL: Minimal logging for better performance + * @return Current logging mode + */ net::LogMode logging_mode() { return logging_mode_.load(std::memory_order::memory_order_relaxed); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
conf/pika.conf(2 hunks)include/pika_conf.h(3 hunks)include/pika_dispatch_thread.h(1 hunks)include/pika_server.h(1 hunks)src/net/include/net_define.h(1 hunks)src/net/include/server_thread.h(2 hunks)src/net/src/dispatch_thread.cc(2 hunks)src/net/src/server_thread.cc(1 hunks)src/pika_admin.cc(3 hunks)src/pika_conf.cc(1 hunks)src/pika_dispatch_thread.cc(1 hunks)src/pika_server.cc(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/net/src/server_thread.cc
- src/pika_dispatch_thread.cc
- src/net/src/dispatch_thread.cc
- conf/pika.conf
🔇 Additional comments (10)
src/net/include/server_thread.h (1)
172-172: Verify thread-safety of logging mode changes
The use of std::atomic for logging_mode_ is good for thread safety. However, please ensure that all read operations in the implementation also handle the atomic variable correctly.
✅ Verification successful
Let me search for the actual usage of these atomic variables to verify thread safety.
Thread-safety of logging mode operations is properly implemented
All usages of the atomic logging_mode_ variable are thread-safe:
- All stores use
std::memory_order::memory_order_relaxed - All loads use
std::memory_order::memory_order_relaxed - No direct access to the atomic variable without proper atomic operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct reads of logging_mode_ that might not handle atomic properly
rg -l "logging_mode_" | xargs rg "logging_mode_\s*[^=\.]" -A 2
Length of output: 365
Script:
#!/bin/bash
# Search for all usages of logging_mode_ to check read operations
rg "logging_mode_" -B 2 -A 2
# Also search for any setter/getter methods related to logging mode
ast-grep --pattern 'LogMode Get$$$'
ast-grep --pattern 'Set$$$LogMode'
Length of output: 3084
src/pika_server.cc (2)
200-200: LGTM: Proper initialization of logging mode during server startup
The logging mode is correctly initialized from configuration during server startup.
1923-1923: LGTM: Clean implementation of SetLogLevel method
The method follows good practices by delegating to the appropriate component.
src/pika_admin.cc (2)
1593-1599: LGTM: Clean implementation of logging-mode config retrieval
The code correctly handles the "logging-mode" parameter in ConfigGet, with proper output formatting and value mapping.
2476-2484: LGTM: Robust implementation of logging-mode config setting
The code includes:
- Input validation for allowed values
- Clear error messages
- Proper state updates for both config and server
include/pika_server.h (1)
495-495: LGTM: Method signature follows class conventions
The SetLogLevel method signature is well-designed with:
- Appropriate access level for configuration management
- Type-safe parameter using LogMode enum
- Consistent naming with other setter methods
include/pika_conf.h (3)
89-91: LGTM: Thread-safe getter with appropriate memory ordering
The getter implementation is well-designed:
- Uses atomic for thread-safety
- Appropriate use of relaxed memory ordering for log level reads
- Consistent with getter patterns in the class
828-834: LGTM: Well-implemented setter with proper validation
The setter implementation is robust:
- Validates input values
- Updates configuration state
- Thread-safe atomic store
- Proper error handling for invalid values
1097-1097: LGTM: Well-designed member variable
The member variable is properly defined:
- Uses atomic for thread-safety
- Appropriate default value
- Strong typing with LogMode enum
src/pika_conf.cc (1)
135-146: LGTM: Robust configuration loading implementation
The configuration loading code is well-implemented:
- Proper validation of input values
- Clear error logging for invalid configurations
- Safe fallback to default mode
- Consistent with existing configuration patterns
cheniujh
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
include/pika_conf.h (1)
Line range hint
77-81: Consider future logging framework integrationWhile the current implementation is clean and focused, consider future integration with a more comprehensive logging framework that could provide:
- Multiple log levels (DEBUG, INFO, WARN, ERROR)
- Category-based logging control
- Runtime log level adjustment
This would allow for more granular control over different types of logs while maintaining the simplicity of the current network activity logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
conf/pika.conf(2 hunks)include/pika_conf.h(3 hunks)include/pika_dispatch_thread.h(1 hunks)include/pika_server.h(1 hunks)src/net/include/server_thread.h(2 hunks)src/net/src/dispatch_thread.cc(2 hunks)src/net/src/server_thread.cc(1 hunks)src/pika_admin.cc(3 hunks)src/pika_conf.cc(1 hunks)src/pika_dispatch_thread.cc(1 hunks)src/pika_server.cc(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/net/src/server_thread.cc
- src/pika_dispatch_thread.cc
- include/pika_server.h
- include/pika_dispatch_thread.h
- src/net/src/dispatch_thread.cc
- src/pika_server.cc
- src/net/include/server_thread.h
🔇 Additional comments (8)
conf/pika.conf (1)
77-81: LGTM: Well-documented configuration parameter
The new log-net-activities parameter is well documented with:
- Clear explanation of valid values (yes/no)
- Default value specification
- Note about dynamic configuration capability
src/pika_conf.cc (2)
135-143: LGTM: Thread-safe configuration loading
The implementation correctly:
- Reads the string value from config
- Converts to boolean with proper validation
- Uses atomic storage for thread safety
Line range hint 833-839: LGTM: Thread-safe setter implementation
The setter implementation correctly:
- Tracks config changes for rewrite
- Updates the atomic boolean value
- Handles value validation
include/pika_conf.h (2)
89-91: LGTM: Efficient thread-safe getter
The getter implementation correctly:
- Uses atomic load with relaxed memory ordering
- Simple and focused purpose
1102-1102: LGTM: Thread-safe member variable
The atomic boolean member is properly:
- Declared as atomic for thread safety
- Initialized to false by default
src/pika_admin.cc (3)
1599-1604: LGTM! New config parameter properly implemented
The implementation of the 'log-net-activities' config parameter follows the established pattern for boolean configs and is correctly formatted.
2495-2503: LGTM! ConfigSet implementation is complete and correct
The implementation:
- Properly validates input values ("yes"/"no")
- Updates both configuration and runtime state
- Provides clear error messages
3357-3359: LGTM! Connection logging properly implemented
The implementation adds informative logging for client disconnections, properly guarded by the log-net-activities configuration parameter.
cheniujh
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pika_conf.cc (1)
136-142: Consider improving string comparison and removing unnecessary semicolon.The implementation could be more consistent with other boolean parameters in the codebase:
- Consider case-insensitive comparison for "yes"
- Remove the unnecessary semicolon after the if-else block
std::string log_net_activities; GetConfStr("log-net-activities", &log_net_activities); + pstd::StringToLower(log_net_activities); - if (log_net_activities == "yes") { + if (log_net_activities == "yes" || log_net_activities == "1") { log_net_activities_.store(true); } else { log_net_activities_.store(false); - }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pika_conf.cc(3 hunks)
🔇 Additional comments (1)
src/pika_conf.cc (1)
136-142: Verify configuration persistence and access patterns.
The implementation looks thread-safe with atomic storage, but let's verify:
- All access points to this configuration
- Proper persistence of configuration changes
Also applies to: 841-843
✅ Verification successful
Configuration implementation is thread-safe and properly persisted
The verification shows a well-implemented configuration with:
- Thread-safe access using atomic variables with explicit memory ordering
- Proper persistence through
SetConfIntinpika_conf.cc - Consistent access patterns across the codebase:
- Network threads read using
load(memory_order_relaxed) - Configuration changes propagate through
g_pika_conf->SetLogNetActivitiesandg_pika_server->SetLogNetActivities - CONFIG SET command validates input ("yes"/"no") before applying changes
- Network threads read using
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of log_net_activities_ to ensure thread-safe access
echo "Searching for log_net_activities_ usage..."
rg "log_net_activities_" -A 3
# Search for config get/set command implementations
echo "Searching for config command implementations..."
rg -g "*.cc" -g "*.h" "CONFIG SET.*log-net-activities" -A 5
rg -g "*.cc" -g "*.h" "CONFIG GET.*log-net-activities" -A 5
Length of output: 2880
* add log-level to config if pika should log the activities of Net connctions --------- Co-authored-by: buzhimingyonghu <[email protected]>
在pika.conf中添加log-net-activities参数，该参数支持config get/set动态调整。
当该参数为no（默认值）：日志中不打印连接信息
当该参数为yes：当dispacher中有新客户端建立连接，会进行相关打印
当有业务采用短连接时，log-net-activities配置为no可以减少大量的日志打印。
Summary by CodeRabbit
Summary by CodeRabbit
New Features
log-net-activitiesfor configuring network activity logging.Bug Fixes
Documentation