Support prometheus metrics #73

7ing · 2024-11-22T22:32:55Z

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_total
certmanager_csi_driver_issue_error_count_total
certmanager_csi_managed_certificate_count_total
certmanager_csi_managed_volume_count_total

fixes: #60

CyberArk tracker: VC-46169

cert-manager-prow · 2024-11-22T22:33:05Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

erikgb · 2024-11-22T22:40:08Z

/ok-to-test

munnerz

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).

munnerz · 2025-01-08T14:20:55Z

metrics/metrics.go

+}
+
+// NewServer registers Prometheus metrics and returns a new Prometheus metrics HTTP server.
+func (m *Metrics) NewServer(ln net.Listener) *http.Server {


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).

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.

Could you possibly update the example/ implementation in the root of this repository to demonstrate how to actually add the /metrics endpoint?

Yes, it is there. And modified accordingly based on recent changes.
https://github.com/7ing/csi-lib/blob/abf15631238fa809d10d9c206178c414d9495b4f/test/integration/metrics_test.go#L83-L95

Thanks for the suggestions. I have changed to use a DefaultHandler instead, which could be served as a reference implementation for http handler.

metrics/metrics.go

7ing · 2025-02-05T00:30:18Z

@munnerz Thank you for your valuable inputs. Sorry took so long to make the change. But I guess this version addressed most of your concerns.

cert-manager-prow · 2025-05-30T18:39:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joshvanl for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

inteon · 2025-06-09T10:05:22Z

test/integration/metrics_test.go

