-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
sar: add page #1532
sar: add page #1532
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the page! I've left a few comments 😺
pages/linux/sar.md
Outdated
|
||
> Monitor performance of various Linux subsystems. | ||
|
||
- Report I/O and Transfer rate every 1 second: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think Transfer
needs to have a capital letter at the beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, acutally the manpage had I/O and Transfer in upper-case, so i did the same.
anyways changed
pages/linux/sar.md
Outdated
|
||
- Report I/O and Transfer rate every 1 second: | ||
|
||
`sar -b 1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There aren't any long forms of the options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately not :(
pages/linux/sar.md
Outdated
|
||
`sar -u ALL 2` | ||
|
||
- Report 20 memory utilization statistics every 1 second: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit awkward. Perhaps we could reword this to something like ...statistics, one per second
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be "once per second" ? I think that sounds better and more natural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought, I think it should be changed to "once every second". Because the other examples are of the form "every 2 seconds", "every 5 seconds" and then you have "one per second", "one per 2 seconds".
Either we use "per second" everywhere or "every second" everywhere. But mixing the 2 does not sound right to me.
pages/linux/sar.md
Outdated
|
||
`sar -q 1 1` | ||
|
||
- Report paging statistics every 1 second: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could change this example to every 5 seconds to change things up? 😺
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap all of your variables in tokens. See the CONTRIBUTING page for how to do it.
Also, when you mention "Report n some statistics", what does it mean ? Does it mean it will print the statistics n times, every x seconds ?
pages/linux/sar.md
Outdated
|
||
> Monitor performance of various Linux subsystems. | ||
|
||
- Report I/O and Transfer rate every 1 second: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What transfer rate are we talking about ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
physical devices, added to the description
pages/linux/sar.md
Outdated
|
||
- Report I/O and Transfer rate every 1 second: | ||
|
||
`sar -b 1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap 1 in tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for missing it ...
fixed
pages/linux/sar.md
Outdated
|
||
`sar -n DEV {{2}} {{10}}` | ||
|
||
- Report CPU utilization every 2 seconds (press CTRL+C to quit): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to mention 'press ctrl-c to quit' after the first time. It's obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
pages/linux/sar.md
Outdated
|
||
`sar -u ALL {{2}}` | ||
|
||
- Report a total of 20 memory utilization statistics, one per second: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double space between "," and "one"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad :(
pages/linux/sar.md
Outdated
|
||
- Report the run queue length and load averages: | ||
|
||
`sar -q {{1}} {{1}}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the first 1 mean its once per second, should we mention that if that is the case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost every command has the same structure, the first arg is the refresh granularity, and the second (if any) is the total number of stats to print.
If the second arg is not provided, it assumes infinity.
So, I think this is obvious, and we should not write it explicitly to confuse people
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a bit confused because you mention 2 things - queue length and load average, and then there are 2 params. Its not very obvious whether the two "1"s are for granularity and no. of stats, or the granularity for queue length and load svg.
That's why I wanted to be extra clear in case there's any confusion. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lets just skip the second param, if anyone wants, it is present in the man page.
This would make it less confusing.
Hey @mfrw - left a couple of comments. Sorry for the back and forth. Our review process might feel a bit tedious. Thank you for the patience ! |
@agnivade your review process is amazing. I am getting to learn a lot of things. I am new to open-source. |
Thanks for the hard work, @mfrw! 😺 |
Fixes #1529.
The page (if new), does not already exist in the repo.
The page (if new), has been added to the correct platform folder:
common/
if it's common to all platforms,linux/
if it's Linux-specific, and so on.The page has 8 or fewer examples.
The PR is appropriately titled:
<command name>: add page
for new pages, or<command name>: <description of changes>
for pages being editedThe page follows the contributing guidelines