add custom metrics (#49)

* add http transport metrics
make webhooks optional

* Delete README-FORK.md

* add license header

* remove linter findings in README

* check for registered metrics on CF clients

* use constants and update comments

* write metrics to file (only for debugging)

* check metrics >= 0

* metrics has to exist

* add webhookCertDir

---------

Co-authored-by: RalfHammer <119853077+RalfHammer@users.noreply.github.com>

.local/README.md

@@ -1,23 +1,34 @@
 # Instructions for local development
 
-Prerequisite: K8s cluster (kind, minikube) with cert-manager installed.
+**Prerequisites**: K8s cluster (kind, minikube) with cert-manager installed.
 
 1. Deploy custom resource definnitions:
+
    ```bash
    kubectl apply -f crds
    ```
 
-2. Deploy webhook definitions and according objects:
+2. Copy a sufficently authorized kubeconfig to `.kubeconfig` in the root folder of this repository, e.g.:
+
+   ```bash
+   cp ~/.kube/config .kubeconfig
+   ```
+
+Afterwards (if using vscode) it should be possible to start the operator with the included launch configuration.
+
+Optional, if you want to test the webhook locally:
+
+1. Deploy webhook definitions and according objects:
+
    ```bash
    # replace HOST_IP below with a non-loopback interface address of your desktop
    HOST_IP=1.2.3.4 envsubst < .local/k8s-resources.yaml | kubectl apply -f -
    ```
 
-3. Extract the TLS server keypair:
+2. Extract the TLS server keypair:
+
    ```bash
    .local/getcerts.sh
    ```
 
-4. Copy a sufficently authorized kubeconfig to `.kubeconfig` in the root folder of this repository.
-
-Afterwards (if using vscode) it should be possible to start the operator with the included launch configuration.
+3. in .vscode/launch.json, set `--enableWebhook=true`

.vscode/launch.json

@@ -14,6 +14,7 @@
                 "--kubeconfig=${workspaceFolder}/.kubeconfig",
                 "--webhook-bind-address=:2443",
                 "--webhook-tls-directory=${workspaceFolder}/.local/ssl",
+                "--enableWebhooks=false",
                 "--cluster-resource-namespace=default",
                 "--sap-binding-metadata"
             ]

internal/cf/client.go

@@ -11,8 +11,10 @@ import (
 
 	cfclient "github.com/cloudfoundry-community/go-cfclient/v3/client"
 	cfconfig "github.com/cloudfoundry-community/go-cfclient/v3/config"
+	"sigs.k8s.io/controller-runtime/pkg/metrics"
 
 	"github.com/sap/cf-service-operator/internal/facade"
+	cfmetrics "github.com/sap/cf-service-operator/pkg/metrics"
 )
 
 const (
@@ -70,6 +72,13 @@ func newOrganizationClient(organizationName string, url string, username string,
 	if err != nil {
 		return nil, err
 	}
+	httpClient := config.HTTPClient()
+	transport, err := cfmetrics.AddMetricsToTransport(httpClient.Transport, metrics.Registry, "cf-api", url)
+	if err != nil {
+		return nil, err
+	}
+	httpClient.Transport = transport
+	config.WithHTTPClient(httpClient)
 	c, err := cfclient.New(config)
 	if err != nil {
 		return nil, err
@@ -94,6 +103,13 @@ func newSpaceClient(spaceGuid string, url string, username string, password stri
 	if err != nil {
 		return nil, err
 	}
+	httpClient := config.HTTPClient()
+	transport, err := cfmetrics.AddMetricsToTransport(httpClient.Transport, metrics.Registry, "cf-api", url)
+	if err != nil {
+		return nil, err
+	}
+	httpClient.Transport = transport
+	config.WithHTTPClient(httpClient)
 	c, err := cfclient.New(config)
 	if err != nil {
 		return nil, err

internal/cf/client_test.go

@@ -13,6 +13,8 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/ghttp"
+	"github.com/prometheus/client_golang/prometheus"
+	"sigs.k8s.io/controller-runtime/pkg/metrics"
 )
 
 // constants useful for this file
@@ -90,10 +92,11 @@ var _ = Describe("CF Client tests", Ordered, func() {
 
 	Describe("NewOrganizationClient", func() {
 		BeforeEach(func() {
-			// Reset the cache so tests can be run independently
+			// Reset some entities to enable tests to run independently
 			clientCache = make(map[clientIdentifier]*clientCacheEntry)
-			// Reset server call counts
+			metrics.Registry = prometheus.NewRegistry()
 			server.Reset()
+
 			// Register handlers
 			server.RouteToHandler("GET", "/", ghttp.CombineHandlers(
 				ghttp.RespondWithJSONEncodedPtr(&statusCode, &rootResult),
@@ -133,6 +136,19 @@ var _ = Describe("CF Client tests", Ordered, func() {
 			Expect(server.ReceivedRequests()[2].URL.Path).To(Equal(spacesURI))
 
 			Expect(server.ReceivedRequests()).To(HaveLen(3))
+
+			// verify metrics
+			metricsList, err := metrics.Registry.Gather()
+			Expect(err).To(BeNil())
+			Expect(metricsList).To(HaveLen(3))
+			verified := false
+			for _, m := range metricsList {
+				if *m.Name == "http_client_requests_total" {
+					Expect(m.Metric[0].Counter.GetValue()).To(BeNumerically(">", 0))
+					verified = true
+				}
+			}
+			Expect(verified).To(BeTrue())
 		})
 
 		It("should be able to query some org twice", func() {
@@ -190,10 +206,11 @@ var _ = Describe("CF Client tests", Ordered, func() {
 
 	Describe("NewSpaceClient", func() {
 		BeforeEach(func() {
-			// Reset the cache so tests can be run independently
+			// Reset some entities to enable tests to run independently
 			clientCache = make(map[clientIdentifier]*clientCacheEntry)
-			// Reset server call counts
+			metrics.Registry = prometheus.NewRegistry()
 			server.Reset()
+
 			// Register handlers
 			server.RouteToHandler("GET", "/", ghttp.CombineHandlers(
 				ghttp.RespondWithJSONEncodedPtr(&statusCode, &rootResult),
@@ -216,7 +233,7 @@ var _ = Describe("CF Client tests", Ordered, func() {
 			Expect(server.ReceivedRequests()).To(HaveLen(1))
 		})
 
-		It("should be able to query space", func() {
+		It("should be able to query some space", func() {
 			spaceClient, err := NewSpaceClient(OrgName, url, Username, Password)
 			Expect(err).To(BeNil())
 
@@ -233,6 +250,19 @@ var _ = Describe("CF Client tests", Ordered, func() {
 			Expect(server.ReceivedRequests()[2].URL.Path).To(Equal(serviceInstancesURI))
 
 			Expect(server.ReceivedRequests()).To(HaveLen(3))
+
+			// verify metrics
+			metricsList, err := metrics.Registry.Gather()
+			Expect(err).To(BeNil())
+			Expect(metricsList).To(HaveLen(3))
+			verified := false
+			for _, m := range metricsList {
+				if *m.Name == "http_client_requests_total" {
+					Expect(m.Metric[0].Counter.GetValue()).To(BeNumerically(">", 0))
+					verified = true
+				}
+			}
+			Expect(verified).To(BeTrue())
 		})
 
 		It("should be able to query some space twice", func() {
@@ -286,5 +316,46 @@ var _ = Describe("CF Client tests", Ordered, func() {
 			Expect(server.ReceivedRequests()[3].RequestURI).To(ContainSubstring(Owner2))
 		})
 
+		It("should register prometheus metrics for OrgClient", func() {
+			orgClient, err := NewOrganizationClient(OrgName, url, Username, Password)
+			Expect(err).To(BeNil())
+			Expect(orgClient).ToNot(BeNil())
+
+			// retrieve names of registered metrics
+			metricsList, err := metrics.Registry.Gather()
+			Expect(err).To(BeNil())
+			Expect(metricsList).To(HaveLen(3))
+			metricNames := make([]string, len(metricsList))
+			for i, m := range metricsList {
+				metricNames[i] = *m.Name
+			}
+
+			Expect(metricNames).To(ContainElement("http_client_request_duration_seconds"))
+			Expect(metricNames).To(ContainElement("http_client_requests_in_flight"))
+			Expect(metricNames).To(ContainElement("http_client_requests_total"))
+		})
+
+		It("should register prometheus metrics for SpaceClient", func() {
+			spaceClient, err := NewSpaceClient(SpaceName, url, Username, Password)
+			Expect(err).To(BeNil())
+			Expect(spaceClient).ToNot(BeNil())
+
+			// retrieve names of registered metrics
+			metricsList, err := metrics.Registry.Gather()
+			Expect(err).To(BeNil())
+			Expect(metricsList).To(HaveLen(3))
+			metricNames := make([]string, len(metricsList))
+			for i, m := range metricsList {
+				metricNames[i] = *m.Name
+			}
+
+			Expect(metricNames).To(ContainElement("http_client_request_duration_seconds"))
+			Expect(metricNames).To(ContainElement("http_client_requests_in_flight"))
+			Expect(metricNames).To(ContainElement("http_client_requests_total"))
+
+			// for debugging: write metrics to file
+			// prometheus.WriteToTextfile("metrics.txt", metrics.Registry)
+		})
+
 	})
 })

main.go

@@ -53,12 +53,14 @@ func main() {
 	var webhookAddr string
 	var webhookCertDir string
 	var enableLeaderElection bool
+	var enableWebhooks bool
 	var clusterResourceNamespace string
 	var enableBindingMetadata bool
 	flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
 	flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
 	flag.StringVar(&webhookAddr, "webhook-bind-address", ":9443", "The address the webhook endpoint binds to.")
-	flag.StringVar(&webhookCertDir, "webhook-tls-directory", "", "The directory containing tls server key and certificate, as tls.key and tls.crt; defaults to $TMPDIR/k8s-webhook-server/serving-certs.")
+	flag.StringVar(&webhookCertDir, "webhook-tls-directory", "", "The directory containing TLS server key and certificate, as tls.key and tls.crt; defaults to $TMPDIR/k8s-webhook-server/serving-certs.")
+	flag.BoolVar(&enableWebhooks, "enableWebhooks", true, "Enable webhooks in controller. May be disabled for local development.")
 	flag.BoolVar(&enableLeaderElection, "leader-elect", false, "Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.")
 	flag.StringVar(&clusterResourceNamespace, "cluster-resource-namespace", "", "The namespace for secrets in which cluster-scoped resources are found.")
 	flag.BoolVar(&enableBindingMetadata, "sap-binding-metadata", false, "Enhance binding secrets by SAP binding metadata by default.")
@@ -97,7 +99,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
+	options := ctrl.Options{
 		Scheme: scheme,
 		// TODO: disable cache for further resources (e.g. secrets) ?
 		Client: client.Options{
@@ -113,16 +115,19 @@ func main() {
 		LeaderElection:                enableLeaderElection,
 		LeaderElectionID:              LeaderElectionID,
 		LeaderElectionReleaseOnCancel: true,
-		WebhookServer: webhook.NewServer(webhook.Options{
-			Host:    webhookHost,
-			Port:    webhookPort,
-			CertDir: webhookCertDir,
-		}),
 		Metrics: metricsserver.Options{
 			BindAddress: metricsAddr,
 		},
 		HealthProbeBindAddress: probeAddr,
-	})
+	}
+	if enableWebhooks {
+		options.WebhookServer = webhook.NewServer(webhook.Options{
+			Host:    webhookHost,
+			Port:    webhookPort,
+			CertDir: webhookCertDir,
+		})
+	}
+	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), options)
 	if err != nil {
 		setupLog.Error(err, "unable to start manager")
 		os.Exit(1)
@@ -169,21 +174,23 @@ func main() {
 		setupLog.Error(err, "unable to create controller", "controller", "ServiceBinding")
 		os.Exit(1)
 	}
-	if err = (&cfv1alpha1.Space{}).SetupWebhookWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create webhook", "webhook", "Space")
-		os.Exit(1)
-	}
-	if err = (&cfv1alpha1.ClusterSpace{}).SetupWebhookWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create webhook", "webhook", "ClusterSpace")
-		os.Exit(1)
-	}
-	if err = (&cfv1alpha1.ServiceInstance{}).SetupWebhookWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create webhook", "webhook", "ServiceInstance")
-		os.Exit(1)
-	}
-	if err = (&cfv1alpha1.ServiceBinding{}).SetupWebhookWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create webhook", "webhook", "ServiceBinding")
-		os.Exit(1)
+	if enableWebhooks {
+		if err = (&cfv1alpha1.Space{}).SetupWebhookWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create webhook", "webhook", "Space")
+			os.Exit(1)
+		}
+		if err = (&cfv1alpha1.ClusterSpace{}).SetupWebhookWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create webhook", "webhook", "ClusterSpace")
+			os.Exit(1)
+		}
+		if err = (&cfv1alpha1.ServiceInstance{}).SetupWebhookWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create webhook", "webhook", "ServiceInstance")
+			os.Exit(1)
+		}
+		if err = (&cfv1alpha1.ServiceBinding{}).SetupWebhookWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create webhook", "webhook", "ServiceBinding")
+			os.Exit(1)
+		}
 	}
 	// +kubebuilder:scaffold:builder