Repository: jetstack/cert-manager (go)

Issue:
clock_time_seconds metric should be a gauge

**Describe the bug**:
As other people reported (https://github.com/jetstack/cert-manager/pull/4105#issuecomment-930563632) the cert-manager clock_time_seconds metric should be a gauge, not a counter for it to work properly.

This is also a general view that timestamps should be gauges: https://www.robustperception.io/are-increasing-timestamps-counters-or-gauges

Taking into account that the other timestamps metrics offered by cert-manager are gauges, this makes even more sense to change to a gauge as well.

Open to discussion and also to contribute to a fix.


Pull request description:
Adds `clock_time_seconds_gauge` metric which returns the current clock time, based on unix time since time began
Fixes #4560 

The clockTimeSeconds metric was first added, however this was
erroneously made a "counter" metric type. Time can in fact go backwards,
 see:
 - https://github.com/jetstack/cert-manager/issues/4560
- https://www.robustperception.io/are-increasing-timestamps-counters-or-gauges
In order to not break users relying on the `clock_time_seconds` metric,

a new `clock_time_seconds_gauge` metric of type gauge is added which
implements the same thing.


```release-note
Adds `clock_time_seconds_gauge` metric which returns the current clock time, based on seconds since 1970/01/01 UTC
```

/milestone v1.7
/kind cleanup


Patch:
diff --git a/pkg/metrics/BUILD.bazel b/pkg/metrics/BUILD.bazel
index 7873d47cd..5266b1da4 100644
--- a/pkg/metrics/BUILD.bazel
+++ b/pkg/metrics/BUILD.bazel
@@ -47,7 +47,9 @@ go_test(
         "//pkg/apis/meta/v1:go_default_library",
         "//pkg/logs/testing:go_default_library",
         "//test/unit/gen:go_default_library",
+        "@com_github_prometheus_client_golang//prometheus:go_default_library",
         "@com_github_prometheus_client_golang//prometheus/testutil:go_default_library",
+        "@com_github_stretchr_testify//assert:go_default_library",
         "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library",
         "@io_k8s_utils//clock:go_default_library",
         "@io_k8s_utils//clock/testing:go_default_library",
diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go
index 38616b6fe..f3e961426 100644
--- a/pkg/metrics/metrics.go
+++ b/pkg/metrics/metrics.go
@@ -29,12 +29,10 @@ import (
 	"net/http"
 	"time"
 
-	"k8s.io/utils/clock"
-
 	"github.com/go-logr/logr"
-
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
+	"k8s.io/utils/clock"
 
 	cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1"
 )
@@ -54,6 +52,7 @@ type Metrics struct {
 	registry *prometheus.Registry
 
 	clockTimeSeconds                 prometheus.CounterFunc
+	clockTimeSecondsGauge            prometheus.GaugeFunc
 	certificateExpiryTimeSeconds     *prometheus.GaugeVec
 	certificateRenewalTimeSeconds    *prometheus.GaugeVec
 	certificateReadyStatus           *prometheus.GaugeVec
@@ -67,10 +66,30 @@ var readyConditionStatuses = [...]cmmeta.ConditionStatus{cmmeta.ConditionTrue, c
 // New creates a Metrics struct and populates it with prometheus metric types.
 func New(log logr.Logger, c clock.Clock) *Metrics {
 	var (
+		// Deprecated in favour of clock_time_seconds_gauge.
 		clockTimeSeconds = prometheus.NewCounterFunc(
 			prometheus.CounterOpts{
 				Namespace: namespace,
 				Name:      "clock_time_seconds",
+				Help:      "DEPRECATED: use clock_time_seconds_gauge instead. The clock time given in seconds (from 1970/01/01 UTC).",
+			},
+			func() float64 {
+				return float64(c.Now().Unix())
+			},
+		)
+
+		// The clockTimeSeconds metric was first added, however this was
+		// erroneously made a "counter" metric type. Time can in fact go backwards,
+		// see:
+		// - https://github.com/jetstack/cert-manager/issues/4560
+		// - https://www.robustperception.io/are-increasing-timestamps-counters-or-gauges
+		// In order to not break users relying on the `clock_time_seconds` metric,
+		// a new `clock_time_seconds_gauge` metric of type gauge is added which
+		// implements the same thing.
+		clockTimeSecondsGauge = prometheus.NewGaugeFunc(
+			prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "clock_time_seconds_gauge",
 				Help:      "The clock time given in seconds (from 1970/01/01 UTC).",
 			},
 			func() float64 {
@@ -146,6 +165,7 @@ func New(log logr.Logger, c clock.Clock) *Metrics {
 		registry: prometheus.NewRegistry(),
 
 		clockTimeSeconds:                 clockTimeSeconds,
+		clockTimeSecondsGauge:            clockTimeSecondsGauge,
 		certificateExpiryTimeSeconds:     certificateExpiryTimeSeconds,
 		certificateRenewalTimeSeconds:    certificateRenewalTimeSeconds,
 		certificateReadyStatus:           certificateReadyStatus,
@@ -160,6 +180,7 @@ func New(log logr.Logger, c clock.Clock) *Metrics {
 // NewServer registers Prometheus metrics and returns a new Prometheus metrics HTTP server.
 func (m *Metrics) NewServer(ln net.Listener) *http.Server {
 	m.registry.MustRegister(m.clockTimeSeconds)
+	m.registry.MustRegister(m.clockTimeSecondsGauge)
 	m.registry.MustRegister(m.certificateExpiryTimeSeconds)
 	m.registry.MustRegister(m.certificateRenewalTimeSeconds)
 	m.registry.MustRegister(m.certificateReadyStatus)


Test patch:
diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go
index cf5262c36..21e75d457 100644
--- a/pkg/metrics/metrics_test.go
+++ b/pkg/metrics/metrics_test.go
@@ -22,39 +22,49 @@ import (
 	"testing"
 	"time"
 
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/testutil"
+	"github.com/stretchr/testify/assert"
 
 	logtesting "github.com/jetstack/cert-manager/pkg/logs/testing"
 	fakeclock "k8s.io/utils/clock/testing"
 )
 
-var (
-	fixedClock = fakeclock.NewFakeClock(time.Now())
-)
+func Test_clockTimeSeconds(t *testing.T) {
+	fixedClock := fakeclock.NewFakeClock(time.Now())
+	m := New(logtesting.TestLogger{T: t}, fixedClock)
+
+	tests := map[string]struct {
+		metricName string
+		metric     prometheus.Collector
 
-func TestClockMetrics(t *testing.T) {
-	type testT struct {
 		expected string
-	}
-	tests := map[string]testT{
-		"clock time seconds as expected": {
+	}{
+		"clock_time_seconds of type counter": {
+			metricName: "certmanager_clock_time_seconds",
+			metric:     m.clockTimeSeconds,
 			expected: fmt.Sprintf(`
-  # HELP certmanager_clock_time_seconds The clock time given in seconds (from 1970/01/01 UTC).
-  # TYPE certmanager_clock_time_seconds counter
-	certmanager_clock_time_seconds %f
+# HELP certmanager_clock_time_seconds DEPRECATED: use clock_time_seconds_gauge instead. The clock time given in seconds (from 1970/01/01 UTC).
+# TYPE certmanager_clock_time_seconds counter
+certmanager_clock_time_seconds %f
+	`, float64(fixedClock.Now().Unix())),
+		},
+		"clock_time_seconds_gauge of type gauge": {
+			metricName: "certmanager_clock_time_seconds_gauge",
+			metric:     m.clockTimeSecondsGauge,
+			expected: fmt.Sprintf(`
+# HELP certmanager_clock_time_seconds_gauge The clock time given in seconds (from 1970/01/01 UTC).
+# TYPE certmanager_clock_time_seconds_gauge gauge
+certmanager_clock_time_seconds_gauge %f
 	`, float64(fixedClock.Now().Unix())),
 		},
 	}
-	for n, test := range tests {
-		t.Run(n, func(t *testing.T) {
-			m := New(logtesting.TestLogger{T: t}, fixedClock)
-
-			if err := testutil.CollectAndCompare(m.clockTimeSeconds,
-				strings.NewReader(test.expected),
-				"certmanager_clock_time_seconds",
-			); err != nil {
-				t.Errorf("unexpected collecting result:\n%s", err)
-			}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			assert.NoError(t,
+				testutil.CollectAndCompare(test.metric, strings.NewReader(test.expected), test.metricName),
+			)
 		})
 	}
 }
diff --git a/test/integration/certificates/metrics_controller_test.go b/test/integration/certificates/metrics_controller_test.go
index db029ae57..87e808d9e 100644
--- a/test/integration/certificates/metrics_controller_test.go
+++ b/test/integration/certificates/metrics_controller_test.go
@@ -43,10 +43,15 @@ import (
 )
 
 var (
-	fixedClock  = fakeclock.NewFakeClock(time.Date(2006, 1, 2, 15, 4, 5, 0, time.UTC))
-	clockMetric = fmt.Sprintf(`# HELP certmanager_clock_time_seconds The clock time given in seconds (from 1970/01/01 UTC).
+	fixedClock = fakeclock.NewFakeClock(time.Date(2006, 1, 2, 15, 4, 5, 0, time.UTC))
+
+	clockCounterMetric = fmt.Sprintf(`# HELP certmanager_clock_time_seconds DEPRECATED: use clock_time_seconds_gauge instead. The clock time given in seconds (from 1970/01/01 UTC).
 # TYPE certmanager_clock_time_seconds counter
 certmanager_clock_time_seconds %.9e`, float64(fixedClock.Now().Unix()))
+	clockGaugeMetric = fmt.Sprintf(`
+# HELP certmanager_clock_time_seconds_gauge The clock time given in seconds (from 1970/01/01 UTC).
+# TYPE certmanager_clock_time_seconds_gauge gauge
+certmanager_clock_time_seconds_gauge %.9e`, float64(fixedClock.Now().Unix()))
 )
 
 // TestMetricscontoller performs a basic test to ensure that Certificates
@@ -152,7 +157,7 @@ func TestMetricsController(t *testing.T) {
 	}
 
 	// Should expose no additional metrics
-	waitForMetrics(clockMetric)
+	waitForMetrics(clockCounterMetric + clockGaugeMetric)
 
 	// Create Certificate
 	crt := gen.Certificate(crtName,
@@ -180,7 +185,7 @@ certmanager_certificate_ready_status{condition="Unknown",name="testcrt",namespac
 # HELP certmanager_certificate_renewal_timestamp_seconds The number of seconds before expiration time the certificate should renew.
 # TYPE certmanager_certificate_renewal_timestamp_seconds gauge
 certmanager_certificate_renewal_timestamp_seconds{name="testcrt",namespace="testns"} 0
-` + clockMetric + `
+` + clockCounterMetric + clockGaugeMetric + `
 # HELP certmanager_controller_sync_call_count The number of sync() calls made by a controller.
 # TYPE certmanager_controller_sync_call_count counter
 certmanager_controller_sync_call_count{controller="metrics_test"} 1
@@ -216,7 +221,7 @@ certmanager_certificate_ready_status{condition="Unknown",name="testcrt",namespac
 # HELP certmanager_certificate_renewal_timestamp_seconds The number of seconds before expiration time the certificate should renew.
 # TYPE certmanager_certificate_renewal_timestamp_seconds gauge
 certmanager_certificate_renewal_timestamp_seconds{name="testcrt",namespace="testns"} 100
-` + clockMetric + `
+` + clockCounterMetric + clockGaugeMetric + `
 # HELP certmanager_controller_sync_call_count The number of sync() calls made by a controller.
 # TYPE certmanager_controller_sync_call_count counter
 certmanager_controller_sync_call_count{controller="metrics_test"} 2
@@ -228,7 +233,7 @@ certmanager_controller_sync_call_count{controller="metrics_test"} 2
 	}
 
 	// Should expose no Certificates and only metrics sync count increase
-	waitForMetrics(clockMetric + `
+	waitForMetrics(clockCounterMetric + clockGaugeMetric + `
 # HELP certmanager_controller_sync_call_count The number of sync() calls made by a controller.
 # TYPE certmanager_controller_sync_call_count counter
 certmanager_controller_sync_call_count{controller="metrics_test"} 3