PR feedback, revised metric naming etc

Author: Dave Storey

controllers/dcluster_controller_databricks.go

@@ -20,9 +20,9 @@ import (
 	"context"
 	"fmt"
 	"reflect"
-	"time"
 
 	databricksv1alpha1 "github.com/microsoft/azure-databricks-operator/api/v1alpha1"
+	"github.com/prometheus/client_golang/prometheus"
 	dbmodels "github.com/xinsnake/databricks-sdk-golang/azure/models"
 )
 
@@ -81,26 +81,30 @@ func (r *DclusterReconciler) delete(instance *databricksv1alpha1.Dcluster) error
 	}
 
 	return trackExecutionTime(dclusterDeleteDuration, func() error {
-		return r.APIClient.Clusters().PermanentDelete(instance.Status.ClusterInfo.ClusterID)
+		err := r.APIClient.Clusters().PermanentDelete(instance.Status.ClusterInfo.ClusterID)
+		trackSuccessFailure(err, dclusterCounterVec, "delete")
+		return err
 	})
 }
 
 func (r *DclusterReconciler) getCluster(clusterID string) (cluster dbmodels.ClusterInfo, err error) {
-	defer trackMillisecondsTaken(time.Now(), dclusterGetDuration)
+	timer := prometheus.NewTimer(dclusterGetDuration)
+	defer timer.ObserveDuration()
 
 	cluster, err = r.APIClient.Clusters().Get(clusterID)
 
-	trackSuccessFailure(err, dclusterGetSuccess, dclusterGetFailure)
+	trackSuccessFailure(err, dclusterCounterVec, "get")
 
 	return cluster, err
 }
 
 func (r *DclusterReconciler) createCluster(instance *databricksv1alpha1.Dcluster) (cluster dbmodels.ClusterInfo, err error) {
-	defer trackMillisecondsTaken(time.Now(), dclusterCreateDuration)
+	timer := prometheus.NewTimer(dclusterCreateDuration)
+	defer timer.ObserveDuration()
 
 	cluster, err = r.APIClient.Clusters().Create(*instance.Spec)
 
-	trackSuccessFailure(err, dclusterCreateSuccess, dclusterCreateFailure)
+	trackSuccessFailure(err, dclusterCounterVec, "create")
 
 	return cluster, err
 }

controllers/dcluster_metrics.go

@@ -22,53 +22,32 @@ import (
 )
 
 var (
-	dclusterCreateSuccess = prometheus.NewCounter(
+	dclusterCounterVec = prometheus.NewCounterVec(
 		prometheus.CounterOpts{
-			Name: "dcluster_create_success_total",
-			Help: "Number of create dcluster success",
-		},
-	)
-	dclusterCreateFailure = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Name: "dcluster_create_failures_total",
-			Help: "Number of create dcluster failures",
-		},
-	)
-
-	dclusterGetSuccess = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Name: "dcluster_get_success_total",
-			Help: "Number of create dcluster success",
-		},
-	)
-	dclusterGetFailure = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Name: "dcluster_get_failures_total",
-			Help: "Number of create dcluster failures",
+			Name: metricPrefix + "dcluster_total",
+			Help: "Counter related to the dCluster CRD partitioned by status and method invoked. Status = success/fail and method indicates REST endpoint",
 		},
