Copilot commented on code in PR #61851:
URL: https://github.com/apache/doris/pull/61851#discussion_r3005893106
##########
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);
+ }
+
Review Comment:
`get_svg_profile()` unconditionally deletes `profile_file` (and does so
before checking `rc`). This is a surprising side effect for a function that
appears to just “get”/convert a profile, and it also makes failures harder to
debug because the input is removed even when `pprof` fails. Consider moving
deletion to the caller, or adding a `delete_after_parse` flag and only deleting
on success (or when explicitly requested).
```suggestion
if (!rc) {
return Status::InternalError("Failed to execute command: {}",
cmd_output);
}
// Best-effort cleanup of the profile file after successful conversion.
static_cast<void>(io::global_local_filesystem()->delete_file(profile_file));
```
##########
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);
Review Comment:
In the raw-profile branch, when `prof_file.is_open()` fails the function
returns without attempting to clean up the temporary profile file. Since this
file was just created by `ProfilerStart/Stop`, consider deleting it
(best-effort) on this error path as well to avoid leaving behind temp files in
`pprof_profile_dir`.
```suggestion
HttpChannel::send_reply(req, str);
static_cast<void>(io::global_local_filesystem()->delete_file(tmp_prof_file_name.str()));
```
##########
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);
Review Comment:
`get_svg_profile()` builds a shell command by concatenating `self_cmdline`
and `profile_file` into a single string and executes it via `popen()`. Because
`popen()` invokes `/bin/sh -c`, any spaces or shell metacharacters in these
paths (e.g., from `config::pprof_profile_dir`) can break the command or be
interpreted by the shell. Prefer executing without a shell (execve/posix_spawn
with argv), or at minimum apply robust shell-escaping/quoting to both arguments
before concatenation.
##########
be/src/util/pprof_utils.h:
##########
@@ -44,6 +44,9 @@ class PprofUtils {
static Status get_readable_profile(const std::string& file_or_content,
bool is_file,
std::stringstream* output);
+ /// generate svg output by `pprof --svg doris_be profile`.
Review Comment:
The new `get_svg_profile()` API deletes the input file as an implicit side
effect (per implementation). Please document this behavior in the header
comment (or adjust the API to avoid deleting caller-owned files), so callers
don’t accidentally pass a non-temporary profile they intend to keep.
```suggestion
/// generate svg output by `pprof --svg doris_be profile_file`.
/// NOTE: `profile_file` is expected to be a temporary file.
Implementations of this API
/// may delete `profile_file` after generating the SVG output, so
callers must not pass
/// a profile file they intend to keep.
```
--
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]
