-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add use-trace-processor-shell flag to automate large trace processing #248
base: master
Are you sure you want to change the base?
Changes from 3 commits
86aebc1
1897453
39fc733
4a91831
679ad5d
681bf54
f567768
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -561,6 +561,32 @@ module Make_commands (Backend : Backend_intf.S) = struct | |||
{ Decode_opts.output_config; decode_opts; print_events } | ||||
;; | ||||
|
||||
let open_url_flags = | ||||
let open Command.Param in | ||||
flag | ||||
"open-url" | ||||
(optional string) | ||||
~doc: | ||||
"opens a url to display trace. options are local, magic, or android to open \ | ||||
http://localhost:10000, https://magic-trace.org, or https://ui.perfetto.dev \ | ||||
respectively" | ||||
|> map ~f:(fun user_input -> | ||||
Option.map user_input ~f:(fun user_choice -> | ||||
match user_choice with | ||||
| "local" -> "http://localhost:10000" | ||||
| "magic" -> "https://magic-trace.org" | ||||
| "android" -> "https://ui.perfetto.dev" | ||||
| _ -> raise_s [%message "unkown user input" (user_choice : string)])) | ||||
;; | ||||
|
||||
let use_processor_flag = | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should take the path via an envvar, e.g. magic-trace/src/tracing_tool_output.ml Line 15 in 872c648
We may also want to consider just using the trace processor shell automatically, past a certain filesize, but I haven't given that much thought. |
||||
let open Command.Param in | ||||
flag | ||||
"use-processor-exe" | ||||
(optional string) | ||||
~doc:"path for standalone processor exe to use to process trace file" | ||||
;; | ||||
|
||||
let run_command = | ||||
Command.async_or_error | ||||
~summary:"Runs a command and traces it." | ||||
|
@@ -573,11 +599,16 @@ module Make_commands (Backend : Backend_intf.S) = struct | |||
magic-trace run -multi-thread ./program -- arg1 arg2\n\n\ | ||||
# Run a process, tracing its entire execution (only practical for short-lived \ | ||||
processes)\n\ | ||||
magic-trace run -full-execution ./program\n") | ||||
magic-trace run -full-execution ./program\n\ | ||||
# Run a process that generates a large trace file, and view it on \ | ||||
magic-trace.org magic-trace run ./program -open-url magic -use-processor-exe \ | ||||
~/local-trace-processor\n") | ||||
(let%map_open.Command record_opt_fn = record_flags | ||||
and decode_opts = decode_flags | ||||
and debug_print_perf_commands = debug_print_perf_commands | ||||
and prog = anon ("COMMAND" %: string) | ||||
and trace_processor_exe = use_processor_flag | ||||
and url = open_url_flags | ||||
and argv = | ||||
flag "--" escape ~doc:"ARGS Arguments for the command. Ignored by magic-trace." | ||||
in | ||||
|
@@ -589,31 +620,53 @@ module Make_commands (Backend : Backend_intf.S) = struct | |||
| Some path -> path | ||||
| None -> failwithf "Can't find executable for %s" prog () | ||||
in | ||||
record_opt_fn ~executable ~f:(fun opts -> | ||||
let elf = Elf.create opts.executable in | ||||
let%bind range_symbols = | ||||
evaluate_trace_filter ~trace_filter:opts.trace_filter ~elf | ||||
in | ||||
let%bind pid = | ||||
let argv = prog :: List.concat (Option.to_list argv) in | ||||
run_and_record | ||||
opts | ||||
let%bind () = | ||||
record_opt_fn ~executable ~f:(fun opts -> | ||||
let elf = Elf.create opts.executable in | ||||
let%bind range_symbols = | ||||
evaluate_trace_filter ~trace_filter:opts.trace_filter ~elf | ||||
in | ||||
let%bind pid = | ||||
let argv = prog :: List.concat (Option.to_list argv) in | ||||
run_and_record | ||||
opts | ||||
~elf | ||||
~debug_print_perf_commands | ||||
~prog | ||||
~argv | ||||
~collection_mode:opts.collection_mode | ||||
in | ||||
let%bind.Deferred perf_maps = Perf_map.Table.load_by_pids [ pid ] in | ||||
decode_to_trace | ||||
~perf_maps | ||||
?range_symbols | ||||
~elf | ||||
~trace_scope:opts.trace_scope | ||||
~debug_print_perf_commands | ||||
~prog | ||||
~argv | ||||
~record_dir:opts.record_dir | ||||
~collection_mode:opts.collection_mode | ||||
in | ||||
let%bind.Deferred perf_maps = Perf_map.Table.load_by_pids [ pid ] in | ||||
decode_to_trace | ||||
~perf_maps | ||||
?range_symbols | ||||
~elf | ||||
~trace_scope:opts.trace_scope | ||||
~debug_print_perf_commands | ||||
~record_dir:opts.record_dir | ||||
~collection_mode:opts.collection_mode | ||||
decode_opts)) | ||||
decode_opts) | ||||
in | ||||
let%bind.Deferred () = | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I don't think we should do this. Oftentimes we'll run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you recommend catching an error here or something else? I was unable to find a way to detect from inside the function whether there is actually a browser available. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we just shouldn't be opening the user's browser. We already print a link to the terminal for where the user should go to view their trace, and many terminals just allow clicking into it directly. |
||||
match url with | ||||
| None -> Deferred.return () | ||||
| Some url -> Async_shell.run "open" [ url ] | ||||
in | ||||
let output_config = decode_opts.Decode_opts.output_config in | ||||
let output = Tracing_tool_output.output output_config in | ||||
let output_file = | ||||
match output with | ||||
| `Sexp _ -> failwith "unimplemented" | ||||
| `Fuchsia store_path -> store_path | ||||
in | ||||
let%bind.Deferred () = | ||||
match trace_processor_exe with | ||||
| None -> | ||||
print_endline "warning: must use local processor on large trace files"; | ||||
Deferred.return () | ||||
| Some processor_path -> Async_shell.run processor_path [ "-D"; output_file ] | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use something like magic-trace/src/perf_tool_backend.ml Line 70 in 872c648
process.ml or something.
|
||||
in | ||||
return ()) | ||||
;; | ||||
|
||||
let select_pid () = | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should allow configuration of this through envvars in
env_vars.ml
, rather than as commandline parameters. These choices tend to be sticky, so it's nicer to not have users have to specify them over and over.I'd say a
MAGIC_TRACE_WEB_UI_URL
that can be any string, and which defaults tohttps://magic-trace.org
, would be sufficient here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once I implement the Env_vars configuration, should I use the commandline flag to indicate whether the user wants to display the ui, or should it always be displayed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now, we should have a separate flag for if the trace processor shell should be used, if available. Maybe in the future we can make the selection automatic using some heuristic based on the size of the trace file (and whether we expect a browser to safely open it), but let's hold off on that bit for now.