Add cg_stats component #33
Conversation
|
Hey, thanks for the contribution! The one thing that caught my eye were the two dependencies.
Neither of these are deal breakers for getting this merged for me, and if it's not worth it to you I'm happy to merge this as is. Just wanted to ask about both of those |
|
You're right, that adding the dependencies is not very useful in this case. I used I also rebased this on |
src/components/cg_stats.rs
Outdated
| }; | ||
| // Read statistics of system services and shorten too long names, e.g., | ||
| // docker-dcd9a8c71b756de71a4a837c005840f84e0ed92574704ae1c89409c57980aaee.scope | ||
| let re = Regex::new(r"\.service|\.scope|\.slice").unwrap(); |
There was a problem hiding this comment.
How about lazy static for these regexes?
There was a problem hiding this comment.
Good idea. Then even the closure could be implemented as a separate function, which can be tested with unit tests.
|
Converted to lazy_static. I also added tests. Not sure how they will behave on non-linux systems. |
|
I don't know of anyone using this on non-linux systems so that's probably fine. 😄 |
src/components/cg_stats.rs
Outdated
| } | ||
| fs::write(&self.state_file, toml::to_string(&now)?) | ||
| .map_err(|e| CgStatsError::FileError(PathBuf::from(&self.state_file), e))?; | ||
| let min_width = INDENT_WIDTH + prepared_cg_stats.max_name_width + 12 + 5; |
There was a problem hiding this comment.
Where does + 12 + 5 come from?
There was a problem hiding this comment.
I will change it to:
let min_width = INDENT_WIDTH
+ prepared_cg_stats.max_name_width
+ "100%".len()
+ "[=========]".len()
+ 2; // spaces
src/components/cg_stats.rs
Outdated
| }; | ||
| // Read statistics of system services and shorten too long names, e.g., | ||
| // docker-dcd9a8c71b756de71a4a837c005840f84e0ed92574704ae1c89409c57980aaee.scope | ||
| let re = Regex::new(r"\.service|\.scope|\.slice").unwrap(); |
There was a problem hiding this comment.
Couldn't this one also be lazy static?
src/components/cg_stats.rs
Outdated
| .unwrap_or((self, Some(Constraints { min_width: None }))) | ||
| } | ||
| async fn print(self: Box<Self>, _global_config: &GlobalConfig, _width: Option<usize>) { | ||
| println!("Cgroup Statistics component failed"); |
There was a problem hiding this comment.
I find it a bit confusing to have two different structs each implement half of the Component trait. Maybe there is a way to consolidate this into one Component struct that holds the configuration and the prepared data or something like that. What do you think?
There was a problem hiding this comment.
Maybe adding #[serde(skip)] to non-config data would help. Will look into it.
It prints CPU usage since the last run dissected by systemd cgroups,
i.e. by users and services. The output can look like this:
CPU usage in the past 5m:
Users:
johndoe 2% [#=======================================]
root 4% [##======================================]
Services:
apache2 7% [###=====================================]
cron 11% [####====================================]
icinga2 2% [#=======================================]
munin-node 2% [#=======================================]
If no cgroup reports usage higher than a threshold (e.g. 1%), the
output is:
CPU usage in the past 5m: almost idle
|
Now, there is just one component implementing both |
|
Thank you very much for implemented the comments. I have to think more about the 1 component thing. I'd prefer something that follows "parse, don't validate". However, I think that would be a bigger refactor that touches other components, so I'd say for now it's fine. |
It prints CPU usage since the last run dissected by systemd cgroups, i.e. by users and services. The output can look like this:
If no cgroup reports usage higher than a threshold (by default 1%), the output is: