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 NetStat to parse /proc/net/stat/... #316
Conversation
4e4d424
to
5308ad7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I added a few in-line comments. I think the struct could be changed to better match the structure of the data. For example, each header name could be the key of a struct which points to a slice of the cpu values.
type NetStat struct {
Filename string
Stats map[string]uint64[]
}
Also, since this is a new feature, can you add a test for this?
Thank you for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just not 100% sure about the name. Maybe we should just call it NetStats?
I thought about the name, but there's It could be named |
We still need to port over the Maybe slightly confusing, but how about we follow this pattern:
|
I think this looks good, because it looks like a naming convention really: name consists of path. I'll rename |
If you're talking about the node_exporter collector name, Yes, we could call it |
When digging through some other issues/PRs, I discovered we already have some partial parsing of How do we want to handle this? We have prometheus/node_exporter#1155 open to add handling of @pgier Should we use this generic parser instead of the conntrack parser? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's merge this. We can make net_conntrackstat.go use the parser here.
@discordianfish , btw, I also have a wip pr with more generic metrics exposer which will expose conntrack stats if they appear at /proc/net/stat: prometheus/node_exporter#1771 |
What should we do with this now that prometheus/node_exporter#1155 has been merged? |
I suggest, prometheus/node_exporter#1155 might be reverted in favor of this one 😂 IMHO, it's better to have generic parser for any metric and any file, instead of hard-code here. Kernel API shouldn't be changed and files format should always be the same. Any new metric will be exposed with no human interaction with generic parser. |
@AlexZzz I agree, lets rebase this and merge it, then create an issue to track using this to replace the conntrack stats in node-exporter. |
062efc4
to
448ddf5
Compare
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
@discordianfish , rebased. |
cc/ @SuperQ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add NetStat to parse /proc/net/stat/... Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
This PR is a step to expose arp/ndisc/route cache stats from
/proc/net/stat/
. It was described in the issue prometheus/node_exporter#1626.I decided to make it similar to lnstat tool, so it will parse all new files in
/proc/net/stat
in future.