-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added a graph of command time statistics to grafana #1751
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
Conversation
} 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 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 function void 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.
} | ||
} | ||
} | ||
info.append(tmp_stream.str()); | ||
} |
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 fine and does not contain any obvious bugs. However, there are a couple of improvement suggestions:
-
Formatting: The indentation in the patch appears inconsistent, which can make the code harder to read and maintain. Make sure to use consistent indentation throughout the code.
-
Clarify variable iteration with value reference: In the line
for (auto iter : *g_pika_server->GetCommandStatMap())
, it would be more efficient and clearer to iterate by reference usingauto&
instead of by value. This avoids unnecessary copying of map elements. -
Simplification of division by 1000: To convert microseconds to milliseconds, you can simplify the code by dividing
iter.second.cmd_time_consuming
by 1000 directly in the loop, rather than repeating the division in each usage. This improves both readability and performance.
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.
src/pika_admin.cc
Outdated
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) / 1000.0) / static_cast<double>(iter.second.cmd_count) |
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.
可以封装一个函数
} | ||
} | ||
} | ||
info.append(tmp_stream.str()); | ||
} |
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.
The code review of the given patch is as follows:
-
MethodofCommandStatistics():
- It calculates the average time per command based on the time-consuming and frequency.
- This method seems fine, but it would be better to provide a more descriptive name for this function.
-
MethodofTotalTimeCalculation():
- It calculates the total time in seconds based on the time-consuming.
- This method seems fine, but it would be better to provide a more descriptive name for this function.
-
InfoCmd::InfoCommandStats():
- The loop iterating over
*g_pika_server->GetCommandStatMap()
can be improved by using a reference parameter instead of creating a copy. - A check is added before printing the "usec_per_call" value to handle potential division by zero errors.
- The loop iterating over
-
Overall, the code looks good, and there are no apparent bug risks. However, consider the following suggestions for improvement:
- Clarify the naming conventions of the methods to make their purpose clearer.
- Add comments to explain the logic and purpose behind critical sections of code.
- Consider using the
const auto&
syntax when iterating over containers if the objects are not modified. - Consider separating concerns into smaller, reusable functions to improve readability and maintainability.
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.
@@ -203,3 +209,5 @@ Screenshots: | |||
 | |||
|
|||
 | |||
|
|||
 |
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.
Based on the code patch you provided, here are a few observations:
- It looks like you're adding new metrics for Pika command execution time and RocksDB metrics to your Prometheus configuration.
- For the Pika Command Execution Time metrics, you have defined three counters:
calls
,usec
, andusec_per_call
with corresponding labels. Make sure that the metric values being recorded align with these definitions in your codebase. - The RocksDB metrics and their associated explanations look fine based on the provided code snippet.
- The screenshots you included at the end of the patch demonstrate the visual representation of the network, RocksDB, and commandstats metrics.
Without additional context or the actual code implementation, it's difficult to identify specific bugs or suggest improvements. However, ensure that the metrics are correctly implemented, labeled, and associated with the appropriate data sources or calculations. Additionally, consider verifying that the Prometheus configuration interacts correctly with your monitoring stack and tools.
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.
LGTM
…on#1751) * Added a graph of command time statistics to grafana
…on#1751) * Added a graph of command time statistics to grafana
Added a graph of command time statistics to grafana
1.Add averge milliseconds of the command time

2.Add Total milliseconds of the command

3.Add total number of commands
