Skip to content
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

Allow sharing traces via MAGIC_TRACE_SHARE_COMMAND #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Xyene
Copy link
Member

@Xyene Xyene commented Apr 8, 2022

Tested with MAGIC_TRACE_SHARE_COMMAND=/usr/bin/echo.

Also fixed a bug where we were unconditionally serving the UI if the
Perfetto base dir was specified. We lose port configurability, but it's
not a big deal and we can add it back as a separate flag if we want to.

@Xyene Xyene requested a review from cgaebel April 8, 2022 17:56
@cgaebel
Copy link
Contributor

cgaebel commented Apr 8, 2022

Port configurability is a big deal! We all use the same machine at work to do magic-trace stuff. That said, I'm fine with adding it back in a separate feature.

Tested with `MAGIC_TRACE_SHARE_COMMAND=/usr/bin/echo`.

Also fixed a bug where we were unconditionally serving the UI if the
Perfetto base dir was specified. We lose port configurability, but it's
not a big deal and we can add it back as a separate flag if we want to.
@Xyene
Copy link
Member Author

Xyene commented Apr 8, 2022

I'm not sure which I like more:

  • a -serve-port flag, or
  • starting from 8080 and trying increasing port numbers until we find one we can bind to

@cgaebel
Copy link
Contributor

cgaebel commented Apr 8, 2022

My heart says (2, but start at 1024), but my brain says (1). S'up to you.

flag
"serve"
no_arg
~doc:[%string " Serves the magic-trace UI on port %{port#Int})"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra paren at the end

~args:[ Filename_unix.realpath store_path ]
()
in
Core.printf "%s%!" output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't show output until the command ends, and it might also swallow stderr? I'd use the cruddy process APIs to transfer stdout/stderr of the subprocess to magic-trace's.


let maybe_param =
Option.map Env_vars.share_command_filename ~f:(fun share_command_filename ->
let%map_open.Command share = flag "share" no_arg ~doc:"Share trace." in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That help text could say more. If you don't want it to be too long, I'm happy with the docs just being magic-trace.org/w/share and then we explain how sharing works on the wiki somewhere.

Serve could arguably work the same way.

@Xyene Xyene added this to the v1.1.0 milestone Apr 18, 2022
@Xyene Xyene modified the milestones: v1.1.0, v1.2.0 Jun 1, 2022
@Xyene Xyene removed this from the v1.2.0 milestone Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants