Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added command time statistics #1660

Merged
merged 3 commits into from
Jul 4, 2023
Merged

Added command time statistics #1660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
55995e6
Optimized code format
Mixficsol Jul 4, 2023
4d87d3b
Add atomic variables to avoid thread collisions
Mixficsol Jul 4, 2023
91ba4f8
Reformat code
Mixficsol Jul 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion include/pika_admin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ class InfoCmd : public Cmd {
kInfoRocksDB,
kInfo,
kInfoAll,
kInfoDebug
kInfoDebug,
kInfoCommandStats
};

InfoCmd(const std::string& name, int arity, uint16_t flag) : Cmd(name, arity, flag) {}
Expand All @@ -237,6 +238,7 @@ class InfoCmd : public Cmd {
const static std::string kDataSection;
const static std::string kRocksDBSection;
const static std::string kDebugSection;
const static std::string kCommandStatsSection;

void DoInitial() override;
void Clear() override {
Expand All @@ -256,6 +258,7 @@ class InfoCmd : public Cmd {
void InfoData(std::string& info);
void InfoRocksDB(std::string& info);
void InfoDebug(std::string& info);
void InfoCommandStats(std::string& info);
};

class ShutdownCmd : public Cmd {
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
1 change: 1 addition & 0 deletions include/pika_cmd_table_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class PikaCmdTableManager {
std::shared_ptr<Cmd> GetCmd(const std::string& opt);
uint32_t DistributeKey(const std::string& key, uint32_t slot_num);
bool CmdExist(const std::string& cmd) const;
CmdTable* GetCmdTable();

private:
std::shared_ptr<Cmd> NewCommand(const std::string& opt);
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
10 changes: 10 additions & 0 deletions include/pika_command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,16 @@ class Cmd : public std::enable_shared_from_this<Cmd> {
std::shared_ptr<SyncMasterSlot> sync_slot;
HintKeys hint_keys;
};
struct CommandStatistics {
CommandStatistics() = default;
CommandStatistics(const CommandStatistics& other) {
cmd_time_consuming.store(other.cmd_time_consuming.load());
cmd_count.store(other.cmd_count.load());
}
std::atomic<int32_t> cmd_count = {0};
std::atomic<int32_t> cmd_time_consuming = {0};
};
CommandStatistics state;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CommandStatistics state;这个要塞进Cmd这个类里

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CommandStatistics state;这个要塞进Cmd这个类里

这个是在Cmd这个基类里面的，放在外面的话会访问不到

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

默认值填一下，默认0

Cmd(std::string name, int arity, uint16_t flag) : name_(std::move(name)), arity_(arity), flag_(flag) {}
virtual ~Cmd() = default;
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved

Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
23 changes: 23 additions & 0 deletions src/pika_admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ using pstd::Status;

extern PikaServer* g_pika_server;
extern std::unique_ptr<PikaReplicaManager> g_pika_rm;
extern std::unique_ptr<PikaCmdTableManager> g_pika_cmd_table_manager;

static std::string ConstructPinginPubSubResp(const PikaCmdArgsType& argv) {
if (argv.size() > 2) {
Expand Down Expand Up @@ -633,6 +634,7 @@ const std::string InfoCmd::kKeyspaceSection = "keyspace";
const std::string InfoCmd::kDataSection = "data";
const std::string InfoCmd::kRocksDBSection = "rocksdb";
const std::string InfoCmd::kDebugSection = "debug";
const std::string InfoCmd::kCommandStatsSection = "commandstats";

void InfoCmd::DoInitial() {
size_t argc = argv_.size();
Expand Down Expand Up @@ -701,6 +703,8 @@ void InfoCmd::DoInitial() {
info_section_ = kInfoRocksDB;
} else if (strcasecmp(argv_[1].data(), kDebugSection.data()) == 0) {
info_section_ = kInfoDebug;
} else if (strcasecmp(argv_[1].data(), kCommandStatsSection.data()) == 0) {
info_section_ = kInfoCommandStats;
} else {
info_section_ = kInfoErr;
}
Expand Down Expand Up @@ -776,6 +780,9 @@ void InfoCmd::Do(std::shared_ptr<Slot> slot) {
case kInfoDebug:
InfoDebug(info);
break;
case kInfoCommandStats:
InfoCommandStats(info);
break;
default:
// kInfoErr is nothing
break;
Expand Down Expand Up @@ -1212,6 +1219,22 @@ void InfoCmd::InfoDebug(std::string& info) {
g_pika_server->ServerStatus(&info);
}

void InfoCmd::InfoCommandStats(std::string& info) {
std::stringstream tmp_stream;
tmp_stream.precision(2);
tmp_stream.setf(std::ios::fixed);
tmp_stream << "# Commandstats" << "\r\n";
for (auto& iter : *g_pika_cmd_table_manager->GetCmdTable()) {
if (iter.second->state.cmd_count != 0) {
tmp_stream << "cmdstat_" << iter.first << ":"
<< "calls=" << iter.second->state.cmd_count << ",usec="
<< iter.second->state.cmd_time_consuming
<< ",usec_per_call=" << (iter.second->state.cmd_time_consuming * 1.0) / iter.second->state.cmd_count << "\r\n";
}
}
info.append(tmp_stream.str());
}

void ConfigCmd::DoInitial() {
if (!CheckArg(argv_.size())) {
res_.SetRes(CmdRes::kWrongNum, kCmdNameConfig);
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code patch seems to be adding functionality related to command statistics in the existing codebase. Here are some suggestions and improvements:

  1. It's generally a good practice to include relevant headers within the implementation file rather than relying on external includes through other files. Ensure that all necessary headers are included directly in the source file InfoCmd.cc.

  2. When using external variables like g_pika_server, g_pika_rm, and g_cmdstat_map, it is recommended to use forward declarations in header files instead of including the actual headers. This helps reduce unnecessary dependencies and compilation times.

  3. Consider encapsulating the global g_cmdstat_map within a class or namespace to avoid potential name clashes and improve code organization.

  4. The usage of std::stringstream in InfoCommandstats could be replaced with std::string concatenation for simpler code. For example, you can change:

tmp_stream << "cmdstat_" << iter->second.cmd_name << ":"
   << "calls=" << iter->second.cmd_count << ",usec="
   << iter->second.cmd_time_consuming
   << ",usec_per_call=" << (iter->second.cmd_time_consuming * 1.0) / iter->second.cmd_count << "\r\n";

to

tmp_stream += "cmdstat_" + iter->second.cmd_name + ":";
tmp_stream += "calls=" + std::to_string(iter->second.cmd_count) + ",usec=";
tmp_stream += std::to_string(iter->second.cmd_time_consuming);
tmp_stream += ",usec_per_call=";
tmp_stream += std::to_string((iter->second.cmd_time_consuming * 1.0) / iter->second.cmd_count) + "\r\n";

  1. Make sure proper error handling and input validation are performed in the code, especially when processing argv_ and accessing elements within it. Ensure that all possible edge cases are properly handled and invalid input does not lead to unexpected behavior or crashes.

  2. Consider adding comments to clearly explain the purpose and functionality of each function and section of the code to improve code readability and maintainability.

  3. Perform thorough testing of the modified functionality to ensure the correct behavior of the added command statistics feature.

These suggestions should help improve the code's quality and reduce potential bugs. However, without a complete understanding of the entire codebase and its requirements, it's difficult to identify all possible issues or improvements.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chatgpt说的对

Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some observations and suggestions for the code patch:

  1. Include Order: Ensure that the included headers are in the correct order.

  2. Variable Naming: Use more descriptive variable names to improve code readability. For example, iter can be renamed to something like command_stat_iterator.

  3. Commandstats Section: The implementation of the InfoCommandstats function is incomplete. It references a CmdTable, but it doesn't seem to be initialized or populated with data. Make sure that the cmdstat_map is properly initialized before iterating over it.

  4. Omitted Code: Depending on the omitted code, it's possible that there are other functions or logic missing that affect the overall behavior of the program. Please ensure that all necessary code is present for a complete understanding of the program flow.

  5. Error Handling: It's important to handle any potential errors that may arise during the execution of the code. Make sure to add appropriate error handling and consider edge cases, such as when an invalid info section is provided.

  6. Commenting: Consider adding comments above functions or sections of code to provide a brief description of their purpose or functionality. This can help other developers understand the code more easily.

Remember to thoroughly test the code after making improvements or changes to ensure its correctness and reliability.

Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Mixficsol marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
4 changes: 4 additions & 0 deletions src/pika_client_conn.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ std::shared_ptr<Cmd> PikaClientConn::DoCmd(const PikaCmdArgsType& argv, const st

// Process Command
c_ptr->Execute();
int64_t duration = pstd::NowMicros() - start_us;
wanghenshui marked this conversation as resolved.
Show resolved Hide resolved
auto iter = g_pika_cmd_table_manager->GetCmdTable();
(*iter)[opt]->state.cmd_count.fetch_add(1);
(*iter)[opt]->state.cmd_time_consuming.fetch_add(duration);

if (g_pika_conf->slowlog_slower_than() >= 0) {
ProcessSlowlog(argv, start_us, c_ptr->GetDoDuration());
Expand Down
4 changes: 4 additions & 0 deletions src/pika_cmd_table_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ std::shared_ptr<Cmd> PikaCmdTableManager::NewCommand(const std::string& opt) {
return nullptr;
}

CmdTable* PikaCmdTableManager::GetCmdTable() {
return cmds_.get();
}

bool PikaCmdTableManager::CheckCurrentThreadDistributionMapExist(const std::thread::id& tid) {
std::shared_lock l(map_protector_);
return thread_distribution_map_.find(tid) != thread_distribution_map_.end();
Expand Down