Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add chip to the DRM parser class #672

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 44 additions & 2 deletions sysfs/class_drm_amdgpu.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"path/filepath"
"regexp"
"strings"
"syscall"

"github.com/prometheus/procfs/internal/util"
Expand All @@ -31,6 +32,10 @@ const (
deviceDriverAMDGPU = "amdgpu"
)

var (
hwmonInvalidMetricChars = regexp.MustCompile("[^a-z0-9:_]")
)

// ClassDRMCardAMDGPUStats contains info from files in
// /sys/class/drm/card<card>/device for a single amdgpu card.
// Not all cards expose all metrics.
Expand All @@ -47,6 +52,38 @@ type ClassDRMCardAMDGPUStats struct {
MemoryVRAMVendor string // The VRAM vendor name.
PowerDPMForcePerformanceLevel string // The current power performance level.
UniqueID string // The unique ID of the GPU that will persist from machine to machine.
HWmonChip string // The hwmon chip.
}

func cleanChipName(name string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to sanitize the name?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

lower := strings.ToLower(name)
replaced := hwmonInvalidMetricChars.ReplaceAllLiteralString(lower, "_")
cleaned := strings.Trim(replaced, "_")
return cleaned
}

func readHWmonChip(dir string) (string, error) {
// generate a name for a sensor path
// construct a name based on device name, always unique
// can be used to relate to hwmon sensor metrics
devicePath, devErr := filepath.EvalSymlinks(filepath.Join(dir, "device"))
if devErr == nil {
devPathPrefix, devName := filepath.Split(devicePath)
_, devType := filepath.Split(strings.TrimRight(devPathPrefix, "/"))

cleanDevName := cleanChipName(devName)
cleanDevType := cleanChipName(devType)

if cleanDevType != "" && cleanDevName != "" {
return cleanDevType + "_" + cleanDevName, nil
}

if cleanDevName != "" {
return cleanDevName, nil
}
}

return "", devErr
}

// ClassDRMCardAMDGPUStats returns DRM card metrics for all amdgpu cards.
Expand All @@ -65,8 +102,10 @@ func (fs FS) ClassDRMCardAMDGPUStats() ([]ClassDRMCardAMDGPUStats, error) {
}
return nil, err
}
cardStats.Name = filepath.Base(card)
stats = append(stats, cardStats)
if cardStats != (ClassDRMCardAMDGPUStats{}) {
cardStats.Name = filepath.Base(card)
stats = append(stats, cardStats)
}
}
return stats, nil
}
Expand Down Expand Up @@ -117,6 +156,9 @@ func parseClassDRMAMDGPUCard(card string) (ClassDRMCardAMDGPUStats, error) {
if v, err := readDRMCardField(card, "unique_id"); err == nil {
stats.UniqueID = v
}
if v, err := readHWmonChip(card); err == nil {
stats.HWmonChip = v
}

return stats, nil
}
1 change: 1 addition & 0 deletions sysfs/class_drm_amdgpu_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func TestClassDRMCardAMDGPUStats(t *testing.T) {
MemoryVRAMVendor: "samsung",
PowerDPMForcePerformanceLevel: "manual",
UniqueID: "0123456789abcdef",
HWmonChip: "lnxsybus:00_pnp0a10:00",
},
}

Expand Down
Loading