Add type and name to the DRM parser class by Deezzir · Pull Request #672 · prometheus/procfs

Deezzir · 2024-10-02T21:48:00Z

Adds a devName and devType fields to ClassDRMCardAMDGPUStats that can be later sanitized to be a chip label in node_exporter.

So, those fields from the DRM parser can be sanitized and used to relate to hwmon sensor metrics to build a better dashboard with clear AMD GPU filters.

For example:

node_drm_card_info{agent_hostname="ubuntu-0", card="card0", instance="ubuntu-0.maas", chip="0000:9d:00_0_0000:9e:00_0", memory_vendor="samsung", power_performance_level="auto", vendor="amd"}

Also closes: #673

Deezzir · 2024-10-03T15:17:48Z

@discordianfish and @SuperQ please review! Thank you

discordianfish · 2024-10-19T11:46:41Z

sysfs/class_drm_amdgpu.go

+	HWmonChip                     string // The hwmon chip.
+}
+
+func cleanChipName(name string) string {


Why do we need to sanitize the name?

It's something that the hwmon does with the chip names as well, so the format is similar.

See: https://github.com/prometheus/node_exporter/blob/e6a9cfbdcdaa21bf9676c6cd37bef8160227f423/collector/hwmon_linux.go#L74

Yes but in that case its used to construct the metric name, which has more limitations. Here its being used as label value, where I don't think we need this

Do you think it will still be the same and relatable by the chip from the hwmon? It seems like a good idea to align the result to what the hwmon does with its chip names

I guess in the node-exporter we should also just use the raw chip name and put it in a label instead of the metric name. But I think either way, here, this being used as label value, it should not get santized. But want tohave @SuperQ opinion on this as well.

A clarification - I don't care if it is sanitized or not; the only reason I am doing a sanitization here is to align the label chip coming from the hwmon metrics (which is also sanitized as per the permalink I provided above) to a new label I've added to the node_drm_card_info metric.

I want to match, aka join a metric like node_hwmon_fan_rpm with node_drm_card_info and have a connection between the two collectors. RN, there is no way to match the metrics coming from those collectors, although both provide metrics for GPUs.

Sanitizing the label before the match makes sense because this is what's being done for the chip label in the hwmon collector.

If you think we shouldn't sanitize the hwmon chip label in the node_exporter, I might provide a PR for that. As of now, I am aligning the behaviour with the existing solution.

Yeah makes sense, let's see what @SuperQ is thinking about this. I'm fine either way but moving chip to a label in the hwmon collector makes most sense to me.

@SuperQ Please take a look

I think we should move the chip name to a label in the hwmon collector.
Either way, we shouldn't sanitize the chip name here but in the node-exporter (if we decide to go that path)

Deezzir · 2025-01-13T09:34:30Z

@discordianfish PTAL

discordianfish · 2025-02-02T10:22:11Z

@Deezzir See my comment, sorry for the delay..

Deezzir · 2025-06-12T20:38:58Z

@discordianfish If I understand your comments correctly, I should leave the logic of extracting the devName and devType and export those without sanitizing and creating a chipName. The logic for that should go into the node_exporter?

Deezzir · 2025-08-19T16:13:00Z

@discordianfish can you please take a look again at the latest comment, so we can finalize the design?

discordianfish · 2025-10-15T11:30:48Z

@discordianfish If I understand your comments correctly, I should leave the logic of extracting the devName and devType and export those without sanitizing and creating a chipName. The logic for that should go into the node_exporter?

Yes, exactly. And sorry again for the delay. Unfortunately I don't have any client currently that would pay me for maintaining the node-exporter, so my attention is limited :-/

Deezzir · 2025-11-03T23:49:31Z

@discordianfish I've updated the PR to have unsatitized fields, PTAL

discordianfish · 2025-11-12T10:06:23Z

sysfs/class_drm_amdgpu.go

 		}
-		cardStats.Name = filepath.Base(card)
-		stats = append(stats, cardStats)
+		if cardStats != (ClassDRMCardAMDGPUStats{}) {


Please see #673

Still don't get it. The function always returns ClassDRMCardAMDGPUStats{}

The line checks if the struct is empty (zero-initialized) and only appends to the stats list if it is not empty. I've also modified the test file for the module. Previously, the test would produce two struct instances: one valid and one with all elements zero-initialized. It doesn't make much sense to let the empty object be passed as a valid one.

discordianfish · 2025-11-12T10:06:44Z

sysfs/class_drm_amdgpu.go

 	if v, err := readDRMCardField(card, "unique_id"); err == nil {
 		stats.UniqueID = v
 	}
+	if n, t, err := readDevInfo(card); err == nil {


Should we silently fail here?

You mean something like this in case we couldn't retrieve the name & type?

if err != nil { return ClassDRMCardAMDGPUStats{}, err }

Addressed in 3242a36

Signed-off-by: Deezzir <yurii.kondrakov@canonical.com>

Deezzir · 2026-01-22T22:31:15Z

@discordianfish PTAL

Deezzir force-pushed the drm-hwmon-name branch from 2b6737d to 3705157 Compare October 2, 2024 22:01

Deezzir marked this pull request as ready for review October 3, 2024 15:15

Deezzir mentioned this pull request Oct 3, 2024

Extend DRM labels prometheus/node_exporter#3145

Draft

discordianfish reviewed Oct 19, 2024

View reviewed changes

Deezzir requested a review from discordianfish October 31, 2024 09:10

Deezzir force-pushed the drm-hwmon-name branch 3 times, most recently from c7a7352 to d49dc61 Compare November 3, 2025 23:46

discordianfish reviewed Nov 12, 2025

View reviewed changes

Deezzir requested a review from discordianfish November 13, 2025 23:43

Deezzir added 3 commits November 17, 2025 13:37

feat: add devName and devType to DRM GPU stats

4725d85

Signed-off-by: Deezzir <yurii.kondrakov@canonical.com>

fix: don't export empty drm card stats

301c97a

Signed-off-by: Deezzir <yurii.kondrakov@canonical.com>

chore: fail the parsing if name & type invalid

3242a36

Signed-off-by: Deezzir <yurii.kondrakov@canonical.com>

Deezzir force-pushed the drm-hwmon-name branch from fe1fcfa to 3242a36 Compare November 17, 2025 18:37

Deezzir changed the title ~~Add chip to the DRM parser class~~ Add type and name to the DRM parser class Nov 17, 2025

Conversation

Deezzir commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Deezzir commented Oct 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deezzir commented Jan 13, 2025

Uh oh!

discordianfish commented Feb 2, 2025

Uh oh!

Deezzir commented Jun 12, 2025

Uh oh!

Deezzir commented Aug 19, 2025

Uh oh!

discordianfish commented Oct 15, 2025

Uh oh!

Deezzir commented Nov 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deezzir commented Jan 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Deezzir commented Oct 2, 2024 •

edited

Loading