+
+	// Should expose that CertificateRequest as ready with expiry and renewal time
+	// node="f56fd9f8b" is the hash value of "test-node" defined in driver_testing.go
+	expectedOutputTemplate := `# HELP certmanager_csi_certificate_request_expiration_timestamp_seconds The date after which the certificate request expires. Expressed as a Unix Epoch Time.


Will certmanager_csi_... be the prefix for the metrics when exposed in https://github.com/cert-manager/csi-driver?
Can we make sure they are all using similar names? Does this happen automatically?
Could you provide an example of before and after for https://github.com/cert-manager/csi-driver with these changes applied?

Thank you @inteon for your review.

Yes, certmanager_csi_... will be the prefix for the metrics when in the csi-driver. It is part of the definition from:
https://github.com/7ing/csi-lib/blob/b9186bad5b6f9af9bc93dbd28571d2d9219700e6/metrics/metrics.go#L27-L31

We choose this name based on cert-manager.io definition: https://github.com/cert-manager/cert-manager/blob/5e09ef6c0552df0bde64746c735cb1ff324b6261/pkg/metrics/metrics.go#L44
All cert-manager controllers have certmanager_.. prefix.

Our https://github.com/cert-manager/csi-driver does not have any certmanager related metrics. Currently it only serves k8s components metrics, like cpu / mem etc. That's why this PR exist. This test files show the expected output regarding certmanager metrics (besides the k8s metrics upon driver configuration).

erikgb · 2025-08-29T17:36:10Z

@7ing We have made some major dependency upgrades in this module. Are you able to rebase your PR, preparing for another round of review? Sorry for the inconvenience and for the delays in review. 😒

7ing · 2025-08-29T18:36:31Z

/retest

7ing · 2025-08-29T18:38:24Z

@erikgb done with rebase

erikgb

This is great stuff! Thanks @7ing! I did my first pass on this PR now, and I think I would prefer using a Prometheus collector to avoid the add/remove metrics for metrics based on API resources. Please take a look and let me know what you think! It's not a blocker, but a pattern we are adopting in cert-manager projects nowadays.

manager/manager.go

metrics/certificaterequest.go

7ing · 2025-09-12T18:44:48Z

Updated the certificate requests' metrics implementation with collector pattern, to align with cert-manager.io project.

Basically the collector will periodically scan the certificaterequest sharedinformer (for expiry) and host metadata files (for renewal time), then record the metrics.

Copilot

Pull Request Overview

This PR adds Prometheus metrics support to the CSI library to expose operational metrics about certificate management. The implementation includes metrics for certificate request expiration timestamps, ready status, renewal times, driver issue call counts, and managed volume/certificate counts.

Key changes:

Added comprehensive metrics collection system with Prometheus integration
Implemented certificate request collector for tracking certificate lifecycle metrics
Added metrics tracking to the manager for monitoring issue calls and errors
Created extensive test coverage for metrics functionality

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
test/util/testutil.go	Exported function to support metrics testing
test/integration/metrics_test.go	Added comprehensive integration test for metrics server functionality
test/driver/driver_testing.go	Added metrics support to test driver infrastructure
metrics/metrics.go	Core metrics implementation with Prometheus registry and HTTP handler
metrics/certificaterequest_test.go	Unit tests for certificate request metrics collection
metrics/certificaterequest_collector.go	Prometheus collector for certificate request lifecycle metrics
manager/manager.go	Integrated metrics tracking into certificate issuance operations
go.mod	Added prometheus client dependency

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

metrics/certificaterequest_collector.go

go.mod

7ing · 2025-09-16T21:35:34Z

/retest-required

7ing · 2025-09-22T16:13:01Z

@erikgb @hjoshi123 I believe this one close to the final version. Let me know if you have further questions. Thanks.

7ing · 2025-10-02T22:26:00Z

Any chance we could add this feature to the next release?

maelvls · 2025-10-16T16:56:24Z

Hi! I'll ask if someone from CyberArk can review this PR.

erikgb · 2025-10-16T19:37:03Z

@7ing, I feel really sorry for the delays in the review of this PR. Sorry! 😢 What worries me most about this (huge) PR is the UX/interface. Are you able to create a draft PR in the csi-driver to see how this can be used in an application? We can always fix bugs in follow-up PR, but breaking changes in APIs is something we try to avoid.

This PR demonstrates csi-lib metrics feature in cert-manager/csi-lib#73 Signed-off-by: Jing Liu <[email protected]>

7ing · 2025-10-22T21:40:13Z

@erikgb we have example-csi in this repo demonstrating how to integrate the metrics handler. Similarly, I have added a draft PR in csi-driver to demonstrate how to use it. cert-manager/csi-driver#502. Hopefully that resolves the concern. Thanks.

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]>

fixes: cert-manager#60 Signed-off-by: Jing Liu <[email protected]>

Signed-off-by: Jing Liu <[email protected]>

7ing · 2025-11-06T23:23:22Z

/retest-required

Copilot

Pull Request Overview

Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-08T14:20:58Z

metrics/metrics.go

@@ -0,0 +1,124 @@
+/*
+Copyright 2024 The cert-manager Authors.


Corrected copyright year from 2024 to 2025 to match the actual year of the file creation (metrics/certificaterequest_collector.go shows 2025).

Suggested change

Copyright 2024 The cert-manager Authors.

Copyright 2025 The cert-manager Authors.

Copilot · 2025-11-08T14:20:59Z

metrics/certificaterequest_test.go

@@ -0,0 +1,450 @@
+/*
+Copyright 2024 The cert-manager Authors.


Corrected copyright year from 2024 to 2025 to match the actual year consistent with other new files in this PR.

Suggested change

Copyright 2024 The cert-manager Authors.

Copyright 2025 The cert-manager Authors.

Copilot · 2025-11-08T14:20:59Z

metrics/metrics.go

+func (m *Metrics) SetupCertificateRequestCollector(nodeId string, metadataReader storage.MetadataReader, certificateRequestLister cmlisters.CertificateRequestLister) {
+	m.certificateRequestCollector = NewCertificateRequestCollector(internalapiutil.HashIdentifier(nodeId), metadataReader, certificateRequestLister)
+	m.registry.MustRegister(m.certificateRequestCollector)
+}
+


The SetupCertificateRequestCollector method re-registers a collector without unregistering the previous one, which will cause a panic from Prometheus. The collector is already registered in the New function at line 97. This method appears unused in the codebase and should likely be removed, or it should first unregister the old collector before registering a new one.

Suggested change

func (m *Metrics) SetupCertificateRequestCollector(nodeId string, metadataReader storage.MetadataReader, certificateRequestLister cmlisters.CertificateRequestLister) {

m.certificateRequestCollector = NewCertificateRequestCollector(internalapiutil.HashIdentifier(nodeId), metadataReader, certificateRequestLister)

m.registry.MustRegister(m.certificateRequestCollector)

}

I agree with Copilot here. This function appears superfluous after the latest changes.

Copilot · 2025-11-08T14:21:00Z

manager/manager.go

 	// It should not be used full-stop :).
 	doNotUse_CallOnEachIssue func()
+
+	// metrics is used to expose Prometheus


The comment is incomplete. It should be: 'metrics is used to expose Prometheus metrics' or 'metrics is used for Prometheus metrics collection'.

Suggested change

// metrics is used to expose Prometheus

// metrics is used to expose Prometheus metrics

Suggested change

// metrics is used to expose Prometheus

// metrics is used for Prometheus metrics collection

erikgb

@7ing, you have been waiting for a long time. Sorry about that! 🙈 This codebase is still a bit unfamiliar to me, so I was hoping for someone else to also review it. I have tried to focus on the UX in my review, so we avoid future breaking changes if something needs to be fixed. Let me know what you think!

I really think this feature is needed, so one way or the other we should try to get it merged soon.

erikgb · 2025-11-08T15:23:13Z

manager/manager.go

 	// things like the number of times issue() is called.
 	// No thread safety is added around this field, and it MUST NOT be used for any implementation logic.
 	// It should not be used full-stop :).
 	doNotUse_CallOnEachIssue func()


Can/should this field be removed now, or is this a follow-up? Ref.

csi-lib/manager/manager.go

Lines 382 to 385 in f3a2df0

// TODO: remove this code and replace with actual metrics support

if m.doNotUse_CallOnEachIssue != nil {

m.doNotUse_CallOnEachIssue()

}

erikgb · 2025-11-08T15:23:59Z

manager/manager.go

 	// It should not be used full-stop :).
 	doNotUse_CallOnEachIssue func()
+
+	// metrics is used to expose Prometheus


Suggested change

// metrics is used to expose Prometheus

// metrics is used for Prometheus metrics collection

erikgb · 2025-11-08T15:32:19Z

metrics/metrics.go

+const (
+	// Namespace is the namespace for csi-lib metric names
+	namespace = "certmanager"
+	subsystem = "csi"


Could it make sense to make the subsystem configurable from the application using csi-lib, with a good default? I am just thinking how this might appear if two CSI drivers using this lib are running in the same environment. It probably doesn't make much sense, but if two drivers using this lib are producing the same metrics, I suggest dropping the driver_ prefix from the metrics name (commented below).

erikgb · 2025-11-08T15:33:29Z

metrics/metrics.go

+			prometheus.CounterOpts{
+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      "driver_issue_call_count_total",


Suggested change

Name: "driver_issue_call_count_total",

Name: "issue_requests_total",

If we want to keep driver_ in the full metrics name, I suggest including it in the subsystem part above.

Naming is hard, but I tried to read https://prometheus.io/docs/practices/naming/, and I think "call" is a bit code-native. Also count/total is almost the same.

erikgb · 2025-11-08T15:33:50Z

metrics/metrics.go

+			prometheus.CounterOpts{
+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      "driver_issue_error_count_total",


Suggested change

Name: "driver_issue_error_count_total",

Name: "issue_errors_total",

If we want to keep driver_ in the full metrics name, I suggest including it in the subsystem part above.

Naming is hard, but I tried to read https://prometheus.io/docs/practices/naming/, and I think count/total is almost the same.

erikgb · 2025-11-08T15:39:16Z

metrics/metrics.go

+func (m *Metrics) SetupCertificateRequestCollector(nodeId string, metadataReader storage.MetadataReader, certificateRequestLister cmlisters.CertificateRequestLister) {
+	m.certificateRequestCollector = NewCertificateRequestCollector(internalapiutil.HashIdentifier(nodeId), metadataReader, certificateRequestLister)
+	m.registry.MustRegister(m.certificateRequestCollector)
+}
+


I agree with Copilot here. This function appears superfluous after the latest changes.

cert-manager-prow bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Nov 22, 2024

cert-manager-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 22, 2024

cert-manager-prow bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 22, 2024

7ing force-pushed the metrics branch from c381e55 to 5f85df6 Compare December 3, 2024 00:28

munnerz reviewed Jan 8, 2025

View reviewed changes

7ing requested a review from munnerz February 13, 2025 21:14

7ing force-pushed the metrics branch from abf1563 to 104e7b0 Compare May 30, 2025 18:39

cert-manager-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 30, 2025

wallrj self-requested a review June 6, 2025 20:26

inteon reviewed Jun 9, 2025

View reviewed changes

wallrj removed their request for review June 20, 2025 09:58

7ing force-pushed the metrics branch from b9186ba to 2b533b0 Compare August 29, 2025 18:13

erikgb reviewed Aug 30, 2025

View reviewed changes

manager/manager.go Outdated Show resolved Hide resolved

manager/manager.go Outdated Show resolved Hide resolved

manager/manager.go Outdated Show resolved Hide resolved

metrics/certificaterequest.go Outdated Show resolved Hide resolved

7ing requested review from erikgb, hjoshi123 and inteon September 12, 2025 18:42

erikgb requested a review from Copilot September 12, 2025 18:51

Copilot AI reviewed Sep 12, 2025

View reviewed changes

metrics/certificaterequest_collector.go Show resolved Hide resolved

go.mod Show resolved Hide resolved

7ing force-pushed the metrics branch from 73994b6 to d6048b8 Compare September 12, 2025 19:04

7ing force-pushed the metrics branch from 14d62c6 to 4cc9575 Compare September 16, 2025 21:29

cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2025

7ing requested a review from hjoshi123 September 17, 2025 22:58

cert-manager-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2025

7ing force-pushed the metrics branch from 7cfaf49 to b5a8ccd Compare October 14, 2025 21:08

cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2025

maelvls added the cybr Used by CyberArk-employed maintainers to report to line management what's being worked on. label Oct 16, 2025

7ing added a commit to 7ing/csi-driver that referenced this pull request Oct 22, 2025

Draft: demo for csi-lib metrics feature

6d9e4e7

This PR demonstrates csi-lib metrics feature in cert-manager/csi-lib#73 Signed-off-by: Jing Liu <[email protected]>

7ing mentioned this pull request Oct 22, 2025

Draft: demo for csi-lib metrics feature cert-manager/csi-driver#502

Draft

7ing added a commit to 7ing/csi-driver that referenced this pull request Oct 22, 2025

Draft: demo for csi-lib metrics feature

a85b2fa

This PR demonstrates csi-lib metrics feature in cert-manager/csi-lib#73 Signed-off-by: Jing Liu <[email protected]>

cert-manager-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2025

7ing added 7 commits November 6, 2025 14:58

Resolve comments for metrics package

79b8abe

fixes: cert-manager#60 Signed-off-by: Jing Liu <[email protected]>

fix promlinter and boilerplate issues

d3aa751

Signed-off-by: Jing Liu <[email protected]>

fix minor golangci-lint lissue

339aec3

Signed-off-by: Jing Liu <[email protected]>

Use prometheus collector pattern for metrics

81eaeda

Signed-off-by: Jing Liu <[email protected]>

Add metric example to simple-csi

2e85438

Signed-off-by: Jing Liu <[email protected]>

Consolidate metrics new function

76af829

Signed-off-by: Jing Liu <[email protected]>

7ing force-pushed the metrics branch from b5a8ccd to 76af829 Compare November 6, 2025 23:18

cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2025

erikgb requested a review from Copilot November 8, 2025 14:18

Copilot AI reviewed Nov 8, 2025

View reviewed changes

erikgb reviewed Nov 8, 2025

View reviewed changes

	Copyright 2024 The cert-manager Authors.
	Copyright 2025 The cert-manager Authors.

	// metrics is used to expose Prometheus
	// metrics is used to expose Prometheus metrics

	// metrics is used to expose Prometheus
	// metrics is used for Prometheus metrics collection

	// TODO: remove this code and replace with actual metrics support
	if m.doNotUse_CallOnEachIssue != nil {
	m.doNotUse_CallOnEachIssue()
	}

	Name: "driver_issue_call_count_total",
	Name: "issue_requests_total",

	Name: "driver_issue_error_count_total",
	Name: "issue_errors_total",

Support prometheus metrics #73

Are you sure you want to change the base?

Support prometheus metrics #73

Conversation

7ing commented Nov 22, 2024 • edited by maelvls Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cert-manager-prow bot commented Nov 22, 2024

Uh oh!

erikgb commented Nov 22, 2024

Uh oh!

munnerz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

7ing commented Feb 5, 2025

Uh oh!

cert-manager-prow bot commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

7ing Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikgb commented Aug 29, 2025

Uh oh!

7ing commented Aug 29, 2025

Uh oh!

7ing commented Aug 29, 2025

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

7ing commented Sep 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

7ing commented Sep 16, 2025

Uh oh!

7ing commented Sep 22, 2025

Uh oh!

7ing commented Oct 2, 2025

Uh oh!

maelvls commented Oct 16, 2025

Uh oh!

erikgb commented Oct 16, 2025

Uh oh!

7ing commented Oct 22, 2025

Uh oh!

7ing commented Nov 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

7ing commented Nov 22, 2024 •

edited by maelvls

Loading

7ing Jun 9, 2025 •

edited

Loading