Read all cgroup v2 metrics that can be read #1120

blt · 2024-12-03T03:45:42Z

What does this PR do?

This commit removes any arbitration of the cgroup v2 heirarchy for a
given process. We instead read anything that can be read, looping over
all cgroup files present but not following the hierarchy down.

Metric names will change but they will now reflect underlying system reality.

blt · 2024-12-03T03:46:00Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

lading/src/observer/linux.rs

scottopell · 2024-12-03T20:03:12Z

lading/src/observer/linux.rs

-                    mem_stat.stat.total_inactive_file
-                };
-                let usage = mem_stat.usage_in_bytes;
-                let working_set = if usage < inactive_file {


Can we preserve working_set calculation as well?

Oh hmm. So in the new cgroup sampler we never actually inspect any of the data being read, just immediately rip it out to metrics. I think we could calculate this easily enough in-platform or we could make a special read to preserve this.

Happy to move forward with either, but you're right. working_set won't be present in the new telemetry stream.

I'd be interested in preserving it, calculating it in-platform is possible, but has zero-discoverability.

Since our intention is to mimic k8s's working_set, lets name it k8s-like.working_set or something like that.

Ah yeah, I'll add that in a separate PR up-stack. Figure I'll make a k8s reader where we can put any synthetic measures like this.

lading/src/observer/linux/cgroup/v2.rs

scottopell · 2024-12-04T19:49:37Z

lading/src/observer/linux/cgroup/v2.rs

+                                    let metric_prefix = match file_name.to_str() {
+                                        Some(s) => format!("cgroup.v2.{s}"),
+                                        None => {
+                                            // Skip files with non-UTF-8 names


worth a warn here? I would be pretty shocked if there were any cgroup file names that were not valid utf-8

Addressed in 67078d8

scottopell · 2024-12-04T19:55:13Z

lading/src/observer/linux/cgroup/v2.rs

+                                        Ok(content) => {
+                                            let content = content.trim();
+
+                                            // Cgroup files that have values are either single-valued or


I went looking for the rules for these files and found this
https://docs.kernel.org/admin-guide/cgroup-v2.html#interface-files

It looks like multiple values aren't supported (whether new-line separated or space separated) and nested keyed aren't supported.

Worth refactoring this match to ... match the documented potential options, even if we intentionally don't support some of them.

Will update the documentation.

Addressed in 67078d8

scottopell · 2024-12-04T19:56:15Z

lading/src/observer/linux/cgroup/v2.rs

+                                    };
+                                    let file_path = entry.path();
+
+                                    match fs::read_to_string(&file_path).await {


Worth checking if the file is readable up-front, some entries in cgroupv2 are write-only. eg:

--w------- 1 root root 0 Jul 15 21:02 memory.reclaim -r--r--r-- 1 root root 0 Jul 15 21:02 memory.stat

Ah yeah that's a good thought.

Ah actually that metadata check is pretty much embedded in the read attempt, so we double up the syscalls by doing a check up front.

Addressed in 67078d8

eh, not going to hold up the PR, but we already fetch the file metadata to determine if its a file. So really you just need to check if metadata.is_file && metadata.permissions.is_readable above.

lading/src/observer/linux/procfs.rs

Signed-off-by: Brian L. Troutwine <[email protected]>

This commit removes any arbitration of the cgroup v2 heirarchy for a given process. We instead read anything that can be read, looping over all cgroup files present but not following the heirarchy down. Signed-off-by: Brian L. Troutwine <[email protected]>

Signed-off-by: Brian L. Troutwine <[email protected]>

This commit splits the Sampler so that cgroup collection is in a separate implementation from procfs, which has gotten sprawling. I have maintained the existing Sampler interface and hidden the two new implementations inside of it, although we might choose to expose them at some point in the future. Signed-off-by: Brian L. Troutwine <[email protected]>

Signed-off-by: Brian L. Troutwine <[email protected]>

scottopell

Looks good, thanks for the iterations. My main concern is around treating all cgroup-parsed data as counts, but we can iterate on this as we go.

scottopell · 2024-12-06T15:36:05Z

lading/src/observer/linux/cgroup/v2.rs

+                                    };
+                                    let file_path = entry.path();
+
+                                    match fs::read_to_string(&file_path).await {


eh, not going to hold up the PR, but we already fetch the file metadata to determine if its a file. So really you just need to check if metadata.is_file && metadata.permissions.is_readable above.

scottopell · 2024-12-06T15:37:39Z

lading/src/observer/linux/cgroup/v2.rs

+                    s => s.parse()?,
+                };
+                let metric_name = format!("{metric_prefix}.{key}");
+                gauge!(metric_name, labels).set(value);


Not a blocking concern, but are there any values that should be collected as counts rather than gauges? I think they'll reveal themselves shortly if they exist, but I could imagine some of these entries (I'm thinking memory.events has oom_kills for example) would be counts.

blt · 2024-12-06T15:39:09Z

Merge activity

Dec 6, 10:39 AM EST: A user merged this pull request with Graphite.

blt added the no-changelog label Dec 3, 2024 — with Graphite App

blt mentioned this pull request Dec 3, 2024

Update procfs to 0.17 #1119

Merged

blt marked this pull request as ready for review December 3, 2024 03:46

blt requested a review from a team as a code owner December 3, 2024 03:46

blt force-pushed the blt/update_procfs_to_0.17 branch from 63f1ce0 to 3f145ec Compare December 3, 2024 17:35

blt force-pushed the blt/read_all_cgroup_v2_metrics_that_can_be_read branch from b4b6486 to 2a32d9b Compare December 3, 2024 17:35

scottopell requested changes Dec 3, 2024

View reviewed changes

lading/src/observer/linux.rs Outdated Show resolved Hide resolved

scottopell reviewed Dec 3, 2024

View reviewed changes

Base automatically changed from blt/update_procfs_to_0.17 to main December 3, 2024 22:27

blt force-pushed the blt/read_all_cgroup_v2_metrics_that_can_be_read branch from 8adc60d to 6b05a98 Compare December 3, 2024 22:57

blt mentioned this pull request Dec 4, 2024

Migrate procfs memory module under procfs/ #1124

Merged

scottopell reviewed Dec 4, 2024

View reviewed changes

blt force-pushed the blt/read_all_cgroup_v2_metrics_that_can_be_read branch from 2d8f756 to 77b5305 Compare December 4, 2024 21:18

blt changed the base branch from main to blt/attempt_generational_storage_of_capture_metrics December 4, 2024 21:18

This was referenced Dec 4, 2024

Attempt generational storage of capture metrics #1125

Merged

cargo-deny ding on an OSI license #1126

Merged

blt force-pushed the blt/attempt_generational_storage_of_capture_metrics branch from 36ced36 to 0c1c3c1 Compare December 4, 2024 21:20

blt force-pushed the blt/read_all_cgroup_v2_metrics_that_can_be_read branch 3 times, most recently from b1048f9 to 2376bfa Compare December 5, 2024 00:17

This was referenced Dec 5, 2024

Remove target RSS_BYTE tracking #1127

Merged

Rely on metrics registry to expire metrics #1128

Merged

Move smaps and smaps_rollup parsing into modules #1129

Merged

Expand smaps / smaps_rollup parsing #1130

Merged

blt changed the base branch from blt/attempt_generational_storage_of_capture_metrics to graphite-base/1120 December 5, 2024 07:57

blt added 4 commits December 5, 2024 07:57

cargo-deny ding on an OSI license

0221410

Signed-off-by: Brian L. Troutwine <[email protected]>

thread labels through new metrics

f10ceef

Signed-off-by: Brian L. Troutwine <[email protected]>

test fix

707fa4d

Signed-off-by: Brian L. Troutwine <[email protected]>

blt added 6 commits December 5, 2024 07:57

remove duplicate code

3c32c4e

Signed-off-by: Brian L. Troutwine <[email protected]>

very explicit error handling

29d322c

Signed-off-by: Brian L. Troutwine <[email protected]>

more recoverable error work

bd1dfb7

Signed-off-by: Brian L. Troutwine <[email protected]>

bug fixes

2be4d4b

Signed-off-by: Brian L. Troutwine <[email protected]>

fiddle with log levels

1704daa

Signed-off-by: Brian L. Troutwine <[email protected]>

blt force-pushed the blt/read_all_cgroup_v2_metrics_that_can_be_read branch from 2376bfa to 8e49816 Compare December 5, 2024 07:57

blt force-pushed the graphite-base/1120 branch from a61114e to 1c73887 Compare December 5, 2024 07:57

blt changed the base branch from graphite-base/1120 to main December 5, 2024 07:58

unused info

2f79087

Signed-off-by: Brian L. Troutwine <[email protected]>

blt force-pushed the blt/read_all_cgroup_v2_metrics_that_can_be_read branch from 8e49816 to 2f79087 Compare December 5, 2024 07:58

PR review

67078d8

Signed-off-by: Brian L. Troutwine <[email protected]>

This was referenced Dec 6, 2024

Slim out joinset #1133

Merged

total_rss_bytes and total_pss_bytes from smaps_rollup #1134

Merged

blt requested a review from scottopell December 6, 2024 03:19

scottopell approved these changes Dec 6, 2024

View reviewed changes

blt merged commit d2a93c4 into main Dec 6, 2024
19 checks passed

blt deleted the blt/read_all_cgroup_v2_metrics_that_can_be_read branch December 6, 2024 15:39

scottopell mentioned this pull request Dec 9, 2024

Add support for transparent huge pages in linux memory observer #1105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read all cgroup v2 metrics that can be read #1120

Read all cgroup v2 metrics that can be read #1120

blt commented Dec 3, 2024 •

edited

Loading

blt commented Dec 3, 2024 •

edited

Loading

scottopell Dec 3, 2024

blt Dec 3, 2024

scottopell Dec 5, 2024

blt Dec 5, 2024

scottopell Dec 4, 2024

blt Dec 5, 2024

scottopell Dec 4, 2024

blt Dec 5, 2024

blt Dec 5, 2024

scottopell Dec 4, 2024

blt Dec 5, 2024

blt Dec 5, 2024

blt Dec 5, 2024

scottopell Dec 6, 2024

scottopell left a comment

scottopell Dec 6, 2024

scottopell Dec 6, 2024

blt commented Dec 6, 2024

Read all cgroup v2 metrics that can be read #1120

Read all cgroup v2 metrics that can be read #1120

Conversation

blt commented Dec 3, 2024 • edited Loading

What does this PR do?

blt commented Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottopell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blt commented Dec 6, 2024

Merge activity

blt commented Dec 3, 2024 •

edited

Loading

blt commented Dec 3, 2024 •

edited

Loading