Show indicators that perf script is running #343
Conversation
|
I want to make a quick tweak and see if I can improve the case where the user cancels the run. Right now, it always appears that a run is successful from the output of the spinner. That is OK, but I think we can do better. The tradeoff is that the code will be a little bit more complex. |
gth828r
force-pushed
the
270.show-perf-script-running
branch
from
54dbffb
to
9b662dc
Compare
This attempted change did not work. perf script will return a successful status code even if it was forcefully terminated by a signal. I just reverted this change, because the original code was much simpler and cleaner. Turning off draft status. |
|
A quick note on the approach: The main goal of this change is that under normal operations, users should see that flamegraph-rs is doing work when it calls As a secondary goal, I had hoped that the final output of the spinner could reflect success or failure, particularly if the command was terminated early with a signal. Unfortunately, when testing I found that if a signal is used to stop With this constraint in mind, I am using a scopeguard block to always mark the spinner as finished, regardless of how the function exits. This helps keep the spinner code isolated and simple while also ensuring that the spinner resources are always cleaned up, even if the function exits early with an error. |
src/lib.rs
Outdated
| @@ -34,6 +34,10 @@ pub enum Workload { | |||
|
|
|||
| #[cfg(target_os = "linux")] | |||
| mod arch { | |||
| use indicatif::{ProgressBar, ProgressStyle}; | |||
| use scopeguard; | |||
There was a problem hiding this comment.
| use scopeguard; |
I think this line may be unnecessary?
There was a problem hiding this comment.
Thanks! This is OBE since we removed scopeguard altogether, but you were right.
|
(If you rebase on current main, the CI audit failure should go away). |
gth828r
force-pushed
the
270.show-perf-script-running
branch
from
9b662dc
to
935f434
Compare
|
Rebased on main and removed the scopeguard dependency. |
| let output = command.output().context("unable to call perf script")?; | ||
| spinner.finish(); |
There was a problem hiding this comment.
So we can more cleanly always call spinner.finish() after command.output() (before yielding the error using the ? operator). I think I would prefer that.
There was a problem hiding this comment.
The behavior here will depend on what happens with our discussion down at #343 (comment), so let me just lay out some details. I don't really feel strongly about any of this, but let's resolve that discussion first before we try to resolve this one. Here are some details about why I want to do these in that particular order:
I've been playing around with different scenarios to see what happens in different situations with the spinner. What I have found is that the resources are always cleaned up when the spinner goes out of scope. The only thing that calling spinner.finish() seems to do is leave a copy of the prefix/message/final tick in the terminal history. Note that the final tick string is configurable, and it is only called once finish() gets set (not part of the spinner animation), so using a special final tick symbol can act as an indicator of success.
If you don't call spinner.finish() before the spinner goes out of scope, then the line where the spinner was is effectively removed from the terminal history, as if the spinner wasn't ever there. If you call finish whether the command succeeds or fails, then we will see the terminal history including the final tick. The one exception is that if you don't use a message/prefix and the default tick is blank, then nothing will be included in the terminal history.
With all of this in mind, if you use the default spinner, the final tick of the spinner is simply blank, so it doesn't really indicate anything. My custom ticks use a checkmark to indicate successful completion. The effect of doing what you propose with the default spinner would be to always have the spinner line left in the terminal history regardless of whether the command succeeded or failed. I think that is OK. If we use a custom final tick, then I think this change would be confusing, because the user might think the command succeeded. If we actually hit the error case here, then in reality it means that the process was never started up in the first place, so I felt that just removing the spinner message from the terminal history made sense.
There was a problem hiding this comment.
Right, I still think it makes sense to unconditionally call .finish() so we leave the elapsed time in terminal history, which I think will be useful in both the success and error cases.
src/lib.rs
Outdated
| spinner.set_style( | ||
| ProgressStyle::with_template("{prefix} [{elapsed}]: {spinner:.green}") | ||
| .unwrap() | ||
| .tick_strings(&[".", "..", "...", " ..", " .", "", "✓"]), |
There was a problem hiding this comment.
Why deviate from the default style?
There was a problem hiding this comment.
Let's split this into two parts: the template and the spinner.
In terms of changing the template, I think there are some important deviations from the default:
- I don't believe there is a way to show the elapsed time in the default style template. I tried calling
with_elapsed(Duration::from_millis(0))on the progress bar instantiation using the default style, and I don't see anything. I'm not a power user of indicatif, so maybe I am doing it wrong, but setting the style with the template seemed like the best way to include the elapsed time. I feel strongly that including the elapsed time is important since I cannot find a way to implement a true progress bar. If there are better ways to include the elapsed time without changing the template, then we can use those. - The default style does not include a prefix that comes before the spinner. You can include a message which comes after the spinner, but I personally think that looks strange. I am used to seeing something of the form
<what is happening>: <status>. I don't feel as strongly about this one, so if you prefer the message after the spinner that is fine.
Given that, please let me know how you want to proceed with changing the template.
For the spinner itself, we can switch back to the default tick strings. I personally didn't like it in part because the bottom of the spinner doesn't line up with the bottom of the prompt, but any style or color works. Shall I go ahead and switch back to the default spinner?
There was a problem hiding this comment.
I think I'm fine with the template, was mostly responding to the custom spinner. Let's remove that for now? If you think the default spinner is bad, consider advocating a change upstream (where I'm also a maintainer).
There was a problem hiding this comment.
If you think the default spinner is bad, consider advocating a change upstream (where I'm also a maintainer).
I wouldn't say "bad", just the bottom of the spinner didn't line up with the bottom of my prefix text. No big deal, and possibly even specific to how my system is rendering it. Like I said before, I'm not a good person to consult on style decisions anyway!
Perf script can take a long time to run. This change adds an indicator to show that it is running so that the user knows that their application is not causing the terminal to hang. Addresses flamegraph-rs#270
gth828r
force-pushed
the
270.show-perf-script-running
branch
from
935f434
to
e0cd1d3
Compare
|
Thanks, this looks good to me. The audit failure in CI is unrelated. |
Perf script can take a long time to run. This change adds an indicator to show that it is running so that the user knows that their application is not causing the terminal to hang.
Addresses #270