-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Added a graph of command time statistics to grafana #1751
Changes from 2 commits
fa43668
c47e4b5
5600c4c
cc02a6c
9078514
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -742,6 +742,8 @@ void InfoCmd::Do(std::shared_ptr<Slot> slot) { | |
info.append("\r\n"); | ||
InfoExecCount(info); | ||
info.append("\r\n"); | ||
InfoCommandStats(info); | ||
info.append("\r\n"); | ||
InfoCPU(info); | ||
info.append("\r\n"); | ||
InfoReplication(info); | ||
|
@@ -1254,15 +1256,17 @@ void InfoCmd::InfoCommandStats(std::string& info) { | |
tmp_stream.precision(2); | ||
tmp_stream.setf(std::ios::fixed); | ||
tmp_stream << "# Commandstats" << "\r\n"; | ||
for (auto& iter : *g_pika_server->GetCommandStatMap()) { | ||
if (iter.second.cmd_count != 0) { | ||
tmp_stream << "cmdstat_" << iter.first << ":" | ||
<< "calls=" << iter.second.cmd_count << ",usec=" | ||
<< iter.second.cmd_time_consuming | ||
<< ",usec_per_call=" | ||
<< static_cast<double>(iter.second.cmd_time_consuming) / static_cast<double>(iter.second.cmd_count) | ||
<< "\r\n"; | ||
} | ||
for (auto iter : *g_pika_server->GetCommandStatMap()) { | ||
tmp_stream << iter.first << ": " | ||
<< "calls=" << iter.second.cmd_count << ", usec=" | ||
<< iter.second.cmd_time_consuming | ||
<< ", usec_per_call="; | ||
if (static_cast<double>(iter.second.cmd_time_consuming) == 0) { | ||
tmp_stream << 0 << "\r\n"; | ||
} else { | ||
tmp_stream << static_cast<double>(iter.second.cmd_time_consuming) / static_cast<double>(iter.second.cmd_count) | ||
<< "\r\n"; | ||
} | ||
} | ||
info.append(tmp_stream.str()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall, the code patch you provided seems fine and does not contain any obvious bugs. However, there are a couple of improvement suggestions:
Here's an updated version of the code that incorporates these suggestions: void InfoCmd::InfoCommandStats(std::string& info) {
std::ostringstream tmp_stream;
tmp_stream.precision(2);
tmp_stream.setf(std::ios::fixed);
tmp_stream << "# Commandstats" << "\r\n";
for (const auto& iter : *g_pika_server->GetCommandStatMap()) {
if (iter.second.cmd_count != 0) {
tmp_stream << iter.first << ":"
<< "calls=" << iter.second.cmd_count
<< ", usec=" << iter.second.cmd_time_consuming / 1000.0
<< ", usec_per_call="
<< (iter.second.cmd_time_consuming / 1000.0) / iter.second.cmd_count
<< "\r\n";
}
}
info.append(tmp_stream.str());
} Remember to apply consistent formatting and ensure appropriate error handling and boundary checks for your specific use case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code review of the given patch is as follows:
Remember that the provided review is based on a brief analysis, and a deeper understanding may require more context about the codebase and its specific requirements. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package metrics | ||
|
||
import "regexp" | ||
|
||
func init() { | ||
Register(collectCommandstatsMetrics) | ||
} | ||
|
||
var collectCommandstatsMetrics = map[string]MetricConfig{ | ||
"commandstats_info": { | ||
Parser: Parsers{ | ||
&versionMatchParser{ | ||
Parser: ®exParser{ | ||
name: "commandstats_info", | ||
reg: regexp.MustCompile(`(?P<cmd>[\w]+)[:\s]*calls=(?P<calls>[\d]+)[,\s]*usec=(?P<usec>[\d]+)[,\s]*usec_per_call=(?P<usec_per_call>[\d]+)`), | ||
Parser: &normalParser{}, | ||
}, | ||
}, | ||
}, | ||
MetricMeta: MetaDatas{ | ||
{ | ||
Name: "commandstats_info", | ||
Help: "pika Average time taken to execute commands", | ||
Type: metricTypeGauge, | ||
Labels: []string{LabelNameAddr, LabelNameAlias, "cmd", "calls", "usec", "usec_per_call"}, | ||
ValueName: "commandstats_info", | ||
}, | ||
}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the code patch you provided seems to address the addition of a new function called
InfoCommandStats
and the modification of the existing functionvoid InfoCmd::Do(std::shared_ptr<Slot> slot)
.Here are a few points to consider for your code review:
Naming: The function name
InfoCommandStats
suggests that it provides information related to command statistics. Make sure the function accurately represents its purpose.String Concatenation: Instead of repeated
info.append("\r\n")
, you can useinfo += "\r\n"
for brevity and readability.Code Formatting: Maintain consistent code formatting for better readability. In the code you provided, indentation is inconsistent. Ensure proper indentation throughout the code.
Iteration: In the for loop, consider using a reference to avoid unnecessary object copying. Change
auto iter
toauto& iter
in the for loop declaration.Avoid Unnecessary Cast: In the line
static_cast<double>(iter.second.cmd_time_consuming) == 0
, casting the value to double to compare with zero is not necessary. You can directly compare it with zero without casting.Division by Zero: Be cautious about potential division by zero. Make sure to handle the case where
iter.second.cmd_count
is zero to avoid division by zero. In the updated code, a check has been implemented to handle this scenario.Code Documentation: Consider adding comments or documentation to clarify the purpose and functionality of the added code, especially if it's complex or not immediately obvious.
Remember to thoroughly test your code changes before deploying them to ensure they work as intended and do not introduce any unexpected bugs.