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.

Sign up for GitHub

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

Jump to bottom

Add support for InfiniBand #164

Merged
merged 1 commit into from
Jun 20, 2019
Merged

Add support for InfiniBand #164

merged 1 commit into from
Jun 20, 2019

Conversation

bdrung
Copy link
Contributor

@bdrung bdrung commented May 23, 2019

The Prometheus node exporter collects metrics for the InfiniBand network protocol including the amount of packets sent and received, the number of times the link has been downed and how many times the link has recovered from an error state.

Reading all those information from sysfs is better placed in the procfs library. Also collect the state, physical state, and the rate.

@bdrung bdrung force-pushed the infiniband-support branch 2 times, most recently from dd36224 to 6431c84 Compare May 23, 2019 17:25
pgier
pgier requested changes May 23, 2019
View reviewed changes
Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a few nits in-line.
Also, I think usually it's preferred to have the parseThing functions take either a string or an io.Reader with the content to be parsed instead of passing a filename. @mdlayher WDYT?

sysfs/class_infiniband.go Outdated Show resolved Hide resolved
sysfs/class_infiniband.go Outdated Show resolved Hide resolved
sysfs/class_infiniband.go Outdated Show resolved Hide resolved
sysfs/class_infiniband.go Outdated Show resolved Hide resolved
sysfs/class_infiniband.go Outdated Show resolved Hide resolved
mdlayher
mdlayher reviewed May 23, 2019
View reviewed changes
Copy link
Contributor

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

Will review in depth later, but TBH I don't think the use of reflect is really justified here. It's not that bad to write a couple of switch statements that match up filenames with struct fields. And the fields should be regular values and not pointers as well.

@bdrung
Copy link
Contributor Author

bdrung commented May 23, 2019

Thanks for the review. I will address those points. @mdlayher, do you really prefer to have a switch statement for the 16 different values? This list might grow. If the counter fields are regular values, what should I do if the system does not have this counter?

@bdrung bdrung force-pushed the infiniband-support branch 2 times, most recently from 6ea13b3 to 337211e Compare May 24, 2019 10:29
@bdrung
Copy link
Contributor Author

bdrung commented May 24, 2019

I looked at https://github.com/prometheus/node_exporter/blob/master/collector/infiniband_linux.go and borrowed the workarounds and the legacy counters.

@bdrung bdrung force-pushed the infiniband-support branch from 337211e to 64dcedd Compare May 24, 2019 13:35
@bdrung
Copy link
Contributor Author

bdrung commented May 24, 2019

I changed the two parse functions to take a string as parameter (instead of a path to the file).

@bdrung bdrung mentioned this pull request May 28, 2019
Merged
@discordianfish
Copy link
Member

Tend to agree with @mdlayher. What about using a temporary map to iterate over instead of using reflect to iterate over the map?

@bdrung bdrung force-pushed the infiniband-support branch from 64dcedd to 0b0644c Compare June 11, 2019 14:59
@bdrung
Copy link
Contributor Author

bdrung commented Jun 11, 2019

sysfs/class_power_supply.go and sysfs/net_class.go uses reflect. Can you point me to a component that uses a temporary map to iterate over?

@bdrung bdrung force-pushed the infiniband-support branch 3 times, most recently from 3229901 to 642c0e0 Compare June 13, 2019 17:34
@bdrung
Copy link
Contributor Author

bdrung commented Jun 13, 2019

@mdlayher I saw your recent commits on master and adapted this pull request to the logic used for PowerSupply. Therefore reflect is not used any more. Additional changes:

  • Introduce InfiniBandDevice type
  • also collect board_id, fw_ver, and hca_type (in InfiniBandDevice)

All remarks are addressed and this pull request is ready for an in depth review.

pgier
pgier approved these changes Jun 13, 2019
View reviewed changes
Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

LGTM

mdlayher
mdlayher reviewed Jun 13, 2019
View reviewed changes
internal/util/valueparser.go Show resolved Hide resolved
sysfs/class_infiniband.go Outdated Show resolved Hide resolved
sysfs/class_infiniband.go Outdated Show resolved Hide resolved
sysfs/class_infiniband.go Outdated Show resolved Hide resolved
sysfs/class_infiniband.go Outdated Show resolved Hide resolved
sysfs/class_infiniband.go Show resolved Hide resolved
sysfs/class_infiniband.go Show resolved Hide resolved
sysfs/class_infiniband_test.go Outdated Show resolved Hide resolved
sysfs/class_infiniband_test.go Show resolved Hide resolved
@bdrung bdrung force-pushed the infiniband-support branch from 642c0e0 to 37a480d Compare June 14, 2019 09:16
@bdrung
Add support for InfiniBand
7acf46b 
The Prometheus node exporter collects metrics for the InfiniBand network
protocol including the amount of packets sent and received, the number
of times the link has been downed and how many times the link has
recovered from an error state.

Reading all those information from sysfs is better placed in the procfs
library. Also collect the state, physical state, and the rate.

Signed-off-by: Benjamin Drung <[email protected]>
@bdrung bdrung force-pushed the infiniband-support branch from 37a480d to 7acf46b Compare June 14, 2019 09:19
@pgier pgier merged commit f959769 into prometheus:master Jun 20, 2019
@pgier
Copy link
Collaborator

pgier commented Jun 20, 2019

Merged, thanks!

@bdrung bdrung deleted the infiniband-support branch June 20, 2019 16:26
@bdrung bdrung mentioned this pull request Jun 25, 2019
Merged
bdrung added a commit to bdrung/node_exporter that referenced this pull request Aug 28, 2019
@bdrung
Use InfiniBandClass from procfs library
7a2d79a 
Parsing the sysfs files for InfiniBand was added to the procfs library
(see prometheus/procfs#164).

Therefore use `InfiniBandClass` from the procfs library instead of
parsing sysfs itself.

If the port counter return `N/A (no PMA)` no metric will be returned
(instead of returning 0 for this metric.

Signed-off-by: Benjamin Drung <[email protected]>
discordianfish pushed a commit to prometheus/node_exporter that referenced this pull request Sep 23, 2019
@bdrung @discordianfish
Use InfiniBandClass from procfs library (#1396)
27b8c93 
Parsing the sysfs files for InfiniBand was added to the procfs library
(see prometheus/procfs#164).

Therefore use `InfiniBandClass` from the procfs library instead of
parsing sysfs itself.

If the port counter return `N/A (no PMA)` no metric will be returned
(instead of returning 0 for this metric.

Signed-off-by: Benjamin Drung <[email protected]>
bobrik pushed a commit to bobrik/procfs that referenced this pull request Jan 14, 2023
@eminence
Merge pull request prometheus#164 from eminence/32bit-ci
f6f227c 
Do cargo-check for 32-bit linux in github CI
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
@bdrung @oblitorum
Use InfiniBandClass from procfs library (prometheus#1396)
9b70023 
Parsing the sysfs files for InfiniBand was added to the procfs library
(see prometheus/procfs#164).

Therefore use `InfiniBandClass` from the procfs library instead of
parsing sysfs itself.

If the port counter return `N/A (no PMA)` no metric will be returned
(instead of returning 0 for this metric.

Signed-off-by: Benjamin Drung <[email protected]>
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
@bdrung @oblitorum
Use InfiniBandClass from procfs library (prometheus#1396)
c74ca4b 
Parsing the sysfs files for InfiniBand was added to the procfs library
(see prometheus/procfs#164).

Therefore use `InfiniBandClass` from the procfs library instead of
parsing sysfs itself.

If the port counter return `N/A (no PMA)` no metric will be returned
(instead of returning 0 for this metric.

Signed-off-by: Benjamin Drung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdlayher mdlayher mdlayher left review comments

@dswarbrick dswarbrick dswarbrick left review comments

@discordianfish discordianfish discordianfish left review comments

@pgier pgier pgier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bdrung @discordianfish @pgier @dswarbrick @mdlayher