-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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(inputs.conntrack): Parse conntrack stats #8958
Conversation
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.
🤝 ✅ CLA has been signed. Thank you!
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.
Thank you for submitting a pull request! I've added some comments for you to review, also looks like you will need to re-base against the latest master for the CI to pass.
@sspaink Thanks for comments! The Suggest config directive allows adding future features, which is nice.
It might be better to call these "features"
Also, I was wandering of renaming the input
Currently the stats are collected using ConntrackStats method. Also conntrackStatsFromFile or |
Thanks for the quick and thorough response! It seems redundant to prepend
I'm also not sure if renaming the input plugin to |
@sspaink Just trying to make the config more readable to the end user. Currently the metrics naming seems to be unique, there shouldn't be any name clashes. The I guess the number of users of this plugin is very limited. As the readme suggests there's just
Eventually the On the other hand |
@deric sorry for not responding for a while, are you still interested to work on this pull request? Looking over it again the way it is seems fine to me, were there any other changes you wanted to make? |
@sspaink Sorry for late response. Hopefully all issues are now addressed. |
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.
Thank you for your continued work. There were changes how the sample configuration is managed, I added some requested changes adding the logic to embed it into the plugin back in. Also a concern about handling errors.
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.
@srebhan Thanks for comments, it should be fixed. |
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Looks good to me! Thanks for your work @deric!
Current
conntrack
module is quite limited, it supports only single metricip_conntrack_count
(the other stats usually don't change over time).This PR adds support for collecting
conntrack
stats while relying on github.com/shirou/gopsutil module. By default no stats will be collected, must be explicitly enabled:The idea is to support netfilter stats that are available on Linux systems via
conntrack -S / --stats
command:Relates issues:
TODO: Currently is not possible to collect only stats and no other metric because of
len(fields) == 0
check.Required for all PRs: