Support prometheus metrics #73
Conversation
cert-manager-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @7ing. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
cert-manager-prow
bot
added
needs-ok-to-test
|
/ok-to-test |
cert-manager-prow
bot
added
ok-to-test
and removed
needs-ok-to-test
Following metrics added: certmanager_csi_certificate_request_expiration_timestamp_seconds certmanager_csi_certificate_request_ready_status certmanager_csi_certificate_request_renewal_timestamp_seconds certmanager_csi_driver_issue_call_count certmanager_csi_driver_issue_error_count certmanager_csi_managed_certificate_count certmanager_csi_managed_volume_count fixes: cert-manager#60 Signed-off-by: Jing Liu <[email protected]>
There was a problem hiding this comment.
This looks great, thanks Jing 🙌 the integration and unit tests here make it far easier to review confidently!
My main questions/concerns are around the construction logic in the metrics subpackage, which I think we need to decouple from net.Listener (and allow more flexibility for projects that already have their own prometheus.Registry they'd like to re-use).
| } | ||
|
|
||
| // NewServer registers Prometheus metrics and returns a new Prometheus metrics HTTP server. | ||
| func (m *Metrics) NewServer(ln net.Listener) *http.Server { |
There was a problem hiding this comment.
This isn't called outside of test cases, and I guess that is by design as it is expected that the corresponding implementation should call NewServer on metrics.Metrics to register their Listener.
Could you possibly update the example/ implementation in the root of this repository to demonstrate how to actually add the /metrics endpoint? I also wonder if the Managed should be extended to be able to auto-serve this endpoint in cases where a user doesn't need to provide their own listener (but does want metrics to be served).
There was a problem hiding this comment.
What if we renamed this to Register(*prometheus.Registry) rather than tying the net.Listener logic into the registration?
We can always have/find some kind of csihelpers.Handle(*http.Server, *prometheus.Registry) function elsewhere then, which needn't be opinionated about csi-lib.
| ) | ||
|
|
||
| // Create Registry and register the recommended collectors | ||
| registry := prometheus.NewRegistry() |
There was a problem hiding this comment.
Could there be cases where a user wants to provide their own Registry and have these metrics registered into it, so they can be served alongside driver-specific metrics? This would make the library more composable with existing drivers that may serve their own metrics already.
| registry := prometheus.NewRegistry() | ||
| registry.MustRegister( | ||
| collectors.NewProcessCollector(collectors.ProcessCollectorOpts{}), | ||
| collectors.NewGoCollector(), |
There was a problem hiding this comment.
I feel like these should be auto-registered, but I think the Metrics struct itself should be able to be setup so that it doesn't embed these Go metrics into them (i.e. so it only adds in the csi-lib specific metrics to the registry).
Perhaps we need some 'basic' constructor function for the csi-lib metrics, with some form of NewRecommendedMetrics function that calls the basic one, but also adds in standard Go-esque metrics? Not sure :)
certmanager_csi_certificate_request_expiration_timestamp_seconds certmanager_csi_certificate_request_ready_status
certmanager_csi_certificate_request_renewal_timestamp_seconds certmanager_csi_driver_issue_call_count
certmanager_csi_driver_issue_error_count
certmanager_csi_managed_certificate_count
certmanager_csi_managed_volume_count
fixes: #60