Conversation
🎨 Perfetto UI Build✅ UI build is ready: https://storage.googleapis.com/perfetto-ci-artifacts/gh-20995970217-1-ui/ui/index.html
|
…ub.com:google/perfetto into perfetto_recover_reboot_traces_9
…ot_traces_9 # Conflicts: # perfetto.rc
…ot_traces_9 # Conflicts: # python/perfetto/protos/perfetto/trace/perfetto_trace_pb2.py # python/perfetto/protos/perfetto/trace/perfetto_trace_pb2.pyi # src/perfetto_cmd/perfetto_cmd.h # src/perfetto_cmd/perfetto_cmd_android.cc # test/cts/host_side_tests/src/android/perfetto/cts/test/PerfettoBootTimeTraceHostTest.java
…ub.com:google/perfetto into perfetto_recover_reboot_traces_9
primiano
left a comment
There was a problem hiding this comment.
as per offline discussion, let's keep this CL purely on the perfetto_cmd (and proto) changes.
let's split out the test and the .rc file, as I think i have a better proposal (Writing a doc now)
src/perfetto_cmd/perfetto_cmd.cc
Outdated
| persistent_trace_out_path_ = std::string(kAndroidPersistentStateDir) + "/" + | ||
| trace_config_->unique_session_name() + | ||
| ".pftrace"; | ||
| PERFETTO_CHECK(trace_config_->output_path().empty()); |
There was a problem hiding this comment.
THis should be an ELOG+return like the ones above
| --subscription-id : ID of the subscription that triggered this trace. | ||
| --upload : Upload trace. | ||
| --upload-after-reboot : Used by Android to upload the traces saved to a | ||
| special directory after the system reboot. |
There was a problem hiding this comment.
can you say: [Android-only] Reports to the Android framework all the traces marked as persist_trace_after_reboot=true which haven't been cleanly terminated.
| #endif | ||
| } | ||
| if (option == OPT_UPLOAD_AFTER_REBOOT) { | ||
| #if PERFETTO_BUILDFLAG(PERFETTO_OS_ANDROID) |
There was a problem hiding this comment.
small thing. can you just set a boolean here, and move the logic below after the arg parsing?
I find a bit easier to reason about if the code here in the top does pure parsing and the logic happens below
| break; | ||
| } | ||
| statsd_logging_ = ShouldLogStatsdEvents( | ||
| *trace_config_, /*unspecified_filed_value=*/upload_flag_); |
There was a problem hiding this comment.
why this is called unspecified_field_value? call it upload_flag (also in the arg name)
|
|
||
| // static | ||
| bool PerfettoCmd::ShouldLogStatsdEvents(const TraceConfig& cfg, | ||
| bool unspecified_filed_value) { |
There was a problem hiding this comment.
s/unspecified_filed_value/upload_flag/ ?
| bool all_reports_succeed = true; | ||
| for (base::ScopedFile& fd : fds) { | ||
| std::optional<uint64_t> maybe_file_size = base::GetFileSize(*fd); | ||
| if (!maybe_file_size.has_value()) { |
There was a problem hiding this comment.
if (!maybe_file_size.has_value() || *maybe_file_size <= 0) {
PERFETTO_PLOG("Failed to stat persistent trace");
continue;
}
uint64_t file_size = maybe_file_size.value();
| continue; | ||
| } | ||
| uint64_t file_size = maybe_file_size.value(); | ||
| if (maybe_file_size == 0) { |
There was a problem hiding this comment.
nit this should be file_size, i think the above is a cast of the bool operator actually (subtle bug i know). but if yuou follow my suggestion above this becomes more compact anyways.
| base::ScopedMmap mmaped_file = | ||
| base::ScopedMmap::FromHandle(std::move(mmap_fd), file_size); | ||
| if (!mmaped_file.IsValid()) { | ||
| continue; |
| } | ||
|
|
||
| if (!trace_config.has_value()) { | ||
| continue; |
There was a problem hiding this comment.
ELOG here (it shoul really never happen)
| return false; | ||
| } | ||
|
|
||
| bool all_reports_succeed = true; |
There was a problem hiding this comment.
i think rather than this, it's more error-proof to:
- increment the counter of suceeded reports if you reach the end
- then you return num_succeeded == fds_count (cache it at the beginning)
because here you are not accounting I think for weird cases like "the trace config could not be parsed" and so on
Tested:
atest CtsPerfettoReporterRebootTestCases
atest CtsPerfettoReporterTestCases