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.

Sign up for GitHub

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

Jump to bottom

improve loadtest watch cmd #44844

Merged
merged 1 commit into from
Aug 13, 2024
Merged

improve loadtest watch cmd #44844

merged 1 commit into from
Aug 13, 2024

Conversation

fspmarshall
Copy link
Contributor

@fspmarshall fspmarshall commented Jul 30, 2024 •
edited
Loading

Improves the usefulness of the tctl loadtest watch command in debugging by adding filtering by event op type, and the option to output a stream of json objects. Ex:

$ tctl loadtest watch --kind=user --ops=del
DEL: user/alice
DEL: user/bob
...

$ tctl loadtest watch --kind=node --ops=put --format=json | jq -r '.resource.spec.hostname'
node-1.example.com
node-2.example.com
...

@fspmarshall fspmarshall added backport/branch/v14 no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 backport/branch/v16 labels Jul 30, 2024
@github-actions github-actions bot added size/sm tctl tctl - Teleport admin tool labels Jul 30, 2024
@github-actions github-actions bot requested review from Joerger and strideynet July 30, 2024 21:09
@fspmarshall fspmarshall force-pushed the fspmarshall/improve-loadtest-watch branch from 65f003e to 600bf77 Compare August 2, 2024 16:39
zmb3
zmb3 approved these changes Aug 6, 2024
View reviewed changes
tool/tctl/common/loadtest_command.go Outdated
@@ -76,6 +81,8 @@ func (c *LoadtestCommand) Initialize(app *kingpin.Application, config *servicecf

c.watch = loadtest.Command("watch", "Monitor event stream").Hidden()
c.watch.Flag("kind", "Resource kind(s) to watch, e.g. --kind=node,user,role").StringVar(&c.kind)
c.watch.Flag("ops", "Operations to watch, e.g. --ops=put,del").Default("put,del").StringVar(&c.ops)
c.watch.Flag("format", "Output format").Default(teleport.Text).StringVar(&c.format)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be an EnumVar?

Same goes for ops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated format to be an EnumVar. ops doesn't map as nicely though since it's intended to be a comma-separated list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't that what EnumsVar is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EnumsVar allows for something like tctl loadtest watch --ops=del --ops=put but doesn't support lists like tctl loadtest watch --ops=put,del. The use of comma-separated list parameters is the goto way to handle flags that can take multiple values across the rest of teleport (token types, request ids, label selectors, etc). I'd rather keep it consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. I had hoped it would support comma separated, as I agree that's more ergonomic.

@fspmarshall fspmarshall force-pushed the fspmarshall/improve-loadtest-watch branch from 600bf77 to 9b2c4b4 Compare August 7, 2024 16:07
@fspmarshall fspmarshall enabled auto-merge August 7, 2024 18:55
@fspmarshall fspmarshall force-pushed the fspmarshall/improve-loadtest-watch branch from 9b2c4b4 to 20d1526 Compare August 12, 2024 21:18
@fspmarshall fspmarshall force-pushed the fspmarshall/improve-loadtest-watch branch from 20d1526 to db8fcad Compare August 13, 2024 15:32
@fspmarshall fspmarshall added this pull request to the merge queue Aug 13, 2024
Merged via the queue into master with commit fb967d8 Aug 13, 2024
35 checks passed
@fspmarshall fspmarshall deleted the fspmarshall/improve-loadtest-watch branch August 13, 2024 16:12
@public-teleport-github-review-bot
Copy link

@fspmarshall See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Create PR
branch/v16 Create PR

This was referenced Aug 13, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosstimothy rosstimothy rosstimothy approved these changes

@zmb3 zmb3 zmb3 approved these changes

Assignees
No one assigned
Labels
backport/branch/v14 backport/branch/v15 backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry size/sm tctl tctl - Teleport admin tool
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fspmarshall @zmb3 @rosstimothy