Skip to content

Conversation

FengPan-Frank
Copy link
Contributor

@FengPan-Frank FengPan-Frank commented Mar 6, 2025

rust version implementation of procdockerstatsd

Why I did it

rust version implementation of procdockerstatsd to save memory utilitization.

How I did it

How to verify it

image
image

memory comparison between python and rs implementation
image

image

@mssonicbld
Copy link

/azp run

Copy link

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@mssonicbld
Copy link

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FengPan-Frank FengPan-Frank requested a review from qiluo-msft March 6, 2025 12:08
@qiluo-msft qiluo-msft requested a review from maipbui March 6, 2025 23:43
Copy link
Contributor

@qiluo-msft qiluo-msft Mar 7, 2025

Choose a reason for hiding this comment

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

2024

Should it be 2025? minor issue. #Closed

Copy link

Choose a reason for hiding this comment

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

this is the rusttoml edition, and yea, it is 2024 for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

127

magic string. May reuse rust lib for swss-common, and get the SonicDBConfig.

Copy link
Contributor

Choose a reason for hiding this comment

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

1000

Is it 1024?

Copy link
Contributor

@qiluo-msft qiluo-msft Mar 7, 2025

Choose a reason for hiding this comment

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

len() < 2

Could you add some function level code comments to help reader? Here adding some sample "lines" will help.

Copy link
Contributor

Choose a reason for hiding this comment

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

redis::cmd

Can we use rust version swss-common?

@qiluo-msft qiluo-msft requested review from r12f and erer1243 March 7, 2025 01:50
@mssonicbld
Copy link

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -0,0 +1,18 @@
[package]
name = "procdockerstatsd_rs"
Copy link

Choose a reason for hiding this comment

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

procdockerstatsd-rs is more rusty : D

authors = ["Feng Pan"]

[dependencies]
redis = "0.23"
Copy link

Choose a reason for hiding this comment

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

we have wrapped the swss-common rust APIs here: https://github.com/sonic-net/sonic-dash-ha/tree/master/crates/swss-common, so I wonder if we can use it instead of using redis package directly.

@@ -0,0 +1,18 @@
[package]
Copy link

Choose a reason for hiding this comment

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

should we update the folder to procdockerstatsd to be more explicit?

if we like a single crate to have multiple bin, then updating the main.rs into procdockerstatsd.rs will be better for future.

}

fn convert_to_bytes(value: &str) -> u64 {
let re = Regex::new(r"(\d+\.?\d*)([a-zA-Z]+)").unwrap();
Copy link

Choose a reason for hiding this comment

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

This recompiles the regex on every call. This is bad practice, though admittedly if it's just once every 2 minutes it won't matter. This code will cache it for you.

fn convert_to_bytes(value: &str) -> u64 {
    static RE: Regex = LazyLock::new(|| Regex::new(r"(\d+\.?\d*)([a-zA-Z]+)").unwrap());
    if let Some(caps) = RE.captures(value) {
    ...

Comment on lines +52 to +63
let mut stats = HashMap::new();
stats.insert("CONTAINER ID".to_string(), values[0].to_string());
stats.insert("NAME".to_string(), values[1].to_string());
stats.insert("CPU%".to_string(), values[2].trim_end_matches('%').to_string());
stats.insert("MEM_BYTES".to_string(), convert_to_bytes(values[3]).to_string());
stats.insert("MEM_LIMIT_BYTES".to_string(), convert_to_bytes(values[5]).to_string());
stats.insert("MEM%".to_string(), values[6].trim_end_matches('%').to_string());
stats.insert("NET_IN_BYTES".to_string(), convert_to_bytes(values[7]).to_string());
stats.insert("NET_OUT_BYTES".to_string(), convert_to_bytes(values[9]).to_string());
stats.insert("BLOCK_IN_BYTES".to_string(), convert_to_bytes(values[10]).to_string());
stats.insert("BLOCK_OUT_BYTES".to_string(), convert_to_bytes(values[12]).to_string());
stats.insert("PIDS".to_string(), values[13].to_string());
Copy link

Choose a reason for hiding this comment

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

this might be cleaner if the key was &'static str, no calls to .to_string() for keys. I also like to construct maps like HashMap::from_iter([("CONTAINER_ID", values[0].to_string()), ("NAME", values[1].to_string()), ...]); but both suggestions are just a matter of taste.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants