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

Add filter flag documentation and examples to help message #1203

Merged
merged 2 commits into from Sep 26, 2023

Conversation

glrf
Copy link
Contributor

@glrf glrf commented Sep 6, 2023

The Hubble CLI provides multiple different flags to filter the returned
flows. However how these filters are combined is not entirely obvious.

This commit adds more detailed help message and multiple examples that
explain how these filters interact if more than one filter flag is
provided.

I struggled to make this concise and understandable, so I added more detailed examples. I'm happy about any input to improve this and make it clearer.

@maintainer-s-little-helper
Copy link

Commit 03677e3 does not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/contributing/#developer-s-certificate-of-origin

@glrf glrf force-pushed the doc/filters branch 2 times, most recently from eb50790 to 74ea417 Compare September 7, 2023 07:38
@glrf glrf marked this pull request as ready for review September 7, 2023 07:40
@glrf glrf requested a review from a team as a code owner September 7, 2023 07:40
@glrf glrf requested review from chancez and removed request for a team September 7, 2023 07:40
Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

Overall a good improvement. I left some comments around grammar and terminology that should be updated as well. My only concern is this is making the already really long --help even longer, but I think this is fine.

cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
@glrf
Copy link
Contributor Author

glrf commented Sep 8, 2023

My only concern is this is making the already really long --help even longer, but I think this is fine.

That's a fair point. The help message is very long now. If this grows even more, it might be a good idea to move the examples to the docs.cilium.io and just link to them in the help message?

@glrf glrf requested a review from chancez September 8, 2023 07:35
@chancez
Copy link
Contributor

chancez commented Sep 8, 2023

@gandro @kaworu any thoughts on if we're okay with adding some more usage examples to the help test?

@kaworu kaworu linked an issue Sep 11, 2023 that may be closed by this pull request
@gandro
Copy link
Member

gandro commented Sep 11, 2023

@gandro @kaworu any thoughts on if we're okay with adding some more usage examples to the help test?

Yeah, I'm also a bit worried about help page length. On the other hand, I do think those examples brings a lot of value to users who are calling hubble observe --help for the first time. And I think having it displayed to users when they are actively looking for help is a nice way to discover it too (since we have to assume that many people will not read or find the docs as quickly as they will try --help).

It's mostly users who already know the basics of Hubble for which this adds a lot of visual clutter, making it harder to quickly look up the reference for a flag they are looking for.

Maybe an option would be to maybe have short and long help flag? That I think would cover both cases: Users who are learning Hubble and want a more verbose help page, vs. users who just want to quickly know about the syntax or name of a particular flag.

@kaworu kaworu added 📄 area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. labels Sep 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label PR is blocked until the release note is set labels Sep 12, 2023
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks @glrf! Couple of typos and nits on the way.

Overall LGTM, agree with @gandro that a short and long help would be best.

cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
cmd/observe/flows.go Outdated Show resolved Hide resolved
The Hubble CLI provides multiple different flags to filter the returned
flows. However how these filters are combined is not entirely obvious.

This commit adds more detailed help message and multiple examples that
explain how these filters interact if more than one filter flag is
provided.

Fixes: cilium#433

Signed-off-by: Fabian Fischer <fabian.fischer@isovalent.com>
Signed-off-by: Fabian Fischer <fabian.fischer@isovalent.com>
@glrf
Copy link
Contributor Author

glrf commented Sep 12, 2023

Thanks for all the input on the help text. I often struggle with technical writing so I'm always happy about feedback.

And about the short and long help flag. I think I agree that this sounds smart, but when I actually tried to implement this I struggled to find a clean solution. I can see three ways of doing this and all three have pros and cons

  • Make -h,--help the short help flag and introduce something like --long-help or --detailed-help or --explain that shows the long help text. The disadvantage here would be that new users might just not see it. Similar problem to moving it to the docs and linking.
  • Make -h,--help the long help flag and introduce something like -b,--brief-help or mabye --ref(for reference? Or IDK naming is hard). The disadvantage here is that most people will likely just continue using -h and live with the clutter, as they're just used to it.
  • Make -h show the short help text and --help show the long help text. I could make this work, but it felt like a hack and I don't know if that's too magical and will confuse users.

Any opinions?

@chancez
Copy link
Contributor

chancez commented Sep 18, 2023

Another alternative is a new sub-command like hubble observe usage-examples or something?

I'm personally okay with updating the existing --help and then we can always follow up with another PR to move the usage examples out of the --help and into somewhere else.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 25, 2023
@kaworu kaworu merged commit ef6dde0 into cilium:main Sep 26, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: improve help message with regards to how filters are applied
4 participants