github-actions[bot] commented on code in PR #61851:
URL: https://github.com/apache/doris/pull/61851#discussion_r3005900325
##########
be/src/util/pprof_utils.cpp:
##########
@@ -128,6 +128,30 @@ Status PprofUtils::get_readable_profile(const std::string&
file_or_content, bool
return Status::OK();
}
+Status PprofUtils::get_svg_profile(const std::string& profile_file,
std::string* output) {
+ std::string pprof_cmd;
+ RETURN_IF_ERROR(PprofUtils::get_pprof_cmd(&pprof_cmd));
Review Comment:
**Temp file leak on early error return**: If `get_pprof_cmd()` or
`get_self_cmdline()` (line 136) returns an error, `RETURN_IF_ERROR` will exit
the function without deleting `profile_file`. The caller
(`ProfileAction::handle`) has already created this temp file via
`ProfilerStart`/`ProfilerStop`.
I acknowledge this follows the same pattern as the existing
`get_readable_profile()` above, but since this is new code, consider cleaning
up the file on early exit, e.g.:
```cpp
auto cleanup = Defer([&profile_file]() {
static_cast<void>(io::global_local_filesystem()->delete_file(profile_file));
});
```
at the top of the function (and remove the explicit `delete_file` call at
line 145).
##########
be/src/util/pprof_utils.cpp:
##########
@@ -128,6 +128,30 @@ Status PprofUtils::get_readable_profile(const std::string&
file_or_content, bool
return Status::OK();
}
+Status PprofUtils::get_svg_profile(const std::string& profile_file,
std::string* output) {
+ std::string pprof_cmd;
+ RETURN_IF_ERROR(PprofUtils::get_pprof_cmd(&pprof_cmd));
+
+ std::string self_cmdline;
+ RETURN_IF_ERROR(PprofUtils::get_self_cmdline(&self_cmdline));
+
+ std::string cmd_output;
+ std::string final_cmd =
+ pprof_cmd + absl::Substitute(" --svg $0 $1", self_cmdline,
profile_file);
+ AgentUtils util;
+ LOG(INFO) << "begin to run command: " << final_cmd;
+ bool rc = util.exec_cmd(final_cmd, &cmd_output, false);
+
+
static_cast<void>(io::global_local_filesystem()->delete_file(profile_file));
+
+ if (!rc) {
+ return Status::InternalError("Failed to execute command: {}",
cmd_output);
+ }
+
+ *output = std::move(cmd_output);
+ return Status::OK();
+}
+
Status PprofUtils::generate_flamegraph(int32_t sample_seconds,
Review Comment:
**Dead code**: `generate_flamegraph()` is no longer called anywhere after
this PR removes its only call site in `pprof_actions.cpp`. Similarly,
`get_perf_cmd()` (defined at line 57) is only used inside
`generate_flamegraph()`. Both functions and their declarations in
`pprof_utils.h` should be removed to keep the codebase clean.
##########
be/src/service/http/action/pprof_actions.cpp:
##########
@@ -157,52 +157,43 @@ void ProfileAction::handle(HttpRequest* req) {
}
const std::string& type_str = req->param("type");
- if (type_str != "flamegraph") {
- // use pprof the sample the CPU
- std::ostringstream tmp_prof_file_name;
- tmp_prof_file_name << config::pprof_profile_dir << "/doris_profile."
<< getpid() << "."
- << rand();
- ProfilerStart(tmp_prof_file_name.str().c_str());
- sleep(seconds);
- ProfilerStop();
-
- if (type_str != "text") {
- // return raw content via http response directly
- std::ifstream prof_file(tmp_prof_file_name.str().c_str(),
std::ios::in);
- std::stringstream ss;
- if (!prof_file.is_open()) {
- ss << "Unable to open cpu profile: " <<
tmp_prof_file_name.str();
- std::string str = ss.str();
- HttpChannel::send_reply(req, str);
- return;
- }
- ss << prof_file.rdbuf();
- prof_file.close();
- std::string str = ss.str();
- HttpChannel::send_reply(req, str);
- return;
- }
+ std::ostringstream tmp_prof_file_name;
+ tmp_prof_file_name << config::pprof_profile_dir << "/doris_profile." <<
getpid() << "."
+ << rand();
+ ProfilerStart(tmp_prof_file_name.str().c_str());
+ sleep(seconds);
+ ProfilerStop();
- // text type. we will return readable content via http response
+ if (type_str == "text") {
std::stringstream readable_res;
Status st = PprofUtils::get_readable_profile(tmp_prof_file_name.str(),
true, &readable_res);
if (!st.ok()) {
HttpChannel::send_reply(req, st.to_string());
} else {
HttpChannel::send_reply(req, readable_res.str());
}
- } else {
- // generate flamegraph
+ } else if (type_str == "flamegraph") {
std::string svg_file_content;
- std::string flamegraph_install_dir =
- std::string(std::getenv("DORIS_HOME")) + "/tools/FlameGraph/";
- Status st = PprofUtils::generate_flamegraph(seconds,
flamegraph_install_dir, false,
- &svg_file_content);
+ Status st = PprofUtils::get_svg_profile(tmp_prof_file_name.str(),
&svg_file_content);
if (!st.ok()) {
HttpChannel::send_reply(req, st.to_string());
} else {
HttpChannel::send_reply(req, svg_file_content);
}
+ } else {
+ std::ifstream prof_file(tmp_prof_file_name.str().c_str(),
std::ios::in);
+ std::stringstream ss;
+ if (!prof_file.is_open()) {
+ ss << "Unable to open cpu profile: " << tmp_prof_file_name.str();
+ std::string str = ss.str();
+ HttpChannel::send_reply(req, str);
+ return;
Review Comment:
Minor: This early `return` exits without attempting to delete the temp file
created at line 163. While the file likely doesn't exist if it can't be opened,
it would be more robust to attempt cleanup here too:
```cpp
static_cast<void>(io::global_local_filesystem()->delete_file(tmp_prof_file_name.str()));
```
before returning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
