-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Feat: consider adopting procfs lib FS.Meminfo() for memory collector #2957
Comments
I don't think we should marshal them to a map though, we should finally make the meminfo metrics follow more the best pratices. E.g using labels for metrics that can be summed up. For that I'd suggest creating a new meminfo collector and deprecate the old one, then in a next major release enabled the new one by default and disable the deprecated one. @SuperQ wdyt? |
Thanks @rexagod! I was mostly waiting on the green light to proceed, I'm still willing to take this on. However, I would be very happy/grateful if you would be willing to help review the PR once it's pushed and/or PR against my branch if you want to collaborate more. Initial thoughts/questions for feedback:
(sorry for the stream of consciousness, like I said, initial thoughts 🙃 ) |
If it's a new collector, it can be disabled/enabled - so no 'feature flag' specifically
If we can support the other OSes with the new collector, cool - if not, we can add support for that later.
Yes, I'd say we make the collectors mutually exclusive so you can use the same metric names where it makes sense
The general best practices apply, so yeah we shouldn't mix counters and gauges. Only things where sum() makes sense should be labels in the same metric.
Thats why I suggest a new collector (and mark the old one deprecated eventually), downstream projects can still use the old one but get warnings that it is deprecated |
As of #2952, the node exporter has been bumped to use procfs lib v0.13.0, which has a fix for safer meminfo parsing from
/proc/meminfo
. This means it's possible to move away from the custom meminfo parsing the node exporter currently does and use the updated library's parsing instead.Considerations:
The node exporter memory collector's
Update()
func uses and expects memory info to be returned as amap[string]float64
from the various platform implementations, which means that even if we adopt the library's updated memory info parsing, we would then need to convert the struct into the expected map type. This can be done with a quick json Marshal/Unmarshal dance playground, if we're willing to pullencoding/json
in as a dependency. I'd really rather avoid manually/explicitly parsing out the struct fields as it feels fragile and prone to breakage on procfs updates, so ideas welcome.I'm willing to implement the changes if the concepts here are accepted 👍
The text was updated successfully, but these errors were encountered: