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

Refactor console UI handling outside of the cmd package #2787

Merged
merged 4 commits into from
Dec 19, 2022

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Nov 30, 2022

This is a refactor I started while working on #2459 (which I'm still working on based on this branch), and evolved into moving all most functionality that handles interacting with the console outside of the cmd package.

See the individual commits for details. Apologies for the large PR ☺️ While I could make separate PRs for each commit, I think it makes more sense reviewed in the final state. Most of the changes are mechanical file moves, so it's mostly a noisy diff.

This involves:

  • Creation of a new ui/console package and Console struct that encapsulates synchronized writing to stdout/stderr, and all previous print* functions.
    This decouples the cmd package from any console UI related behavior, which can be accessed via globalState.console.
    Another benefit is that handling of color output is mostly done in a single place, and we avoid the numerous isTTY and noColor checks that were in many places in cmd before.
  • Consolidating most console related functionality into subpackages of ui/console. So ui.Form is now in ui/console/form, and ui/pb in ui/console/pb.
    This makes it clear that these packages are console-only, while freeing up the ui namespace to potentially add other UIs to k6 (resurrect the old web UI, maybe? 😉).

There are some leftover things I'd like to address eventually, though. Primarily, the coupling of executors with progress bars. It doesn't make sense for these low-level components to be interacting with a UI element directly. Even though pb is effectively standalone and decoupled from any OS dependencies, the way the rendering is done is very much specific to a console output. Instead, there should be an intermediate Progress type that executors can return, and pb can read. I didn't get into it here, as it would expand the scope even more, but I can create an issue for it if there's agreement that it should be done. Let me know.

Note that this isn't part of the v0.42.0 cycle, so it can be reviewed during the cooldown phase or later.

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Did the first round of review, generally looks good.

A bunch of the comments TODO cross-platform check ❓ me, are we going to do that before merging, right?

ui/console/console.go Outdated Show resolved Hide resolved
ui/console/console.go Outdated Show resolved Hide resolved
cmd/cloud.go Outdated
@@ -64,13 +63,13 @@ func (c *cmdCloud) preRun(cmd *cobra.Command, args []string) error {
// TODO: split apart some more
//nolint:funlen,gocognit,cyclop
func (c *cmdCloud) run(cmd *cobra.Command, args []string) error {
printBanner(c.gs)
c.gs.console.Printf("\n%s\n\n", c.gs.console.Banner())
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why haven't we checked for the c.gs.flags.quiet here?

And the more general thing is there a further plan to also encapsulate this check somewhere in the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I overlooked it, that's why 😅 Nice catch!

Fixed in 2298299.

I'm against encapsulating the check, since it's more readable to have the logic that decides whether to do something (print the banner in this case) outside of any functions, unless the check is overly verbose or is used in many places. Previously, printBanner() did that check internally, but now it's clearer that the banner is only printed if quiet wasn't enabled.

In general, I'm not a fan of the pattern of calling a function that returns right away depending on some check, especially if that check can be done by the caller. Functions should do what it says on the tin; nothing more and nothing less. 🙂

We could always have another maybePrintBanner() or printBannerUnlessQuiet() function, but I think that's a bit silly considering it only saves us 2 lines, and it's called from 2 places.

Notice that I also didn't encapsulate the actual printing of the banner, since calling console.Printf() directly is also clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm against encapsulating the check, since it's more readable to have the logic that decides whether to do something

I see where you are coming from, but generally can't agree 😅

Looking at all usages of the Banner right now, we see everywhere the if statement, and considering it, that likely there will be no place that won't use the same logic (maybePrintBanner) having these if are unnecessary details. Moreover, it can lead to errors because the maintainer/contributor can easily forget to put in the additional check before placing the banner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer favoring clarity, even at the expense of verbosity or duplication, so I don't see the external check as an unnecessary detail. The only reason I forgot to add it in this case is because I removed the printBanner() function, and forgot to update this usage of it. Since it only happens in 2 places, I doubt this would happen again.

But since you feel strongly about it, I added a maybePrintBanner function to cmd in 6ca990a. I also noticed the same issue with printing the cloud progress bar, so also added maybePrintBar. Let me know if this addresses your concern.

I tested the behavior of k6 cloud, with and without --quiet, and it now matches the behavior on master-next.

@imiric imiric changed the base branch from master to master-next December 7, 2022 17:47
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Merging #2787 (1c02d37) into master-next (a2ab89a) will increase coverage by 0.01%.
The diff coverage is 55.45%.

❗ Current head 1c02d37 differs from pull request most recent head 6ca990a. Consider uploading reports for the commit 6ca990a to get more accurate results

@@               Coverage Diff               @@
##           master-next    #2787      +/-   ##
===============================================
+ Coverage        76.11%   76.13%   +0.01%     
===============================================
  Files              212      214       +2     
  Lines            16504    16515      +11     
===============================================
+ Hits             12562    12573      +11     
- Misses            3173     3179       +6     
+ Partials           769      763       -6     
Flag Coverage Δ
ubuntu 76.02% <55.45%> (+0.05%) ⬆️
windows 75.87% <54.24%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/cloud.go 12.83% <0.00%> (-0.58%) ⬇️
cmd/convert.go 68.29% <0.00%> (ø)
cmd/inspect.go 26.53% <0.00%> (ø)
cmd/login_cloud.go 20.00% <0.00%> (+0.23%) ⬆️
cmd/login_influxdb.go 11.66% <0.00%> (ø)
cmd/pause.go 37.50% <0.00%> (ø)
cmd/resume.go 37.50% <0.00%> (ø)
cmd/scale.go 40.00% <0.00%> (ø)
cmd/stats.go 42.85% <0.00%> (ø)
cmd/status.go 42.85% <0.00%> (ø)
... and 33 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@imiric
Copy link
Contributor Author

imiric commented Dec 7, 2022

Thanks for the review @olegbespalov!

A bunch of the comments TODO cross-platform check ❓ me, are we going to do that before merging, right?

That's all cruft from the existing implementation, which I'm not planning to touch here 😅 The state of our UI, how we print progressbars, etc., is still a mess, but refactoring that and addressing all those TODOs is a much larger and trickier task that what was done here.

Comment on lines +40 to +43
outMx := &sync.Mutex{}
outCW := newConsoleWriter(stdout, outMx, termType)
errCW := newConsoleWriter(stderr, outMx, termType)
isTTY := outCW.isTTY && errCW.isTTY
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this mostly assumes that both stdout and stderr will either be a TTY or not together. I.e. that there won't be a case where stdout is a TTY, but stderr isn't. Previously we were checking this separately.

Is this a safe assumption to make? In that case, I might just pull out the check from newConsoleWriter() and do it only once here, for either stdout or stderr, and remove the isTTY field from consoleWriter.

WDYT?

imiric pushed a commit that referenced this pull request Dec 12, 2022
Resolves #2787 (comment)

Also fixes the issue with printing the cloud progress bar conditionally,
and removes the ModifyAndPrintBar method.
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@imiric imiric merged commit 5070a2c into master-next Dec 19, 2022
@imiric imiric deleted the refactor/split-ui-cmd branch December 19, 2022 10:11
// is properly synchronized: currently when a test is aborted during the
// init phase, some logs might be emitted after the above command returns...
// see: https://github.com/grafana/k6/issues/2790
ts.outMutex.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I should have reviewed the PR 😞 This should not have been removed here - the fact that there was a data race was actually quite valuable! We want to get data races in tests when we don't expect unsynchronized access, this is super useful.

loggerHook *testutils.SimpleLogrusHook

cwd string

expectedExitCode int
}

// A thread-safe buffer implementation.
type safeBuffer struct {
Copy link
Member

Choose a reason for hiding this comment

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

As I wrote in the other comment, I think this is wrong. We don't want a safe and synchronized buffer here, we want the exact opposite. We want tests to fail with a data race when we have unsynchronized access to the stdout, e.g. if multiple things are concurrently writing there. Or, as it happened in the recent integration tests, if trying to read it after an integration test was supposedly done resulted in a data race because some goroutine wasn't done and was still trying to write.

These are super valuable revelations we are masking, not actually fixing, by using locking here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned on Slack, this synchronizes writing with reading, which is only useful for tests. I.e. we never need to inspect the contents of stdout in the real world, so this is unique for tests.

I don't think we're masking anything with this. We we're manually locking stdout before so we can read from it, and this simply pushes it down the stack so it happens transparently for the test.

Copy link
Member

Choose a reason for hiding this comment

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

The fact is that whenever a test wants to inspect the contents of stdout or stderr when it knows they might be written to (i.e. in the middle of the test), it can manually lock them. But when it wants to inspect them after the test is supposed to be finished, as is the case for the majority of assertions in the integration tests, it's useful to access them directly, without locking.

That way, if there is a data race, we will be able to see it, and it will most likely be a great signal that we have an actual bug. That is, we want the test to fail! For example, if some forgotten goroutine that is still active and logs stuff after the test is supposed to be finished, as was the case for the FIXME comment you deleted. Discovering that data race prompted me to open #2790. And this bug was not fixed in this PR, so you shouldn't have deleted the FIXME, it was just masked by this safeBuffer. The bug was actually fixed by #2800, which deleted the stdout locking and FIXME comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, understood. I saw the fix in #2800, but didn't think it was a big deal to handle it this way.

How do you want to proceed? Another reason I added safeBuffer is because the stdout mutex is no longer exposed to GlobalState after the changes here, so tests wouldn't have access to it. I also prefer not having to deal with this locking in the many tests here that inspect stdout while the test is running.

I won't have time to look into this today, but feel free to create a PR if you have a good idea for handling this.

Copy link
Member

@na-- na-- Dec 23, 2022

Choose a reason for hiding this comment

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

I've started working on a PR, but it's likely going to be ready after the holidays. 😞

many tests here that inspect stdout while the test is running.

Are there really that many tests? There are 5 outMutex.Lock() calls that are removed here, one of which is the FIXME under my other comment, another is fixed by another PR, one seems unnecessary (in TestPrometheusRemoteWriteOutput()) and the last 2 are combined in a helper function by d5124d1 (from #2800)

So, by my reading, after we fix the actual bugs, there will be 1 or 2 necessary lockings the stdout mutex in the integration tests, used only in the few places where we actually care about the stdout contents mid-test.

imiric pushed a commit that referenced this pull request Jan 9, 2023
* Revert "Move console form to ui/console/form package"

This reverts commit 6ade203.

* Revert "Move ui/pb package to ui/console/pb"

This reverts commit 8f81858.

* Revert "Move CLI UI to ui/console package"

This reverts commit b0e0243.

* Run go mod tidy

* Run gofmt -s

* Add go doc to ui package

The linter complained about it missing.
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.

5 participants