+		[]string{"status", "method"},
 	)
 
 	dclusterCreateDuration = prometheus.NewHistogram(prometheus.HistogramOpts{
-		Name:    "dcluster_creation_duration",
-		Help:    "Duration of DB api dcluster create calls.",
-		Buckets: prometheus.LinearBuckets(100, 10, 20),
+		Name: metricPrefix + "dcluster_creation_request_duration_seconds",
+		Help: "Duration of DB api dcluster create calls.",
 	})
 
 	dclusterGetDuration = prometheus.NewHistogram(prometheus.HistogramOpts{
-		Name:    "dcluster_get_duration",
-		Help:    "Duration of DB api dcluster get calls.",
-		Buckets: prometheus.LinearBuckets(100, 10, 20),
+		Name: metricPrefix + "dcluster_get_request_duration_seconds",
+		Help: "Duration of DB api dcluster get calls.",
 	})
 
 	dclusterDeleteDuration = prometheus.NewHistogram(prometheus.HistogramOpts{
-		Name:    "dcluster_delete_duration",
-		Help:    "Duration of DB api dcluster delete calls.",
-		Buckets: prometheus.LinearBuckets(100, 10, 20),
+		Name: metricPrefix + "dcluster_delete_request_duration_seconds",
+		Help: "Duration of DB api dcluster delete calls.",
 	})
 )
 
 func init() {
 	// Register custom metrics with the global prometheus registry
-	metrics.Registry.MustRegister(dclusterCreateSuccess, dclusterCreateFailure, dclusterGetSuccess, dclusterGetFailure,
+	metrics.Registry.MustRegister(dclusterCounterVec,
 		dclusterCreateDuration, dclusterGetDuration, dclusterDeleteDuration)
 }

controllers/djob_controller_databricks.go

@@ -21,9 +21,9 @@ import (
 	"fmt"
 	"reflect"
 	"strings"
-	"time"
 
 	databricksv1alpha1 "github.com/microsoft/azure-databricks-operator/api/v1alpha1"
+	"github.com/prometheus/client_golang/prometheus"
 	dbmodels "github.com/xinsnake/databricks-sdk-golang/azure/models"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -139,27 +139,31 @@ func (r *DjobReconciler) delete(instance *databricksv1alpha1.Djob) error {
 	}
 
 	return trackExecutionTime(djobDeleteDuration, func() error {
-		return r.APIClient.Jobs().Delete(jobID)
+		err := r.APIClient.Jobs().Delete(jobID)
+		trackSuccessFailure(err, djobCounterVec, "delete")
+		return err
 	})
 }
 
 func (r *DjobReconciler) getJob(jobID int64) (job dbmodels.Job, err error) {
-	defer trackMillisecondsTaken(time.Now(), djobGetDuration)
+	timer := prometheus.NewTimer(djobGetDuration)
+	defer timer.ObserveDuration()
 
 	job, err = r.APIClient.Jobs().Get(jobID)
 
-	trackSuccessFailure(err, djobGetSuccess, djobGetFailure)
+	trackSuccessFailure(err, djobCounterVec, "get")
 
 	return job, err
 }
 
 func (r *DjobReconciler) createJob(instance *databricksv1alpha1.Djob) (job dbmodels.Job, err error) {
-	defer trackMillisecondsTaken(time.Now(), djobCreateDuration)
+	timer := prometheus.NewTimer(djobCreateDuration)
+	defer timer.ObserveDuration()
 
 	jobSettings := databricksv1alpha1.ToDatabricksJobSettings(instance.Spec)
 	job, err = r.APIClient.Jobs().Create(jobSettings)
 
-	trackSuccessFailure(err, djobCreateSuccess, djobCreateFailure)
+	trackSuccessFailure(err, djobCounterVec, "create")
 
 	return job, err
 }

controllers/djob_metrics.go

@@ -22,53 +22,32 @@ import (
 )
 
 var (
-	djobCreateSuccess = prometheus.NewCounter(
+	djobCounterVec = prometheus.NewCounterVec(
 		prometheus.CounterOpts{
-			Name: "djob_create_success_total",
-			Help: "Number of create djob success",
-		},
-	)
-	djobCreateFailure = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Name: "djob_create_failures_total",
-			Help: "Number of create djob failures",
-		},
-	)
-
-	djobGetSuccess = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Name: "djob_get_success_total",
-			Help: "Number of get djob success",
-		},
-	)
-	djobGetFailure = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Name: "djob_get_failures_total",
-			Help: "Number of get djob failures",
+			Name: metricPrefix + "djob_total",
+			Help: "Counter related to the dJob CRD partitioned by status and method invoked. Status = success/fail and method indicates REST endpoint",
 		},
+		[]string{"status", "method"},
 	)
 
 	djobCreateDuration = prometheus.NewHistogram(prometheus.HistogramOpts{
-		Name:    "djob_creation_duration",
-		Help:    "Duration of DB api djob create calls.",
-		Buckets: prometheus.LinearBuckets(100, 10, 20),
+		Name: metricPrefix + "djob_creation_request_duration_seconds",
+		Help: "Duration of DB api djob create calls.",
 	})
 
 	djobGetDuration = prometheus.NewHistogram(prometheus.HistogramOpts{
-		Name:    "djob_get_duration",
-		Help:    "Duration of DB api djob get calls.",
-		Buckets: prometheus.LinearBuckets(100, 10, 20),
+		Name: metricPrefix + "djob_get_request_duration_seconds",
+		Help: "Duration of DB api djob get calls.",
 	})
 
 	djobDeleteDuration = prometheus.NewHistogram(prometheus.HistogramOpts{
-		Name:    "djob_delete_duration",
-		Help:    "Duration of DB api djob delete calls.",
-		Buckets: prometheus.LinearBuckets(100, 10, 20),
+		Name: metricPrefix + "djob_delete_request_duration_seconds",
+		Help: "Duration of DB api djob delete calls.",
 	})
 )
 
 func init() {
 	// Register custom metrics with the global prometheus registry
-	metrics.Registry.MustRegister(djobCreateSuccess, djobCreateFailure, djobGetSuccess, djobGetFailure,
+	metrics.Registry.MustRegister(djobCounterVec,
 		djobCreateDuration, djobGetDuration, djobDeleteDuration)
 }

controllers/metrics.go

@@ -17,28 +17,25 @@ limitations under the License.
 package controllers
 
 import (
-	"time"
-
 	"github.com/prometheus/client_golang/prometheus"
 )
 
-func trackExecutionTime(histogram prometheus.Histogram, f func() error) error {
-	startTime := time.Now()
-
-	defer trackMillisecondsTaken(startTime, histogram)
+const (
+	metricPrefix  = "databricks_"
+	successMetric = "success"
+	failureMetric = "failure"
+)
 
+func trackExecutionTime(histogram prometheus.Histogram, f func() error) error {
+	timer := prometheus.NewTimer(histogram)
+	defer timer.ObserveDuration()
 	return f()
 }
 
-func trackMillisecondsTaken(startTime time.Time, histogram prometheus.Histogram) {
-	duration := float64(time.Since(startTime) / time.Millisecond)
-	histogram.Observe(duration)
-}
-
-func trackSuccessFailure(err error, success prometheus.Counter, failure prometheus.Counter) {
+func trackSuccessFailure(err error, counterVec *prometheus.CounterVec, method string) {
 	if err == nil {
-		success.Inc()
+		counterVec.With(prometheus.Labels{"status": successMetric, "method": method}).Inc()
 	} else {
-		failure.Inc()
+		counterVec.With(prometheus.Labels{"status": failureMetric, "method": method}).Inc()
 	}
 }

controllers/run_controller_databricks.go

@@ -24,6 +24,7 @@ import (
 	"time"
 
 	databricksv1alpha1 "github.com/microsoft/azure-databricks-operator/api/v1alpha1"
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/xinsnake/databricks-sdk-golang/azure"
 	dbmodels "github.com/xinsnake/databricks-sdk-golang/azure/models"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -112,12 +113,16 @@ func (r *RunReconciler) delete(instance *databricksv1alpha1.Run) error {
 	time.Sleep(15 * time.Second)
 
 	return trackExecutionTime(runDeleteDuration, func() error {
-		return r.APIClient.Jobs().RunsDelete(runID)
+		err := r.APIClient.Jobs().RunsDelete(runID)
+		trackSuccessFailure(err, runCounterVec, "delete")
+		return err
 	})
 }
 
 func (r *RunReconciler) runUsingRunNow(instance *databricksv1alpha1.Run) (*dbmodels.Run, error) {
-	defer trackMillisecondsTaken(time.Now(), runNowDuration)
+	timer := prometheus.NewTimer(runNowDuration)
+	defer timer.ObserveDuration()
+
 	runParameters := dbmodels.RunParameters{
 		JarParams:         instance.Spec.JarParams,
 		NotebookParams:    instance.Spec.NotebookParams,
@@ -142,12 +147,13 @@ func (r *RunReconciler) runUsingRunNow(instance *databricksv1alpha1.Run) (*dbmod
 	})
 
 	run, err := r.APIClient.Jobs().RunNow(k8sJob.Status.JobStatus.JobID, runParameters)
-	trackSuccessFailure(err, runNowSuccess, runNowFailure)
+	trackSuccessFailure(err, runCounterVec, "runsnow")
 	return &run, err
 }
 
 func (r *RunReconciler) runUsingRunsSubmit(instance *databricksv1alpha1.Run) (*dbmodels.Run, error) {
-	defer trackMillisecondsTaken(time.Now(), runSubmitDuration)
+	timer := prometheus.NewTimer(runSubmitDuration)
+	defer timer.ObserveDuration()
 
 	clusterSpec := dbmodels.ClusterSpec{
 		NewCluster:        instance.Spec.NewCluster,
@@ -162,26 +168,28 @@ func (r *RunReconciler) runUsingRunsSubmit(instance *databricksv1alpha1.Run) (*d
 	}
 
 	run, err := r.APIClient.Jobs().RunsSubmit(instance.Spec.RunName, clusterSpec, jobTask, instance.Spec.TimeoutSeconds)
-	trackSuccessFailure(err, runSubmitSuccess, runSubmitFailure)
+	trackSuccessFailure(err, runCounterVec, "runssubmit")
 	return &run, err
 }
 
 func (r *RunReconciler) getRun(runID int64) (dbmodels.Run, error) {
-	defer trackMillisecondsTaken(time.Now(), runGetDuration)
+	timer := prometheus.NewTimer(runGetDuration)
+	defer timer.ObserveDuration()
 
 	runOutput, err := r.APIClient.Jobs().RunsGet(runID)
 
-	trackSuccessFailure(err, runGetSuccess, runGetFailure)
+	trackSuccessFailure(err, runCounterVec, "get")
 
 	return runOutput, err
 }
 
 func (r *RunReconciler) getRunOutput(runID int64) (azure.JobsRunsGetOutputResponse, error) {
-	defer trackMillisecondsTaken(time.Now(), runGetOutputDuration)
+	timer := prometheus.NewTimer(runGetOutputDuration)
+	defer timer.ObserveDuration()
 
 	runOutput, err := r.APIClient.Jobs().RunsGetOutput(runID)
 
-	trackSuccessFailure(err, runGetOutputSuccess, runGetOutputFailure)
+	trackSuccessFailure(err, runCounterVec, "getoutput")
 
 	return runOutput, err
 }