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

Conversation

Deezzir
Copy link

@Deezzir Deezzir commented Oct 2, 2024

Adds a chip label to the DRM parser class so the data can be related to hwmon metrics when exported by the node_exporter.

For example, node_exporter produces the following metric:

node_hwmon_chip_names{agent_hostname="ubuntu-0", chip="0000:9d:00_0_0000:9e:00_0", chip_name="amdgpu", instance="ubuntu-0.maas"}

So, by having a chip label coming from the DRM parser, DRM metrics can be related 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

Signed-off-by: Deezzir <deezzir@gmail.com>
Signed-off-by: Deezzir <deezzir@gmail.com>
Signed-off-by: Deezzir <deezzir@gmail.com>
Signed-off-by: Deezzir <deezzir@gmail.com>
Signed-off-by: Deezzir <deezzir@gmail.com>
@Deezzir Deezzir marked this pull request as ready for review October 3, 2024 15:15
@Deezzir
Copy link
Author

Deezzir commented Oct 3, 2024

@discordianfish and @SuperQ please review! Thank you

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.

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.

DRM_amdgpu class returns entries even if AMD GPUs are not present
2 participants