java-profile flamegraphs saved in tmp folder, duplicate jvm arg fix #207
Conversation
Why was that a problem? |
src/data/java_profile.rs
Outdated
| params.run_name, key | ||
| ); | ||
|
|
||
| fs::copy(tmp_loc.clone(), html_loc).ok(); |
There was a problem hiding this comment.
A move would be better than a copy. Although, the answer to my question of why we have to write to /tmp/ in the first place might mean it has to be a copy.
| use std::fs; | ||
| use std::os::unix::fs::PermissionsExt; | ||
|
|
||
| pub static APERF_TMP: &str = "/tmp/aperf_tmp"; |
There was a problem hiding this comment.
This should be passed in as a parameter.
There was a problem hiding this comment.
Do you mean like another CLI option and part of collector params?
There was a problem hiding this comment.
As part of the collector params passed into each of the data type.
src/data/java_profile.rs
Outdated
|
|
||
| let html_loc = html_path.to_str().unwrap(); | ||
| let tmp_loc = format!( | ||
| "/tmp/aperf_tmp/{}-java-flamegraph-{}.html", |
There was a problem hiding this comment.
We are hard-coding /tmp/aperf_tmp in multiple locations. We should have the tmp location passed in as a parameter.
When aperf is launched with sudo, the record folder it creates has sudo permissions. A non-sudo JVM being profiled will not be able to save the flamegraph to this folder. |
src/data.rs
Outdated
| @@ -58,6 +58,7 @@ pub struct CollectorParams { | |||
| pub data_dir: String, | |||
| pub run_name: String, | |||
| pub profile: HashMap<String, String>, | |||
| pub tmp_dir: String, | |||
There was a problem hiding this comment.
Let's use PathBuf for paths. We should change the other members of the CollectorParams from String to PathBuf.
lancelui-amzn
force-pushed
the
record-temp-folder
branch
from
07227d9 to
17c8a2b
Compare
Description of changes:
CollectorParamspath fields toPathBuffromStringTesting:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.