Expand smaps / smaps_rollup parsing #1130
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 5, 2024
blt
force-pushed
the
blt/move_smaps_and_smaps_rollup_parsing_into_modules
branch
from
364810d to
20dd840
Compare
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
from
88bf086 to
5aa9221
Compare
GeorgeHahn
approved these changes
Dec 5, 2024
Comment on lines
248
to
261
| shared_clean: shared_clean.unwrap_or(0), | ||
| shared_dirty: shared_dirty.unwrap_or(0), | ||
| private_clean: private_clean.unwrap_or(0), | ||
| private_dirty: private_dirty.unwrap_or(0), | ||
| referenced: referenced.unwrap_or(0), | ||
| anonymous: anonymous.unwrap_or(0), | ||
| lazy_free: lazy_free.unwrap_or(0), | ||
| anon_huge_pages: anon_huge_pages.unwrap_or(0), | ||
| shmem_pmd_mapped: shmem_pmd_mapped.unwrap_or(0), | ||
| file_pmd_mapped: file_pmd_mapped.unwrap_or(0), | ||
| shared_hugetlb: shared_hugetlb.unwrap_or(0), | ||
| private_hugetlb: private_hugetlb.unwrap_or(0), | ||
| swap_pss: swap_pss.unwrap_or(0), | ||
| locked: locked.unwrap_or(0), |
There was a problem hiding this comment.
Some preference towards doing the same thing with size, pss, rss, and swap to make handling consistent unless there's a reason to have different behavior for those.
There was a problem hiding this comment.
Agreed, I'd like to see the required fields unwrapped similarly to how size, pss, rss etc are unwrapped above.
Only a few of these fields are actually optional, and I don't want to report 0 incorrectly.
There was a problem hiding this comment.
Hmm the more I look at this the more I think the current approach is bankrupt. Best to read the whole file in, rip through it and create metrics without the intermediary structure.
There was a problem hiding this comment.
new approach seems good in terms of simpler code, but I'm concerned that we're blindly reporting 0 for any values that don't exist.
Unless I'm misunderstanding the code here, but it seems like we would end up reporting 0 in cases where there is no value, which makes this data confusing to work with.
scottopell
reviewed
Dec 5, 2024
lading/src/observer/linux/procfs.rs
Outdated
| @@ -329,8 +355,26 @@ impl Sampler { | |||
| gauge!("smaps.pss.sum", &labels).set(measures.pss as f64); | |||
| gauge!("smaps.size.sum", &labels).set(measures.size as f64); | |||
| gauge!("smaps.swap.sum", &labels).set(measures.swap as f64); | |||
|
|
|||
| // This code reads smaps_rollup | |||
| gauge!("smaps.shared_clean.sum", &labels).set(measures.shared_clean as f64); | |||
There was a problem hiding this comment.
I'd be open to removing the smaps.sum fields, I think this is just duplicating what we already get from smaps_rollup
scottopell
requested changes
Dec 5, 2024
Comment on lines
248
to
261
| shared_clean: shared_clean.unwrap_or(0), | ||
| shared_dirty: shared_dirty.unwrap_or(0), | ||
| private_clean: private_clean.unwrap_or(0), | ||
| private_dirty: private_dirty.unwrap_or(0), | ||
| referenced: referenced.unwrap_or(0), | ||
| anonymous: anonymous.unwrap_or(0), | ||
| lazy_free: lazy_free.unwrap_or(0), | ||
| anon_huge_pages: anon_huge_pages.unwrap_or(0), | ||
| shmem_pmd_mapped: shmem_pmd_mapped.unwrap_or(0), | ||
| file_pmd_mapped: file_pmd_mapped.unwrap_or(0), | ||
| shared_hugetlb: shared_hugetlb.unwrap_or(0), | ||
| private_hugetlb: private_hugetlb.unwrap_or(0), | ||
| swap_pss: swap_pss.unwrap_or(0), | ||
| locked: locked.unwrap_or(0), |
There was a problem hiding this comment.
Agreed, I'd like to see the required fields unwrapped similarly to how size, pss, rss etc are unwrapped above.
Only a few of these fields are actually optional, and I don't want to report 0 incorrectly.
blt
force-pushed
the
blt/move_smaps_and_smaps_rollup_parsing_into_modules
branch
from
20dd840 to
996eaf8
Compare
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
2 times, most recently
from
1bd074a to
5a1f0ac
Compare
This was referenced Dec 6, 2024
Merged
blt
force-pushed
the
blt/move_smaps_and_smaps_rollup_parsing_into_modules
branch
from
3596e48 to
2ffde30
Compare
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
from
b3f9606 to
e8d0095
Compare
blt
force-pushed
the
blt/move_smaps_and_smaps_rollup_parsing_into_modules
branch
from
2ffde30 to
050e8ca
Compare
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
from
e8d0095 to
237e686
Compare
blt
force-pushed
the
blt/move_smaps_and_smaps_rollup_parsing_into_modules
branch
from
050e8ca to
2c3fd89
Compare
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
from
237e686 to
2a09dca
Compare
blt
force-pushed
the
blt/move_smaps_and_smaps_rollup_parsing_into_modules
branch
2 times, most recently
from
52db4dc to
bdb4dcc
Compare
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
from
2a09dca to
e4a843b
Compare
blt
force-pushed
the
blt/move_smaps_and_smaps_rollup_parsing_into_modules
branch
from
bdb4dcc to
08943ba
Compare
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
from
e4a843b to
2cbeb08
Compare
blt
changed the base branch from
blt/move_smaps_and_smaps_rollup_parsing_into_modules
to
graphite-base/1130
This commit expands the number of fields we parse from the smaps/smaps_rollup files. I have not hooked these up yet to metrics emission but will in a follow-up commit to this PR. Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
Signed-off-by: Brian L. Troutwine <[email protected]>
blt
force-pushed
the
graphite-base/1130
branch
from
08943ba to
47ff608
Compare
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
from
0cb9e7e to
7877403
Compare
Signed-off-by: Brian L. Troutwine <[email protected]>
blt
force-pushed
the
blt/expand_smaps___smaps_rollup_parsing
branch
from
7877403 to
89c9fb1
Compare
scottopell
approved these changes
Dec 6, 2024
Merge activity
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This commit expands the number of fields we parse from the smaps/smaps_rollup
files. I have not hooked these up yet to metrics emission but will in a follow-up
commit to this PR.