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

Restore optional temperatures properties on Linux #1546

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

srebhan
Copy link

@srebhan srebhan commented Oct 31, 2023

fixes #1319

This PR restores the properties reported before #905 such as minimum and alarm temperatures reported by some devices on Linux. To do so we introduce a Optional struct-member containing all values found with the device prefix. This way, the PR is backward compatible, i.e. existing code will continue to work without changes, but provides those properties including a way to check if High and Critical properties actually exists or not.

@srebhan srebhan changed the title Restore min and alarm temperatures Restore optional temperatures properties on Linux Nov 1, 2023
@srebhan
Copy link
Author

srebhan commented Nov 1, 2023

@shirou any comment on the PR?

@shirou
Copy link
Owner

shirou commented Nov 2, 2023

According to linux kernel document, there are a lot of parameters. So I agree to use map[string]float64 without defining all these properties.

However, temperature exists in psutil, but https://psutil.readthedocs.io/en/latest/#sensors did not seem to support anything other than the existing current, high, and critical.

Also, other than Linux, only FreeBSD is supported on psutil. And on FreeBSD, it seemed that only current could be obtained with sysctl.

I think that gopsutil should align to psutil as much as possible, and I also want to remove parameters that can be obtained only on Linux as much as possible. Therefore, I am of the opinion that if anyone wants temperatures, I think that person should create a separate library for Linux.

If we want to add a parameter that does not exist in psutil, I believe we have to be able to take values in at least two distributions, which means Windwos/mac/Linux/*BSD.

@srebhan
Copy link
Author

srebhan commented Nov 3, 2023

@shirou how about defining (named) structure members the same way psutil does but provide the said map for all arch-specific properties? We are currently using this library in telegraf and most users usually want to monitor everything the arch supports...

@srebhan
Copy link
Author

srebhan commented Nov 22, 2023

@shirou any chance you accept this PR? Otherwise I will close it.

@shirou
Copy link
Owner

shirou commented Nov 23, 2023

Unfortunately, in the current version v3, this PR is not acceptable. However, we are considering the possibility of handling platform specific information in v4 in #1547. Maybe that version will be able to handle this kind of information.

@srebhan
Copy link
Author

srebhan commented Nov 24, 2023

Can you explain why this is not possible in v3? This is a non-breaking change, so all existing users are uneffected...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Temperature changes miss data on linux
2 participants