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

Separate NetStat parsing logic #472

Merged
merged 1 commit into from Dec 17, 2022
Merged

Conversation

Dentrax
Copy link
Contributor

@Dentrax Dentrax commented Oct 13, 2022

As procfs already did use parseX(io.Reader) functions for other filesystem formats, this was missing in netstat parsing. It should not do both reading and parsing stuff in the same function. Anyone who want to just access parser func using go:linkname directive, now it's possible. I had to copy-paste the actual parser logic since the function itself also reads a file.

Signed-off-by: Furkan furkan.turkal@trendyol.com

Signed-off-by: Furkan <furkan.turkal@trendyol.com>
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.

Makes sense in general but neither functions are exposed, so they can't be used by consumers of the package anyway. We might want to consider exposing them. Do you have a use case for this?
But otherwise just for consistency alone this makes sense, so LGTM

@Dentrax
Copy link
Contributor Author

Dentrax commented Oct 18, 2022

so they can't be used by consumers of the package anyway

AFAIK, procfs doesn't expose parsing functions to the public in general. So this statement also valid for all other functions.

We might want to consider exposing them.

It would be nice to expose all parsing functions to public in another PR! You're right! I intentionally make this private for consistency.

Do you have a use case for this?

I am able to use other parse functions using go:linkname directive, but couldn't use this one. So have to copy+paste the actual logic. I'm not working on Linux, but I have a netstat file that needed to parse.

Thanks! 🤗

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Yes, let's make the parsing functions public. That would be very nice.

@SuperQ SuperQ merged commit 421a743 into prometheus:master Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants