Add use-trace-processor-shell flag to automate large trace processing #248

salaast · 2022-07-18T16:12:35Z

Add use-processor-exe and open-url flags to run_command in trace.ml to automate large trace file processing and ui url opening

Signed-off-by: Abena Laast <[email protected]>

… to allow large trace processing and url opening automation Signed-off-by: Abena Laast <[email protected]>

Signed-off-by: Abena Laast <[email protected]>

Xyene · 2022-07-19T16:26:25Z

src/trace.ml

@@ -561,6 +561,32 @@ module Make_commands (Backend : Backend_intf.S) = struct
    { Decode_opts.output_config; decode_opts; print_events }
  ;;

+  let open_url_flags =


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 to https://magic-trace.org, would be sufficient here.

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?

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.

Xyene · 2022-07-19T16:28:30Z

src/trace.ml

+               | _ -> raise_s [%message "unkown user input" (user_choice : string)]))
+  ;;
+
+  let use_processor_flag =


I think we should take the path via an envvar, e.g. MAGIC_TRACE_PROCESSOR_SHELL_PATH; then we can have an optional -use-trace-processor-shell flag if that envvar is set. See

magic-trace/src/tracing_tool_output.ml

Line 15 in 872c648

match Env_vars.perfetto_dir with

for inspiration.

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.

Xyene · 2022-07-19T16:41:35Z

src/trace.ml

+           | 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 ]


Can we use something like

magic-trace/src/perf_tool_backend.ml

Line 70 in 872c648

let perf_fork_exec ?env ~prog ~argv () =

here, to avoid the trace processor shell outliving magic-trace, in case magic-trace is Ctrl+C'd. May want to extract it into a helper module like process.ml or something.

Xyene · 2022-07-19T16:42:35Z

src/trace.ml

-               decode_opts))
+                 decode_opts)
+         in
+         let%bind.Deferred () =


Hmm, I don't think we should do this. Oftentimes we'll run magic-trace attach ... -serve through SSH, so there'd be no browser to open and this'd fail.

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.

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.

remove open_url flag, add processor_path env var and remove arg for use_processor_shell_path flag, create Enable module for processor_path param Signed-off-by: Abena Laast <[email protected]>

Signed-off-by: Abena Laast <[email protected]>

Xyene · 2022-08-05T13:19:43Z

src/env_vars.ml

+(* Points to a filesystem path with a trace processor executable. 
+   If provided, the flag can be indicated to automatically run the processor 
+   at the path once the trace file is created *)
+let processor_path = Unix.getenv "MAGIC_TRACE_PROCESSOR_SHELL_PATH"


Suggested change

let processor_path = Unix.getenv "MAGIC_TRACE_PROCESSOR_SHELL_PATH"

let processor_path = Unix.getenv "MAGIC_TRACE_TRACE_PROCESSOR_SHELL_PATH"

Xyene · 2022-08-05T13:20:06Z

src/env_vars.mli

@@ -4,3 +4,4 @@ val debug : bool
 val perf_is_privileged : bool
 val perfetto_dir : string option
 val no_dlfilter : bool
+val processor_path : string option


Suggested change

val processor_path : string option

val trace_processor_shell_path : string option

to be explicit

Xyene · 2022-08-05T13:20:39Z

src/trace.ml

