Add responsive progress bars #1332
Conversation
cmd/ui.go
Outdated
| // Add a small delay to allow executors time to process | ||
| // the done context, so that the correct status symbol is | ||
| // outputted for each progress bar. | ||
| time.Sleep(50 * time.Millisecond) |
There was a problem hiding this comment.
Any ideas for handling this... gracefully? :D This seemed like a quick fix for an annoying issue.
There was a problem hiding this comment.
Is this a problem only with Ctrl+C? Or even in normal script execution endings?
There was a problem hiding this comment.
No, it's only a problem with ^C.
There was a problem hiding this comment.
I have to dig in deeper, but I think that means we should probably address the cancellation chain after Ctrl+C. Maybe something like this - Ctrl+C aborts the Engine context, but not the one passed to the progressbar rendering code - it is aborted only after the engine returns, either normally or when interrupted?
There was a problem hiding this comment.
That makes sense. I'll take a look at how that could be accomplished.
Related to ^C, what do you think about this comment now that #971 is fixed?
I like the idea that the first ^C would trigger a "soft" shutdown ("SIGTERM", running something like gracefulStop() to allow all iterations to complete), but a second ^C would interrupt that and stop immediately ("SIGKILL"). Could we plan this as part of #1279?
There was a problem hiding this comment.
I no longer think we should wait for iterations to complete on a first Ctrl+C interruption - it could potentially take minutes, and would usually be at least several seconds. Also, it would add significant implementation complexity for a dubious benefit. I can't think of a valid use case of why someone would care if an iteration doesn't complete when they have interrupted k6 with Ctrl+C...
In fixing the Ctrl+C handling, beyond the progressbar fix I mentioned above, I think we should modify the code like this:
- The first Ctrl+C does what it currently does - interrupts the context for the
Engine/ExecutionSchedulerand waits for them to finish. Mind you, this is still graceful in a sense, we're interrupting the k6 execution in a very orderly manner and nothing is lost. Especially after your progressbar fixes, it's going to be very apparent when executors are interrupted... - The second Ctrl+C directly calls
os.Exit()with some predefined exit code. This would prevent anything like k6 gets stuck on a broken h2c connection upgrade #971 happening again. Or, rather, if something like that happens, it's not going to be a big issue.
There was a problem hiding this comment.
So 1d28f2b unintentionally fixed this issue altogether! 🎉
No need for hacks and special Ctrl+C handling, all progress bar status symbols are rendered correctly now. I'm puzzled as to why that is exactly, but didn't look further into it.
So I don't think we need this refactor anymore. :) Test from the latest push and let me know.
Codecov Report
@@ Coverage Diff @@
## new-schedulers #1332 +/- ##
==================================================
- Coverage 75.44% 75.33% -0.12%
==================================================
Files 160 162 +2
Lines 12507 12579 +72
==================================================
+ Hits 9436 9476 +40
- Misses 2576 2607 +31
- Partials 495 496 +1
Continue to review full report at Codecov.
|
cmd/options.go
Outdated
| @@ -89,6 +90,12 @@ func optionFlagSet() *pflag.FlagSet { | |||
| flags.StringSlice("tag", nil, "add a `tag` to be applied to all samples, as `[name]=[value]`") | |||
| flags.String("console-output", "", "redirects the console logging to the provided output file") | |||
| flags.Bool("discard-response-bodies", false, "Read but don't process or save HTTP response bodies") | |||
| flags.String("ui-mode", UIModeResponsive.String(), | |||
There was a problem hiding this comment.
Hmm I'm wondering if this is even necessary... In my mind the important distinction to make is between interactive (i.e. a terminal) and non-interractive (i.e. CI) mode, and we mostly can and do make that distinction automatically...
I don't think there's a big reason to add this extra option... What's a use case for not wanting responsive progress bars when you have a proper terminal? @mstoykov, thoughts?
There was a problem hiding this comment.
Hhmm true. I suppose we can detect if we're running in non-interactive mode easily enough and adapt accordingly, but can we rely on all CIs to run k6 non-interactively?
Having an option makes this easier to control / override regardless of the environment, which seems safer. That said, I also can't think of a solid reason for turning off responsiveness if running in an interactive terminal, unless someone is annoyed by it or it doesn't work great in their terminal.
There was a problem hiding this comment.
I think it is better for there to be the option to disable/enable it
There was a problem hiding this comment.
Again, this has nothing to do with whether we're running on a CI or not, this is determined here https://github.com/loadimpact/k6/blob/92a9716e6b3ac3574cd9a449e0afdad584c75b5a/cmd/ui.go#L250-L255
That said, I'm not strictly against having such an option, I just don't see a lot of reasons why someone would choose full or compact over responsive.
I think there would be a lot more value in this option, if we had log as one of the possible values. Then, instead of any progressbars, k6 can just emit execution information as log messages, making it even more suitable for CI. Ideally (post-#883), it'd be nice to have a way to specify that 1s/100ms refresh rate I linked to above, say --ui-mode="log,5s" or something like that.
There was a problem hiding this comment.
Hey, what can we decide here?
I'm not against having responsive mode always, and repurposing --ui-mode for changing how output is handled. I like being able to specify log-mode only, but maybe the refresh rate could be a separate option, which would work for all "UI modes", and we can avoid the string splitting. Something like --ui-mode=log --ui-refresh-rate=5000. But that's definitely for another PR, so create an issue for it?
In case we go for the above, I would remove --ui-mode from this PR, but that still leaves users with no way to force compact mode, for CI scenarios, which was the main reason for adding it.
@na-- Can we rely on that to always be false in all CI environments? It seems like having explicit control over this behavior would be safer.
cmd/ui.go
Outdated
| // Add a small delay to allow executors time to process | ||
| // the done context, so that the correct status symbol is | ||
| // outputted for each progress bar. | ||
| time.Sleep(50 * time.Millisecond) |
There was a problem hiding this comment.
Is this a problem only with Ctrl+C? Or even in normal script execution endings?
See [1]. This also accidentally fixes [2], and the time.Sleep hack is not needed! [1]: #1332 (comment) [2]: #1332 (comment)
imiric
force-pushed
the
feat/1007-1279-responsive-progress-bars
branch
from
92a9716
to
b9a377e
Compare
Codecov Report
@@ Coverage Diff @@
## new-schedulers #1332 +/- ##
==================================================
- Coverage 76.32% 76.18% -0.14%
==================================================
Files 161 162 +1
Lines 12625 12683 +58
==================================================
+ Hits 9636 9663 +27
- Misses 2479 2510 +31
Partials 510 510
Continue to review full report at Codecov.
|
imiric
force-pushed
the
feat/1007-1279-responsive-progress-bars
branch
from
d214157
to
548b617
Compare
There was a problem hiding this comment.
This looks very good to me, but I want just one major change - to remove the ui-mode option for now. That is, remove the code from here and add an issue to add something like it again in the future.
My biggest reason for wanting to remove the option now is that I think we still don't know precisely how a good and flexible solution here looks like. I think the current one isn't it, since I can find plenty of issues and missing things in it. And if we leave it in as it is, we might be tying our hands with backwards compatibility to implement a better solution in the future for a very small amount of benefit now.
Here are some of the issues with and missing things in --ui-mode that I can think of at the moment:
-
I don't think the
uiModeis something that should be controlled from the exported scriptoptions. For one - it only matters fork6 run, notk6 cloud. Again, conceptually this is much closer to--logformat,--quietand--console-outputthan it is to--vus,--duration, etc. - it is not an inherent property of the load test, but an option that configures extra k6 behavior that doesn't affect the load test in any way. -
I'm not sure the current name is the best one. "UI mode" suggests more power than this option possesses, since it affects only the progress bars, and in very limited ways at that.
-
We would probably actually want to give this option (or add others) more power, because here are the issues it currently can't deal with:
3.1. It's completely plausible to have test runs with dozens of executors, but don't have a nice way to limit the amount of progressbars we're showing.
3.2. As mentioned elsewhere in the PR, using log messages for progress instead of pseudo-GUI progressbars is probably preferred in CI environments, but we don't have a way to specify that. And when we do have such a way, the current--ui-modevalues wouldn't make a lot of sense with it.
3.2. We don't have a way to specify the refresh rate - we have fixed 1s / 100ms, but we have had requests for that to be configurable, especially for CI
3.4. You can't specify something like a fixed window size.
3.5. We have to figure out how all of these things interact with--quietand--verbose.
3.6. I think the current form of the option, especially it being inlib.Options, exacerbates Configuration issues #883
So, I think we should open an issue and discuss a flexible solution (i.e. a new option or a set of options) that tackle all of these issues and more, before we commit to specific solution. Until then, the current default UI mode responsive for interactive terminals and compact for CIs (i.e. !stdoutTTY) seems like a more than adequate solution.
Progress bars will now dynamically resize depending on terminal window width, being replaced with percentages if "squished" too far. It works best on *nix platforms because of SIGWINCH, while on Windows retrieving the size is required before each render iteration, and isn't as responsive. Terminal multiplexers are a bigger issue: tmux seems to buffer SIGWINCH/terminfo and is plain broken on versions including the current latest stable (3.0a) (see issue #2005[1]). This was fixed on `master`, but is yet to be released, and even with the fix the experience is far from ideal (lags quite a bit, no continuous resize). GNU screen also has some rendering issues I haven't looked too much into yet. Untested on macOS terminals, but I expect it to work as well as on Linux. Terminals tested: - Linux: st, urxvt, xterm - Windows: cmd.exe and PowerShell standalone, and both inside the new Windows Terminal[2] app. Resizing works fine in all cases, though isn't as responsive as on Linux. Part of #1279 [1]: tmux/tmux#2005 [2]: https://github.com/microsoft/terminal
See [1]. This also accidentally fixes [2], and the time.Sleep hack is not needed! [1]: #1332 (comment) [2]: #1332 (comment)
Progress bars will be responsive by default, and fixed compact (showing percentages) only in non-interactive terminals (e.g. CI). Resolves #1332 (review)
imiric
force-pushed
the
feat/1007-1279-responsive-progress-bars
branch
from
548b617
to
ff5eab2
Compare
|
@na-- see ff5eab2 for the I'm still not sure if this will work correctly in CI environments, though. I haven't been able to make |
It's very easy to do, just run
it should be fine in 99% of cases, but I agree, it would be better if you can set this explicitly as well. You can consider that reason 3.7. in the list above 😄 That is, we can discuss how to handle this use case in the new issue for
👍 I'll take a look at this again after I finish reviewing your other PR |
There was a problem hiding this comment.
This LGTM now, with 2 very minor issues. One I noted noted in-line, another one I found is that we have superfluous logging in https://github.com/loadimpact/k6/blob/17c92555aea12972ea999612b30c3b91ab455762/cmd/run.go#L280-L283
Run k6 run script.js 2>&1 | cat to see what I mean. This can be substituted by the future --ui-mode=log or whatever we decide to add.
cmd/ui.go
Outdated
| } | ||
| updateTermWidth() |
There was a problem hiding this comment.
A minor issue, and maybe also what @mstoykov referenced above, but this select seems strange. I think it shouldn't have a default, but only 3 normal cases:
- context is done, in which case it does what it currently does and exits
- received a signal from the ticker, render progress bars
- received a
winchsignal from, render progress bars
not a big deal right now with the 1s/100ms refresh rate, but if we allow setting custom refresh rates, this may hold up the script after it should be aborted, because if the context is interrupted while you wait on the timer, you're not going to abort immediately.
There was a problem hiding this comment.
Hm ... might've confused how this should work and made things worse ... but yeah I also find it strange.
IMO updateTermWidth should go ... and the code should be :
var winch chan os.Signal
if sig := getWinchSignal(); sig !=nil {
winch = make(chan os.Signal, 1)
signal.Notify(winch, sig)
}
for {
select {
case<-cxtDone:
//what is currently + maybe locking outMutex ?
case <-winch: // it is okay if it is nil in the case of the
termWidth, _, _ = terminal.GetSize(fd)
case <-ticker.C:
if winch == nil {
termWidth, _, _ = terminal.GetSize(fd)
}
}
// the rendering things
}my whole previous thought process (if I remember correctly) was that we don't want to at all do anything on ticker.C if winch is defined which is not true 🤦♂️ even if the size isn't changing we still want to update the progressbars, we just don't want to get the size if there is no need :D
There was a problem hiding this comment.
Thanks guys, it does make more sense to use a single select. I was trying to avoid checks in the for loop, but this is simpler. I'm not sure if we need to lock outMutex, but I suppose it's safer.
See 886859a.
There was a problem hiding this comment.
LGTM now. outMutex would still need to be locked, otherwise something else (like the logger) could write something in the middle of the progressbars, no?
imiric
force-pushed
the
feat/1007-1279-responsive-progress-bars
branch
from
40d2caf
to
886859a
Compare
|
@na-- Re: superfluous logging, would you like to get rid of that entire |
yeah, the whole |
With the new progress bars this logging is not needed, though we might bring it back once something like `--ui-mode=log` is implemented. Resolves #1332 (review)
|
OK, removed the logging in a64a3bc. One thing I noticed when running with Should I remove this warning as well? |
Sort of - I think we should show that warning only if |
With the new progress bars this logging is not needed, though we might bring it back once something like `--ui-mode=log` is implemented. Resolves #1332 (review)
imiric
force-pushed
the
feat/1007-1279-responsive-progress-bars
branch
from
a64a3bc
to
1d34063
Compare
|
OK, hopefully everything is resolved in 1d34063. |
This PR makes progress bars responsive to the terminal window width, replacing them with percentages when the shortest possible length is reached. See the individual commits for details.
Demo on st: https://streamable.com/nrgs0
Best results are achieved on GNU/Linux terminals because of SIGWINCH, but it works reasonably well in Windows. The poorest performance is under any terminal multiplexer, but I'm not sure how concerned we should be about that, or well, if there's anything we can do about it. Untested on macOS terminals: testers needed! I've read about SIGWINCH support in some BSDs, so I'm hoping it works well in macOS too.
The rendering will be responsive by default, and in non-interactive terminals (e.g. CI) will fallback to a fixed compact mode (showing only percentages).
Part of #1279