Support Perfetto trace recovery across device reboot - #6928
Support Perfetto trace recovery across device reboot#6928safayat-google wants to merge 5 commits into
Conversation
Bug: 382209797 Test: m -j100 perfetto_unittests Flag: EXEMPT BUGFIX Change-Id: Ib1e121976c90309cb34db0a09617f0fba0951a42
🎨 Perfetto UI Builds
|
LalitMaganti
left a comment
LalitMaganti
left a comment
There was a problem hiding this comment.
First pass through these changes. Overall looking in the right direction but some questions.
| // Supported on: Android 26Q4+. | ||
| optional bool persist_trace_across_reboots = 49; | ||
|
|
||
| // If set to true, forces explicit buffer flushes prior to periodic write |
There was a problem hiding this comment.
Isn't this the default now? Why do we need this?
|
|
||
| package perfetto.protos; | ||
|
|
||
| // TODO: or should we just reuse an existing an existing proto file? |
There was a problem hiding this comment.
It would be nice to use an existing file if possible.
| "Copying persistent trace %s for write_into_file session %" PRIu64 | ||
| " into bugreport path %s", | ||
| persistent_path.c_str(), it->tsid, out_path.c_str()); | ||
| base::CopyFileContents(*fd_in, *fd_out); |
There was a problem hiding this comment.
Should this not be on a thread? This seems inconsistent with how clone works otherwise.
| // 3:ts : Timed out | ||
| const char* kRebootTraceStatusProp = "traced.reboot_trace_status"; | ||
| constexpr uint64_t kUploadWaitTimeoutNs = | ||
| 5ULL * 60 * 1000 * 1000 * 1000; // 5 minutes |
There was a problem hiding this comment.
This seems like a very large timeout on the surface. Is there some motivation? Maybe add a comment?
There was a problem hiding this comment.
Oh is this the time to wait for boot? Feels this variable is misnamed...
| // If property is set, but the persistent trace file STILL exists on disk, | ||
| // log error, unlink file, and crash. | ||
| if (base::FileExists(target_file_path)) { | ||
| PERFETTO_ELOG( |
There was a problem hiding this comment.
We need statsd attoms for any case like this...
| WaitForPreviousRebootTraceUpload(clean_name, file_path.c_str()); | ||
|
|
||
| // Unlink any pre-existing instance of this persistent trace file. | ||
| unlink(file_path.c_str()); |
There was a problem hiding this comment.
If we did O_TRUNC in the flags below, wouldn't that be stricly better than unlink + open?
| persistent_path.c_str(), it->tsid); | ||
| } | ||
| sync_fn(); | ||
| continue; |
There was a problem hiding this comment.
this continue seems a bit off to me, if we hit then we'll never invoke CloneSessionOnThread below? I think we should not guard on basis of is_write_into_file.
3 participants
Adds support for persistent traces to survive device reboots on Android.
When
persist_trace_across_rebootsis enabled inTraceConfig, Perfetto saves persistent trace files to disk and recovers/uploads them on the next boot via--upload-after-reboot.Summary of changes:
persist_trace_across_rebootsto TraceConfig andAfterRebootTraceEventproto header to annotate recovered trace metadata.UploadPersistentTracesAfterRebootto prevent crash loops if parsing or upload fails.traced.reboot_trace_statussysprop so new tracing sessions wait for active uploads to finish before starting.Tested on a device and verified new
perfetto_unittestsBug: 382209797
Test: atest perfetto_unittests;