@@ -8,8 +8,10 @@ let supports_command command =
  Lazy.from_fun (fun () ->
      match Core_unix.fork () with
      | `In_the_child ->
-        Core_unix.close Core_unix.stdout;
-        Core_unix.close Core_unix.stderr;
+        (* gracefully hide perf outputs *)


We should leave a comment here about what versions of perf we observed breakages when close-ing and under what circumstances, because it was surprising.

Xyene · 2022-08-05T13:21:25Z

src/trace.ml

@@ -561,6 +563,41 @@ module Make_commands (Backend : Backend_intf.S) = struct
    { Decode_opts.output_config; decode_opts; print_events }
  ;;

+  module Enable = struct


Suggested change

module Enable = struct

module Enable = struct

Nit: we should give this module a more descriptive name.

Xyene · 2022-08-05T13:21:45Z

src/trace.ml

+
+    type t =
+      | Disabled
+      | Enabled of enabled


Suggested change

| Enabled of enabled

| Enabled of { value : string }

Let's inline this type.

Let's also give value a better name, like trace_processor_shell_path.

Xyene · 2022-08-05T13:22:21Z

src/trace.ml

+  (* Same as [Caml.exit] but does not run at_exit handlers *)
+  external sys_exit : int -> 'a = "caml_sys_exit"
+
+  let call_trace_processor ?env ~prog ~argv () =


Let's either generalize this with the perf version, or leave a CR-someday to do this later.

I am unsure which part of this function is perf-specific.

None of it is, so ideally this'd be the same function as

magic-trace/src/perf_tool_backend.ml

Line 70 in d51d797

let perf_fork_exec ?env ~prog ~argv () =

so we don't have two copies.

You don't need to do this in this PR, but it would be good to leave a note for the future.

Xyene · 2022-08-05T13:24:12Z

src/trace.ml

-         magic-trace run -full-execution ./program\n")
+         magic-trace run -full-execution ./program\n\
+         # Run a process that generates a large trace file, magic-trace run ./program \
+         -use-trace-processor-shell\n")


We should leave this out of the documentation for now, or maybe make the -use-trace-processor-shell part of it conditional on the environment variable being present. Definitely don't want to give help text for a flag that doesn't exist.

change processor_path to trace_processor_shell_path, add comment about closing stdout and stderr breaking things, better name for Enable module, inline Enabled, better name for value, CR-someday for generalizing call_trace_processor with the perf version, update processor flag doc to clarify conditional existence Signed-off-by: Abena Laast <[email protected]>

Xyene · 2022-08-05T19:29:48Z

src/trace.ml

+      match Env_vars.trace_processor_shell_path with
+      | None -> Command.Param.return Disabled
+      | Some processor_shell_path ->
+        let%map_open.Command processor =


Suggested change

let%map_open.Command processor =

let%map_open.Command use_trace_processor_shell =

Nit: descriptive variable names.

Xyene · 2022-08-05T19:30:55Z

src/trace.ml

+    let param =
+      match Env_vars.trace_processor_shell_path with
+      | None -> Command.Param.return Disabled
+      | Some processor_shell_path ->


Suggested change

| Some processor_shell_path ->

| Some trace_processor_shell_path ->

Nit: avoid having multiple variable names for the semantically-same thing. It's best to shadow the previously-declared name in such cases.

Xyene · 2022-08-05T19:31:09Z

src/trace.ml

+          flag
+            "use-trace-processor-shell"
+            no_arg
+            ~doc:[%string "use the trace processor set in environment variables"]


Suggested change

~doc:[%string "use the trace processor set in environment variables"]

~doc:[%string "use trace processor shell to view trace"]

Xyene · 2022-08-05T19:37:56Z

There seem to be some conflicts with the base branch, could you please try rebasing to resolve them?

salaast force-pushed the abena-mega-trace branch from 4b1ee1e to 08e21a9 Compare July 18, 2022 16:51

salaast added 3 commits July 18, 2022 18:40

mega traces: some early work

86aebc1

Signed-off-by: Abena Laast <[email protected]>

Add trace_processor_exe and open_url flags to run_command in trace.ml…

1897453

… to allow large trace processing and url opening automation Signed-off-by: Abena Laast <[email protected]>

Update use_processor_flag doc

39fc733

Signed-off-by: Abena Laast <[email protected]>

salaast force-pushed the abena-mega-trace branch from 08e21a9 to 39fc733 Compare July 18, 2022 18:44

salaast changed the title ~~Abena mega trace~~ Add use-processor-exe and open-url flags to run_command Jul 18, 2022

Xyene reviewed Jul 19, 2022

View reviewed changes

salaast added 2 commits August 2, 2022 15:28

close stdout and stderr, and write to devnull in supports_command,

4a91831

remove open_url flag, add processor_path env var and remove arg for use_processor_shell_path flag, create Enable module for processor_path param Signed-off-by: Abena Laast <[email protected]>

add clarifying comment

679ad5d

Signed-off-by: Abena Laast <[email protected]>

salaast changed the title ~~Add use-processor-exe and open-url flags to run_command~~ Add use-trace-processor-shell flag to automate large trace processing Aug 2, 2022

remove unnecessary async_shell from libraries

681bf54

Signed-off-by: Abena Laast <[email protected]>

Xyene reviewed Aug 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add use-trace-processor-shell flag to automate large trace processing #248

Add use-trace-processor-shell flag to automate large trace processing #248

salaast commented Jul 18, 2022

Xyene Jul 19, 2022

salaast Jul 20, 2022

Xyene Jul 22, 2022

Xyene Jul 19, 2022

Xyene Jul 19, 2022

Xyene Jul 19, 2022

salaast Jul 21, 2022

Xyene Jul 22, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

salaast Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene Aug 5, 2022

Xyene commented Aug 5, 2022

	let processor_path = Unix.getenv "MAGIC_TRACE_PROCESSOR_SHELL_PATH"
	let processor_path = Unix.getenv "MAGIC_TRACE_TRACE_PROCESSOR_SHELL_PATH"

	val processor_path : string option
	val trace_processor_shell_path : string option

	let%map_open.Command processor =
	let%map_open.Command use_trace_processor_shell =

	\| Some processor_shell_path ->
	\| Some trace_processor_shell_path ->

	~doc:[%string "use the trace processor set in environment variables"]
	~doc:[%string "use trace processor shell to view trace"]

Add use-trace-processor-shell flag to automate large trace processing #248

Are you sure you want to change the base?

Add use-trace-processor-shell flag to automate large trace processing #248

Conversation

salaast commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xyene commented Aug 5, 2022