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

Feat/new stdout exporter switches (Add option to display more than five processes) #98

Merged
merged 7 commits into from Sep 18, 2021

Conversation

uggla
Copy link
Collaborator

@uggla uggla commented Apr 16, 2021

Hello,

This MR fixes #91.

@uggla uggla requested a review from bpetit April 16, 2021 23:06
Copy link
Contributor

@bpetit bpetit left a comment

Choose a reason for hiding this comment

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

It's great and works all fine, thanks a lot ! I'd just suggest to allow both options to run together (-p being a limit for the number of processes -r returns).

scaphandre stdout -p 20

You can filter the processes to display with `-r`. A warning will be risen if this option is used with `-p` at the same time.
In such case, `-p` behavior is disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd find more intuitive to pe able to use both. -p could then be used to limit the number of results from -r. Is there a particular challenge to do it I don't see right now ? WDYT ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello @bpetit,

No there is no challenge to mix the both options. However I made it "incompatible" because, the default value of -p is 5 even if you don't specify it. I thought it could be misleading, because it could truncate the filtered output silently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand. Actually it seems fine to me too to declare those options incompatible.

@bpetit bpetit added this to Triage in General Apr 23, 2021
@bpetit bpetit moved this from Triage to To do in General Apr 23, 2021
@bpetit bpetit moved this from To do to In progress in General Apr 23, 2021
@uggla uggla mentioned this pull request Apr 24, 2021
uggla added 6 commits May 4, 2021 21:39
- Introduce new switches -p (number of consumers) and -r (regex filter)
- Display the chosen number of consumers
- Start implementation of filter
- Filters implemented

TODO:
- Refactor get_filtered_processes().
- Explain that -f as priority over -p.
- Documentation update.
- Refactor get_filtered_processes() and get_top_consumers().
- Explain that -r as priority over -p.
- Add a warning if -p and -r are used at the same time.
- Remove default values for regex_filter.
- Update documentation.
- Fix clippy warnings
@uggla uggla force-pushed the feat/new_stdout_exporter_switches branch from 35e00fc to 394b81e Compare May 4, 2021 19:53
@uggla
Copy link
Collaborator Author

uggla commented May 4, 2021

@bpetit , this is rebased.

  • clippy ok
  • build ok

Despite I checked it, please have a quick new review to be sure the rebase has not introduced something wrong. Then I think you will be able to merge this MR.

@uggla uggla requested a review from bpetit May 25, 2021 23:28
@bpetit bpetit added this to the Release v0.4.0 milestone Sep 11, 2021
@bpetit bpetit merged commit c22b672 into hubblo-org:main Sep 18, 2021
General automation moved this from In progress to Done Sep 18, 2021
@bpetit bpetit moved this from Done to Previous releases in General Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
General
Previous releases
Development

Successfully merging this pull request may close these issues.

Add option to display more than five processes (stdout exporter)
2 participants