fastly / cli

Build, deploy and configure Fastly services from your terminal
https://fastly.dev/reference/cli/
Apache License 2.0
139 stars 59 forks source link

[FEATURE REQUEST] Support more profilers with `fastly compute serve` #1103

Open jameysharp opened 11 months ago

jameysharp commented 11 months ago

Is your feature request related to a problem? Please describe. The fastly compute serve --profile-guest option works on all platforms and doesn't require installing any additional software, so it's a great default choice for profiling, but it has some limitations compared to native profilers like Linux perf or Intel VTune. Viceroy supports these other profilers via its --profile option but the Fastly CLI doesn't expose those alternatives when invoking Viceroy with fastly compute serve.

Describe the solution you'd like Add an option to fastly compute serve that causes it to pass e.g. --profile=jitdump to Viceroy. Currently Viceroy also supports perfmap and vtune variants.

It would be preferable to also wrap Viceroy in the appropriate profiler. For example, profiling with VTune means running vtune -run-pass-thru=--no-altstack -collect hotspots viceroy --profile=vtune instead of just running viceroy.

When using Linux perf in jitdump mode, after recording the profile with perf record, the user has to additionally run something like perf inject --jit --input perf.data --output perf.jit.data. It would be nice to do this for them. It would also be nice to have perf record write to a temporary file and have perf inject output to the perf.data name that perf report expects to read from.

The user may want to pass additional options to the profiler so it would be good to have an option for that.

Describe alternatives you've considered Adding support for passing through uninterpreted arguments to Viceroy, as suggested in #1047, might work here too. However, that means the user has to wrap the profiler around the Fastly CLI and ensure that it profiles child processes as well, which not only makes the user experience more complex but will add a bunch of data they don't care about to the profile.

In the future, Wasmtime and Viceroy may gain support for more profilers. It would be nice if we didn't need to change the Fastly CLI to support new profilers, which would suggest making the profiler name be an uninterpreted string. We could probably define some way for Viceroy to tell the CLI what profilers it supports and perhaps how to invoke them, or make Viceroy re-spawn itself under the appropriate profiler.

Additional context The Wasmtime book documents Wasmtime's support for external profilers, specifically perf and VTune. There's also documentation on using Samply with Wasmtime which relies on the same perfmap profiling mode that perf can use.

tedmielczarek-fastly commented 6 months ago

Could you make this work by writing a wrapper script that invokes viceroy under your profiler of choice, and passing that to the CLI via --viceroy-path?

tedmielczarek-fastly commented 6 months ago

For the record, I tested this and it does work:

% cat > /tmp/viceroy.sh
#!/bin/sh

exec viceroy --unknown-import-behavior zero-or-null "$@"

Then:

% fastly compute serve --viceroy-path=/tmp/viceroy.sh
jameysharp commented 6 months ago

That's a great proof of concept, thanks! I don't think shell-script wrappers really address the UX points I was raising, but I do think this is a good way to prototype the necessary functionality. For Linux perf in "jitdump" mode as an example, where there's a required postprocessing step after the profile is collected, I think something like this should work, though I haven't tested it:

#!/bin/sh
perf record -k mono viceroy --profile jitdump "$@"
perf inject --jit --input perf.data --output perf.jit.data
echo 'now run `perf report --input perf.jit.data`'
fgsch commented 6 months ago

Until this is implemented, this can be achieved via #1186 .

jameysharp commented 6 months ago

1186 is great but note that if you do something like --viceroy-args '--profile jitdump' then you have to run the profiler around the entire Fastly CLI process, rather than just profiling Viceroy itself. I'm not sure whether all the profilers Wasmtime supports work with multi-process setups like that.

fgsch commented 6 months ago

@jameysharp good point. I wonder if we should include some scripts together with the CLI and direct people to use --viceroy-path=... instead of adding the option to specify other profilers.