added process metrics and Go runtime metrics #198
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droot The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
|
@JoelSpeed Can you pl. take a quick look at this one ? |
|
For the record, I seem to recall some people hesitant about including process and runtime stats. I think it's probably fine for now, and if people file issues, we can always add a flag to turn it off. |
| // expose process metrics like CPU, Memory, file descriptor usage etc. | ||
| prometheus.NewProcessCollector(prometheus.ProcessCollectorOpts{}), | ||
| // expose Go runtime metrics like GC stats, memory stats etc. | ||
| metrics.Registry.MustRegister(prometheus.NewGoCollector()), |
There was a problem hiding this comment.
Looks like you don't need to wrap prometheus.NewGoCollector() in this MustRegister
| metrics.Registry.MustRegister(prometheus.NewGoCollector()), | |
| prometheus.NewGoCollector(), |
There was a problem hiding this comment.
Good catch, last minute refactor introduced it. I had two separate MustRegister calls earlier.
| @@ -48,4 +48,10 @@ func init() { | |||
| metrics.Registry.MustRegister(QueueLength) | |||
| metrics.Registry.MustRegister(ReconcileErrors) | |||
| metrics.Registry.MustRegister(ReconcileTime) | |||
| metrics.Registry.MustRegister( | |||
| // expose process metrics like CPU, Memory, file descriptor usage etc. | |||
| prometheus.NewProcessCollector(prometheus.ProcessCollectorOpts{}), | |||
There was a problem hiding this comment.
I think it might be worth adding the a namespace to the ProcessCollerctorOpts{}, if we add the namespace controller_runtime then all metrics here will be prefixed with controller_runtime_ and will fit in with the other metrics we are emitting from the library, WDYT?
| prometheus.NewProcessCollector(prometheus.ProcessCollectorOpts{}), | |
| prometheus.NewProcessCollector(prometheus.ProcessCollectorOpts{Namespace: "controller_runtime"}), |
There was a problem hiding this comment.
I thought about it and was a bit hesitant because these are not really controller-runtime specific metrics. If users are running other Go services (and k8s metrics for other components), they might already have configuration for processing these default prometheus metrics in their dashboards/pipelines. So keeping the same name might help them. WDYT ?
There was a problem hiding this comment.
Yeah good point, I've double checked with what the Kubernetes API server does and it doesn't prefix so let's keep it as is
There was a problem hiding this comment.
@droot I've added a couple of comments here
I'm not sure this would build as it is currently with the Collector being wrapped by MustRegister?
I'm wondering whether it is worth changing this to register all of the Collectors in one call to MustRegister or whether we should keep them all separate, would be good to be consistent
|
Thanks for the review @JoelSpeed I have moved all the registration in single call (fits our use case). |
Enabled Prometheus collector for process metrics and Go runtime
droot
force-pushed
the
feature/add-runtime-metrics
branch
from
cb07718
to
40e1154
Compare
k8s-ci-robot
added
size/S
|
@JoelSpeed addressed the comments. PTAL. |
droot
added
the
lgtm
…-metrics added process metrics and Go runtime metrics
Rollback base image to ubuntu
Enabled Prometheus collector for process metrics and Go runtime