feat: add prometheus metrics around proxy extension requests (argopro…

…j#17012)

* feat: add prometheus metrics around proxy extension requests

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* update go.mod

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* fix metrics bugs

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* fix unit-test

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* Add unit suffix in the duration metric

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* update doc

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

---------

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
Signed-off-by: penglongli <pelenli@tencent.com>

docs/operator-manual/metrics.md

@@ -70,6 +70,8 @@ Scraped at the `argocd-server-metrics:8083/metrics` endpoint.
 | `argocd_redis_request_total` | counter | Number of Kubernetes requests executed during application reconciliation. |
 | `grpc_server_handled_total` | counter | Total number of RPCs completed on the server, regardless of success or failure. |
 | `grpc_server_msg_sent_total` | counter | Total number of gRPC stream messages sent by the server. |
+| `argocd_proxy_extension_request_total` | counter | Number of requests sent to the configured proxy extensions. |
+| `argocd_proxy_extension_request_duration_seconds` | histogram | Request duration in seconds between the Argo CD API server and the proxy extension backend. |
 
 ## Repo Server Metrics
 Metrics about the Repo Server.

go.mod

@@ -26,6 +26,7 @@ require (
 	github.com/cyphar/filepath-securejoin v0.2.4
 	github.com/dustin/go-humanize v1.0.1
 	github.com/evanphx/json-patch v5.9.0+incompatible
+	github.com/felixge/httpsnoop v1.0.3
 	github.com/fsnotify/fsnotify v1.6.0
 	github.com/gfleury/go-bitbucket-v1 v0.0.0-20220301131131-8e7ed04b843e
 	github.com/go-git/go-git/v5 v5.11.0
@@ -179,7 +180,6 @@ require (
 	github.com/evanphx/json-patch/v5 v5.6.0 // indirect
 	github.com/exponent-io/jsonpath v0.0.0-20151013193312-d6023ce2651d // indirect
 	github.com/fatih/camelcase v1.0.0 // indirect
-	github.com/felixge/httpsnoop v1.0.3 // indirect
 	github.com/fvbommel/sortorder v1.0.1 // indirect
 	github.com/ghodss/yaml v1.0.0 // indirect
 	github.com/go-errors/errors v1.4.2 // indirect

server/extension/extension.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/felixge/httpsnoop"
 	log "github.com/sirupsen/logrus"
 	"gopkg.in/yaml.v3"
 
@@ -300,6 +301,19 @@ type Manager struct {
 	project     ProjectGetter
 	rbac        RbacEnforcer
 	registry    ExtensionRegistry
+	metricsReg  ExtensionMetricsRegistry
+}
+
+// ExtensionMetricsRegistry exposes operations to update http metrics in the Argo CD
+// API server.
+type ExtensionMetricsRegistry interface {
+	// IncExtensionRequestCounter will increase the request counter for the given
+	// extension with the given status.
+	IncExtensionRequestCounter(extension string, status int)
+	// ObserveExtensionRequestDuration will register the request roundtrip duration
+	// between Argo CD API Server and the extension backend service for the given
+	// extension.
+	ObserveExtensionRequestDuration(extension string, duration time.Duration)
 }
 
 // NewManager will initialize a new manager.
@@ -423,7 +437,8 @@ func validateConfigs(configs *ExtensionConfigs) error {
 }
 
 // NewProxy will instantiate a new reverse proxy based on the provided
-// targetURL and config.
+// targetURL and config. It will remove sensitive information from the
+// incoming request such as the Authorization and Cookie headers.
 func NewProxy(targetURL string, headers []Header, config ProxyConfig) (*httputil.ReverseProxy, error) {
 	url, err := url.Parse(targetURL)
 	if err != nil {
@@ -484,6 +499,10 @@ func (m *Manager) RegisterExtensions() error {
 	if err != nil {
 		return fmt.Errorf("error getting settings: %s", err)
 	}
+	if settings.ExtensionConfig == "" {
+		m.log.Infof("No extensions configured.")
+		return nil
+	}
 	err = m.UpdateExtensionRegistry(settings)
 	if err != nil {
 		return fmt.Errorf("error updating extension registry: %s", err)
@@ -683,13 +702,26 @@ func (m *Manager) CallExtension() func(http.ResponseWriter, *http.Request) {
 
 		prepareRequest(r, extName, app)
 		m.log.Debugf("proxing request for extension %q", extName)
-		proxy.ServeHTTP(w, r)
+		// httpsnoop package is used to properly wrap the responseWriter
+		// and avoid optional intefaces issue:
+		// https://github.com/felixge/httpsnoop#why-this-package-exists
+		// CaptureMetrics will call the proxy and return the metrics from it.
+		metrics := httpsnoop.CaptureMetrics(proxy, w, r)
+
+		go registerMetrics(extName, metrics, m.metricsReg)
 	}
 }
 
-// prepareRequest is reponsible for preparing and cleaning the given
-// request, removing sensitive information before forwarding it to the
-// proxy extension.
+func registerMetrics(extName string, metrics httpsnoop.Metrics, extensionMetricsRegistry ExtensionMetricsRegistry) {
+	if extensionMetricsRegistry != nil {
+		extensionMetricsRegistry.IncExtensionRequestCounter(extName, metrics.Code)
+		extensionMetricsRegistry.ObserveExtensionRequestDuration(extName, metrics.Duration)
+	}
+}
+
+// prepareRequest is reponsible for cleaning the incoming request URL removing
+// the Argo CD extension API section from it. It will set the cluster destination name
+// and cluster destination server in the headers as it is defined in the given app.
 func prepareRequest(r *http.Request, extName string, app *v1alpha1.Application) {
 	r.URL.Path = strings.TrimPrefix(r.URL.Path, fmt.Sprintf("%s/%s", URLPrefix, extName))
 	if app.Spec.Destination.Name != "" {
@@ -699,3 +731,8 @@ func prepareRequest(r *http.Request, extName string, app *v1alpha1.Application)
 		r.Header.Set(HeaderArgoCDTargetClusterURL, app.Spec.Destination.Server)
 	}
 }
+
+// AddMetricsRegistry will associate the given metricsReg in the Manager.
+func (m *Manager) AddMetricsRegistry(metricsReg ExtensionMetricsRegistry) {
+	m.metricsReg = metricsReg
+}

server/extension/extension_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"sync"
 	"testing"
 
 	"github.com/sirupsen/logrus/hooks/test"
@@ -188,10 +189,6 @@ func TestRegisterExtensions(t *testing.T) {
 			configYaml string
 		}
 		cases := []testCase{
-			{
-				name:       "no config",
-				configYaml: "",
-			},
 			{
 				name:       "no name",
 				configYaml: getExtensionConfigNoName(),
@@ -234,7 +231,7 @@ func TestRegisterExtensions(t *testing.T) {
 				err := f.manager.RegisterExtensions()
 
 				// then
-				assert.Error(t, err)
+				assert.Error(t, err, fmt.Sprintf("expected error in test %s but got nil", tc.name))
 			})
 		}
 	})
@@ -247,6 +244,7 @@ func TestCallExtension(t *testing.T) {
 		settingsGetterMock *mocks.SettingsGetter
 		rbacMock           *mocks.RbacEnforcer
 		projMock           *mocks.ProjectGetter
+		metricsMock        *mocks.ExtensionMetricsRegistry
 		manager            *extension.Manager
 	}
 	defaultProjectName := "project-name"
@@ -256,10 +254,12 @@ func TestCallExtension(t *testing.T) {
 		settMock := &mocks.SettingsGetter{}
 		rbacMock := &mocks.RbacEnforcer{}
 		projMock := &mocks.ProjectGetter{}
+		metricsMock := &mocks.ExtensionMetricsRegistry{}
 
 		logger, _ := test.NewNullLogger()
 		logEntry := logger.WithContext(context.Background())
 		m := extension.NewManager(logEntry, settMock, appMock, projMock, rbacMock)
+		m.AddMetricsRegistry(metricsMock)
 
 		mux := http.NewServeMux()
 		extHandler := http.HandlerFunc(m.CallExtension())
@@ -271,6 +271,7 @@ func TestCallExtension(t *testing.T) {
 			settingsGetterMock: settMock,
 			rbacMock:           rbacMock,
 			projMock:           projMock,
+			metricsMock:        metricsMock,
 			manager:            m,
 		}
 	}
@@ -328,6 +329,11 @@ func TestCallExtension(t *testing.T) {
 		f.projMock.On("Get", prj.GetName()).Return(prj, nil)
 	}
 
+	withMetrics := func(f *fixture) {
+		f.metricsMock.On("IncExtensionRequestCounter", mock.Anything, mock.Anything)
+		f.metricsMock.On("ObserveExtensionRequestDuration", mock.Anything, mock.Anything)
+	}
+
 	withRbac := func(f *fixture, allowApp, allowExt bool) {
 		var appAccessError error
 		var extAccessError error
@@ -406,6 +412,18 @@ func TestCallExtension(t *testing.T) {
 		proj := getProjectWithDestinations("project-name", nil, []string{clusterURL})
 		f.appGetterMock.On("Get", mock.Anything, mock.Anything).Return(app, nil)
 		withProject(proj, f)
+		var wg sync.WaitGroup
+		wg.Add(2)
+		f.metricsMock.
+			On("IncExtensionRequestCounter", mock.Anything, mock.Anything).
+			Run(func(args mock.Arguments) {
+				wg.Done()
+			})
+		f.metricsMock.
+			On("ObserveExtensionRequestDuration", mock.Anything, mock.Anything).
+			Run(func(args mock.Arguments) {
+				wg.Done()
+			})
 
 		// when
 		resp, err := http.DefaultClient.Do(r)
@@ -420,13 +438,21 @@ func TestCallExtension(t *testing.T) {
 		assert.Equal(t, backendResponse, actual)
 		assert.Equal(t, clusterURL, resp.Header.Get(extension.HeaderArgoCDTargetClusterURL))
 		assert.Equal(t, "Bearer some-bearer-token", resp.Header.Get("Authorization"))
+
+		// waitgroup is necessary to make sure assertions aren't executed before
+		// the goroutine initiated by extension.CallExtension concludes which would
+		// lead to flaky test.
+		wg.Wait()
+		f.metricsMock.AssertCalled(t, "IncExtensionRequestCounter", backendEndpoint, http.StatusOK)
+		f.metricsMock.AssertCalled(t, "ObserveExtensionRequestDuration", backendEndpoint, mock.Anything)
 	})
 	t.Run("proxy will return 404 if extension endpoint not registered", func(t *testing.T) {
 		// given
 		t.Parallel()
 		f := setup()
 		withExtensionConfig(getExtensionConfigString(), f)
 		withRbac(f, true, true)
+		withMetrics(f)
 		cluster1Name := "cluster1"
 		f.appGetterMock.On("Get", "namespace", "app-name").Return(getApp(cluster1Name, "", defaultProjectName), nil)
 		withProject(getProjectWithDestinations("project-name", []string{cluster1Name}, []string{"some-url"}), f)
@@ -466,6 +492,7 @@ func TestCallExtension(t *testing.T) {
 		withRbac(f, true, true)
 		withExtensionConfig(getExtensionConfigWith2Backends(extName, beSrv1.URL, cluster1Name, beSrv2.URL, cluster2URL), f)
 		withProject(getProjectWithDestinations("project-name", []string{cluster1Name}, []string{cluster2URL}), f)
+		withMetrics(f)
 
 		ts := startTestServer(t, f)
 		defer ts.Close()
@@ -511,6 +538,7 @@ func TestCallExtension(t *testing.T) {
 		extName := "some-extension"
 		withRbac(f, allowApp, allowExtension)
 		withExtensionConfig(getExtensionConfig(extName, "http://fake"), f)
+		withMetrics(f)
 		ts := startTestServer(t, f)
 		defer ts.Close()
 		r := newExtensionRequest(t, "Get", fmt.Sprintf("%s/extensions/%s/", ts.URL, extName))
@@ -533,6 +561,7 @@ func TestCallExtension(t *testing.T) {
 		extName := "some-extension"
 		withRbac(f, allowApp, allowExtension)
 		withExtensionConfig(getExtensionConfig(extName, "http://fake"), f)
+		withMetrics(f)
 		ts := startTestServer(t, f)
 		defer ts.Close()
 		r := newExtensionRequest(t, "Get", fmt.Sprintf("%s/extensions/%s/", ts.URL, extName))
@@ -556,6 +585,7 @@ func TestCallExtension(t *testing.T) {
 		noCluster := []string{}
 		withRbac(f, allowApp, allowExtension)
 		withExtensionConfig(getExtensionConfig(extName, "http://fake"), f)
+		withMetrics(f)
 		ts := startTestServer(t, f)
 		defer ts.Close()
 		r := newExtensionRequest(t, "Get", fmt.Sprintf("%s/extensions/%s/", ts.URL, extName))
@@ -580,6 +610,7 @@ func TestCallExtension(t *testing.T) {
 		extName := "some-extension"
 		withRbac(f, allowApp, allowExtension)
 		withExtensionConfig(getExtensionConfig(extName, "http://fake"), f)
+		withMetrics(f)
 		ts := startTestServer(t, f)
 		defer ts.Close()
 		r := newExtensionRequest(t, "Get", fmt.Sprintf("%s/extensions/%s/", ts.URL, extName))
@@ -604,6 +635,7 @@ func TestCallExtension(t *testing.T) {
 		differentProject := "differentProject"
 		withRbac(f, allowApp, allowExtension)
 		withExtensionConfig(getExtensionConfig(extName, "http://fake"), f)
+		withMetrics(f)
 		ts := startTestServer(t, f)
 		defer ts.Close()
 		r := newExtensionRequest(t, "Get", fmt.Sprintf("%s/extensions/%s/", ts.URL, extName))
@@ -634,6 +666,7 @@ func TestCallExtension(t *testing.T) {
 		withRbac(f, true, true)
 		withExtensionConfig(getExtensionConfigWith2Backends(extName, "url1", "clusterName", "url2", "clusterURL"), f)
 		withProject(getProjectWithDestinations("project-name", nil, []string{"srv1", destinationServer}), f)
+		withMetrics(f)
 
 		ts := startTestServer(t, f)
 		defer ts.Close()
@@ -666,6 +699,7 @@ func TestCallExtension(t *testing.T) {
 		differentProject := "differentProject"
 		withRbac(f, allowApp, allowExtension)
 		withExtensionConfig(getExtensionConfig(extName, "http://fake"), f)
+		withMetrics(f)
 		ts := startTestServer(t, f)
 		defer ts.Close()
 		r := newExtensionRequest(t, "Get", fmt.Sprintf("%s/extensions/", ts.URL))

server/metrics/metrics.go

@@ -14,8 +14,10 @@ import (
 
 type MetricsServer struct {
 	*http.Server
-	redisRequestCounter   *prometheus.CounterVec
-	redisRequestHistogram *prometheus.HistogramVec
+	redisRequestCounter      *prometheus.CounterVec
+	redisRequestHistogram    *prometheus.HistogramVec
+	extensionRequestCounter  *prometheus.CounterVec
+	extensionRequestDuration *prometheus.HistogramVec
 }
 
 var (
@@ -34,6 +36,21 @@ var (
 		},
 		[]string{"initiator"},
 	)
+	extensionRequestCounter = prometheus.NewCounterVec(
+		prometheus.CounterOpts{
+			Name: "argocd_proxy_extension_request_total",
+			Help: "Number of requests sent to configured proxy extensions.",
+		},
+		[]string{"extension", "status"},
+	)
+	extensionRequestDuration = prometheus.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Name:    "argocd_proxy_extension_request_duration_seconds",
+			Help:    "Request duration in seconds between the Argo CD API server and the extension backend.",
+			Buckets: []float64{0.1, 0.25, .5, 1, 2, 5, 10},
+		},
+		[]string{"extension"},
+	)
 )
 
 // NewMetricsServer returns a new prometheus server which collects api server metrics
@@ -48,14 +65,18 @@ func NewMetricsServer(host string, port int) *MetricsServer {
 
 	registry.MustRegister(redisRequestCounter)
 	registry.MustRegister(redisRequestHistogram)
+	registry.MustRegister(extensionRequestCounter)
+	registry.MustRegister(extensionRequestDuration)
 
 	return &MetricsServer{
 		Server: &http.Server{
 			Addr:    fmt.Sprintf("%s:%d", host, port),
 			Handler: mux,
 		},
-		redisRequestCounter:   redisRequestCounter,
-		redisRequestHistogram: redisRequestHistogram,
+		redisRequestCounter:      redisRequestCounter,
+		redisRequestHistogram:    redisRequestHistogram,
+		extensionRequestCounter:  extensionRequestCounter,
+		extensionRequestDuration: extensionRequestDuration,
 	}
 }
 
@@ -67,3 +88,11 @@ func (m *MetricsServer) IncRedisRequest(failed bool) {
 func (m *MetricsServer) ObserveRedisRequestDuration(duration time.Duration) {
 	m.redisRequestHistogram.WithLabelValues("argocd-server").Observe(duration.Seconds())
 }
+
+func (m *MetricsServer) IncExtensionRequestCounter(extension string, status int) {
+	m.extensionRequestCounter.WithLabelValues(extension, strconv.Itoa(status)).Inc()
+}
+
+func (m *MetricsServer) ObserveExtensionRequestDuration(extension string, duration time.Duration) {
+	m.extensionRequestDuration.WithLabelValues(extension).Observe(duration.Seconds())
+}