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

Rely on a Netlink go implementation instead of conntrack command calls #15

Open
fdomain opened this issue Aug 9, 2022 · 6 comments
Open

Comments

@fdomain
Copy link
Contributor

fdomain commented Aug 9, 2022

This is a proposal to replace usage of conntrack tool to fetch statistics on the conntrack table, with a golang lib able to bring this information directly over the Netlink socket. The main benefit would be to get rid of the conntrack's output parsing.

There is already a lib allowing to do that: https://github.com/ti-mo/conntrack
I have used it for another project and it seems pretty easy to get stats and have the result in a proper golang struct.

I can work on this but I'd like to have your opinion before rushing into it.

@jwkohnen
Copy link
Owner

jwkohnen commented Aug 9, 2022

The idea sounds great.

What I happen to be doing right now is writing test cases (I expect to push them next weekend), because I want to refactor this software thoroughly. This exporter started as a quick write-once hack and the awful code quality showed its ugly head in the moment the first feature adding PR came in (and made it even worse).

Secondary goal of the refactor is to get rid of the prometheus client library, because it is a huge monster of a dependency and its usage is hard to understand and contributes to weird code (in small projects like this). In other closed source projects of mine of similar size I tend to write OpenMetrics handlers directly and I want to transfer code here.

Ideally this project has no dependencies at all, and I like the idea of swapping the conntrack-tools off-process dependency with the lib you mention (not reaching the ideal goal).

I don't know how to progress from here right now, yet. The end goal would be to use the conntrack lib, remove prometheus-client and build statically on a distroless or even scratch container. But I'd recon that a first step would be to start converging towards filling "a proper Go struct" as a meeting point of both refactoring streams?

I'll contemplate on that for the next days and invite you to do the same.

@fdomain
Copy link
Contributor Author

fdomain commented Aug 11, 2022

Thanks for your answer, regarding Prometheus metrics, we can do a simpler implementation (see my attempt here: criteo-forks@ace1af9 please disregard the things I have commented in the main.go, it's just to give you an overview on how I would implement this).

This is a much simpler approach, more readable IMO, and no need to redefine Collect() func etc...

If we also use the conntrack lib, the code will be even simpler and easier to understand.

@jwkohnen
Copy link
Owner

That looks like a good step forward. I've cleaned up my testing code and pushed it to a new branch dev and used that to also change what the exporter package exports and what it doesn't. That gives a good hint on what I think should be in the main package and what shouldn't. Also it hides the fact that the metrics are generated by the prometheus client library (which I was to get rid of eventually).

Hopefully my tests are robust enough to still pass when new labels (netns) are added.

Would you be interested to shape your code towards the dev branch?

@fdomain
Copy link
Contributor Author

fdomain commented Aug 11, 2022

Sure I'll work on this soon.

@fdomain
Copy link
Contributor Author

fdomain commented Aug 22, 2022

I'll push another PR for the Prometheus metrics based on criteo-forks@ace1af9 once #16 is approved and merged, I don't want to deal with a rebase full of merge conflicts :)

@jwkohnen
Copy link
Owner

I have pushed a "big refactoring." The sloc rised quite a bit, but despite that I think the code structure is a lot cleaner and easier to do adapt the library approach. I apologise if that ruins your rebase plans!

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

No branches or pull requests

2 participants