-
Notifications
You must be signed in to change notification settings - Fork 116
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 metrics for iptables operations #323
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.
thanks, looks good to me. I am just not sure about some small things
Co-authored-by: leonnicolas <60091705+leonnicolas@users.noreply.github.com>
pkg/iptables/metrics.go
Outdated
"operation", | ||
"table", | ||
}) | ||
registerer.MustRegister(counterWithChain, counterWithoutChain) |
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.
if we do the registerer == nil
check here, we don't have to do it in the caller twice (for ipv4 and ipv6)
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.
@clive-jevons WDYT? I don't feel it's the cleanest piece of code ever to hand in a nil
value for registerer
here, but I have no strong feelings against it either...
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.
Great idea - done 😁
Co-authored-by: leonnicolas <60091705+leonnicolas@users.noreply.github.com>
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.
nice 🌮
@@ -245,7 +245,7 @@ func runRoot(_ *cobra.Command, _ []string) error { | |||
if port < 1 || port > 1<<16-1 { | |||
return fmt.Errorf("invalid port: port mus be in range [%d:%d], but got %d", 1, 1<<16-1, port) | |||
} | |||
m, err := mesh.New(b, enc, gr, hostname, port, s, local, cni, cniPath, iface, cleanUpIface, createIface, mtu, resyncPeriod, prioritisePrivateAddr, iptablesForwardRule, log.With(logger, "component", "kilo")) | |||
m, err := mesh.New(b, enc, gr, hostname, port, s, local, cni, cniPath, iface, cleanUpIface, createIface, mtu, resyncPeriod, prioritisePrivateAddr, iptablesForwardRule, log.With(logger, "component", "kilo"), registry) |
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.
Since the mesh accepts a registerer in its constructor, I think it's pretty awkward to still require calling "RegisterMetrics" a couple of lines below. We should do this automatically now during creation of the mesh.
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.
Love it. Just one comment about the registration of the Mesh metrics. I'd like to do this cleanup together, but we can also do a follow up.
I'll add it over the weekend! 👍 |
Thanks @dajudge and great work here |
I'm a bit puzzled why those e2e tests would fail... 🤔 |
Retried the e2e tests and it passed; I think the failure was just a flake |
Netlify also passed but the status isn't syncing. Merging 🚀🚀 |
To have better visibility of the iptables operations performed by kilo it would be nice to have prom metrics for them.
This PR introduces such metrics for all iptables operations by wrapping a proxy around the iptables client.