Merge branch 'id-concurrent-connections-prometheus' into 'main'

Add more metrics for gitlab-sshd

See merge request gitlab-org/gitlab-shell!574

4 files changed, 84 insertions, 26 deletions

diff --git a/internal/config/config.go b/internal/config/config.go
index 5185736..efce9f9 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -9,7 +9,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/prometheus/client_golang/prometheus/promhttp"
 	yaml "gopkg.in/yaml.v2"
 
 	"gitlab.com/gitlab-org/gitlab-shell/client"
@@ -114,7 +113,7 @@ func (c *Config) HttpClient() (*client.HttpClient, error) {
 		}
 
 		tr := client.Transport
-		client.Transport = promhttp.InstrumentRoundTripperDuration(metrics.HttpRequestDuration, tr)
+		client.Transport = metrics.NewRoundTripper(tr)
 
 		c.httpClient = client
 	})
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 78b2ed4..e422c4e 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -25,7 +25,7 @@ func TestConfigApplyGlobalState(t *testing.T) {
 	require.Equal(t, "foo", os.Getenv("SSL_CERT_DIR"))
 }
 
-func TestHttpClient(t *testing.T) {
+func TestCustomPrometheusMetrics(t *testing.T) {
 	url := testserver.StartHttpServer(t, []testserver.TestRequestHandler{})
 
 	config := &Config{GitlabUrl: url}
@@ -38,14 +38,19 @@ func TestHttpClient(t *testing.T) {
 	ms, err := prometheus.DefaultGatherer.Gather()
 	require.NoError(t, err)
 
-	lastMetric := ms[0]
-	require.Equal(t, lastMetric.GetName(), "gitlab_shell_http_request_seconds")
-
-	labels := lastMetric.GetMetric()[0].Label
-
-	require.Equal(t, "code", labels[0].GetName())
-	require.Equal(t, "404", labels[0].GetValue())
-
-	require.Equal(t, "method", labels[1].GetName())
-	require.Equal(t, "get", labels[1].GetValue())
+	var actualNames []string
+	for _, m := range ms[0:6] {
+		actualNames = append(actualNames, m.GetName())
+	}
+
+	expectedMetricNames := []string{
+		"gitlab_shell_http_in_flight_requests",
+		"gitlab_shell_http_request_duration_seconds",
+		"gitlab_shell_http_requests_total",
+		"gitlab_shell_sshd_concurrent_limited_sessions_total",
+		"gitlab_shell_sshd_connection_duration_seconds",
+		"gitlab_shell_sshd_in_flight_connections",
+	}
+
+	require.Equal(t, expectedMetricNames, actualNames)
 }
diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go
index 6174bca..c46fb8f 100644
--- a/internal/metrics/metrics.go
+++ b/internal/metrics/metrics.go
@@ -1,14 +1,25 @@
 package metrics
 
 import (
+	"net/http"
+
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
+	"github.com/prometheus/client_golang/prometheus/promhttp"
 )
 
 const (
 	namespace     = "gitlab_shell"
 	sshdSubsystem = "sshd"
 	httpSubsystem = "http"
+
+	httpInFlightRequestsMetricName       = "in_flight_requests"
+	httpRequestsTotalMetricName          = "requests_total"
+	httpRequestDurationSecondsMetricName = "request_duration_seconds"
+
+	sshdConnectionsInFlightName = "in_flight_connections"
+	sshdConnectionDuration      = "connection_duration_seconds"
+	sshdHitMaxSessions          = "concurrent_limited_sessions_total"
 )
 
 var (
@@ -16,7 +27,7 @@ var (
 		prometheus.HistogramOpts{
 			Namespace: namespace,
 			Subsystem: sshdSubsystem,
-			Name:      "connection_duration_seconds",
+			Name:      sshdConnectionDuration,
 			Help:      "A histogram of latencies for connections to gitlab-shell sshd.",
 			Buckets: []float64{
 				0.005, /* 5ms */
@@ -32,32 +43,72 @@ var (
 		},
 	)
 
+	SshdConnectionsInFlight = promauto.NewGauge(
+		prometheus.GaugeOpts{
+			Namespace: namespace,
+			Subsystem: sshdSubsystem,
+			Name:      sshdConnectionsInFlightName,
+			Help:      "A gauge of connections currently being served by gitlab-shell sshd.",
+		},
+	)
+
 	SshdHitMaxSessions = promauto.NewCounter(
 		prometheus.CounterOpts{
 			Namespace: namespace,
 			Subsystem: sshdSubsystem,
-			Name:      "concurrent_limited_sessions_total",
+			Name:      sshdHitMaxSessions,
 			Help:      "The number of times the concurrent sessions limit was hit in gitlab-shell sshd.",
 		},
 	)
 
-	HttpRequestDuration = promauto.NewHistogramVec(
+	// The metrics and the buckets size are similar to the ones we have for handlers in Labkit
+	// When the MR: https://gitlab.com/gitlab-org/labkit/-/merge_requests/150 is merged,
+	// these metrics can be refactored out of Gitlab Shell code by using the helper function from Labkit
+	httpRequestsTotal = promauto.NewCounterVec(
+		prometheus.CounterOpts{
+			Namespace: namespace,
+			Subsystem: httpSubsystem,
+			Name:      httpRequestsTotalMetricName,
+			Help:      "A counter for http requests.",
+		},
+		[]string{"code", "method"},
+	)
+
+	httpRequestDurationSeconds = promauto.NewHistogramVec(
 		prometheus.HistogramOpts{
 			Namespace: namespace,
 			Subsystem: httpSubsystem,
-			Name:      "request_seconds",
-			Help:      "A histogram of latencies for gitlab-shell http requests",
+			Name:      httpRequestDurationSecondsMetricName,
+			Help:      "A histogram of latencies for http requests.",
 			Buckets: []float64{
-				0.01, /* 10ms */
-				0.05, /* 50ms */
-				0.1,  /* 100ms */
-				0.25, /* 250ms */
-				0.5,  /* 500ms */
-				1.0,  /* 1s */
-				5.0,  /* 5s */
-				10.0, /* 10s */
+				0.005, /* 5ms */
+				0.025, /* 25ms */
+				0.1,   /* 100ms */
+				0.5,   /* 500ms */
+				1.0,   /* 1s */
+				10.0,  /* 10s */
+				30.0,  /* 30s */
+				60.0,  /* 1m */
+				300.0, /* 5m */
 			},
 		},
 		[]string{"code", "method"},
 	)
+
+	httpInFlightRequests = promauto.NewGauge(
+		prometheus.GaugeOpts{
+			Namespace: namespace,
+			Subsystem: httpSubsystem,
+			Name:      httpInFlightRequestsMetricName,
+			Help:      "A gauge of requests currently being performed.",
+		},
+	)
 )
+
+func NewRoundTripper(next http.RoundTripper) promhttp.RoundTripperFunc {
+	rt := next
+
+	rt = promhttp.InstrumentRoundTripperCounter(httpRequestsTotal, rt)
+	rt = promhttp.InstrumentRoundTripperDuration(httpRequestDurationSeconds, rt)
+	return promhttp.InstrumentRoundTripperInFlight(httpInFlightRequests, rt)
+}
diff --git a/internal/sshd/connection.go b/internal/sshd/connection.go
index f6d8fb5..7ba9bed 100644
--- a/internal/sshd/connection.go
+++ b/internal/sshd/connection.go
@@ -31,6 +31,9 @@ func newConnection(maxSessions int64, remoteAddr string) *connection {
 func (c *connection) handle(ctx context.Context, chans <-chan ssh.NewChannel, handler channelHandler) {
 	ctxlog := log.WithContextFields(ctx, log.Fields{"remote_addr": c.remoteAddr})
 
+	metrics.SshdConnectionsInFlight.Inc()
+	defer metrics.SshdConnectionsInFlight.Dec()
+
 	defer metrics.SshdConnectionDuration.Observe(time.Since(c.begin).Seconds())
 
 	for newChannel := range chans {