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

Jump to bottom

Introduce parsing for /proc/<pid>/net/snmp, /proc/<pid>/net/snmp6 and /proc/<pid>/net/netstat. #435

Merged
merged 3 commits into from Apr 1, 2022
Merged

Introduce parsing for /proc/<pid>/net/snmp, /proc/<pid>/net/snmp6 and /proc/<pid>/net/netstat. #435

merged 3 commits into from Apr 1, 2022

Conversation

nikakis
Copy link
Contributor

@nikakis nikakis commented Feb 14, 2022

No description provided.

@nikakis nikakis mentioned this pull request Feb 14, 2022
Closed
@SuperQ SuperQ self-requested a review February 14, 2022 14:40
@SuperQ
Copy link
Member

SuperQ commented Feb 14, 2022

Nice, do we have to worry about missing fields in various kernel versions? We like to be able to support very old kernels in this library, since Go will run on kernels as old as 2.6.23.

@nikakis
Copy link
Contributor Author

nikakis commented Feb 17, 2022

@SuperQ I believe according to this RFC it seems that we will be fine.

@SuperQ
Copy link
Member

SuperQ commented Feb 17, 2022 •
edited

Any chance I can also get you to implement snmp6 parsing in this PR too?

@nikakis
Copy link
Contributor Author

nikakis commented Feb 17, 2022

Sure i will implement snmp6 and netstat also.
Maybe i can also do the refactoring on the node_exporter side when this is done.

@nikakis nikakis changed the title Introduce parsing for /proc/<pid>/net/snmp. Introduce parsing for /proc/<pid>/net/snmp, /proc/<pid>/net/snmp6 and /proc/<pid>/net/netstat. Feb 18, 2022
@nikakis
Copy link
Contributor Author

nikakis commented Feb 18, 2022

@SuperQ I added also snmp6 and netstat. Have a look please!

SuperQ
SuperQ requested changes Feb 20, 2022
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
proc_netstat.go Outdated Show resolved Hide resolved
proc_snmp.go Outdated Show resolved Hide resolved
@nikakis
Introduce parsing for /proc/<pid>/net/snmp, /proc/<pid>/net/snmp6…
3d753b0 
… and `/proc/<pid>/net/netstat`.

Signed-off-by: Nikos Kakavas <nikakis@protonmail.com>
discordianfish
discordianfish reviewed Mar 7, 2022
View reviewed changes
proc_netstat.go Outdated Show resolved Hide resolved
discordianfish
discordianfish reviewed Mar 7, 2022
View reviewed changes
proc_snmp.go Outdated Show resolved Hide resolved
@nikakis
remove mapstructure
5c12586 
Signed-off-by: Nikos Kakavas <nikakis@protonmail.com>
@nikakis nikakis force-pushed the introduce-support-for-snmp branch from dc5e963 to 5c12586 Compare March 9, 2022 15:23
@nikakis
Copy link
Contributor Author

nikakis commented Mar 9, 2022

@discordianfish @SuperQ Have a look again please!

discordianfish
discordianfish approved these changes Mar 9, 2022
View reviewed changes
Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

@nikakis nikakis requested a review from SuperQ March 10, 2022 08:31
@nikakis
Copy link
Contributor Author

nikakis commented Mar 15, 2022

Hello! @discordianfish @SuperQ
Sorry for spamming but any update with merging this?
Thanks!

@discordianfish
Copy link
Member

Still needs a LGTM from @SuperQ or @pgier

pgier
pgier approved these changes Mar 22, 2022
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, just had a couple of optional in-line suggestions.

proc_netstat.go Outdated Show resolved Hide resolved
proc_netstat.go Show resolved Hide resolved
proc_snmp.go Outdated Show resolved Hide resolved
@nikakis
Introduce strings.TrimSuffix instead of custom split
6ff7304 
Signed-off-by: Nikos Kakavas <nikakis@protonmail.com>
@pgier pgier merged commit d917e64 into prometheus:master Apr 1, 2022
@nikakis nikakis mentioned this pull request Apr 3, 2022
Open
@nikakis nikakis deleted the introduce-support-for-snmp branch April 3, 2022 16:31
discordianfish
discordianfish reviewed Apr 14, 2022
View reviewed changes
proc_snmp6.go
// and returns a map contains those metrics.
func parseSNMP6Stats(r io.Reader) (ProcSnmp6, error) {
var (
scanner = bufio.NewScanner(r)
Copy link
Member

Choose a reason for hiding this comment

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

How did this gofmt issue not surfaced in the PR? @SuperQ @pgier any idea?

Copy link
Contributor Author

@nikakis nikakis Apr 14, 2022 •
edited

Choose a reason for hiding this comment

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

I will fix this fmt issue and also try to investigate why this didn't worked properly

Copy link
Member

Choose a reason for hiding this comment

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

Weird. Maybe out of date source branch?

remijouannet pushed a commit to remijouannet/procfs that referenced this pull request Oct 20, 2022
@pgier
Merge pull request prometheus#435 from nikakis/introduce-support-for-…
1ab1d7d 
…snmp

Introduce parsing for `/proc/<pid>/net/snmp`, `/proc/<pid>/net/snmp6` and `/proc/<pid>/net/netstat`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgier pgier pgier approved these changes

@discordianfish discordianfish discordianfish approved these changes

@SuperQ SuperQ Awaiting requested review from SuperQ

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@nikakis @SuperQ @discordianfish @pgier