Merge branch 'id-metrics-for-results' into 'main'

Add additional metrics to gitlab-sshd

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

4 files changed, 32 insertions, 9 deletions

diff --git a/internal/sshd/connection.go b/internal/sshd/connection.go
index 25b082a..1312833 100644
--- a/internal/sshd/connection.go
+++ b/internal/sshd/connection.go
@@ -2,7 +2,6 @@ package sshd
 
 import (
 	"context"
-	"time"
 
 	"golang.org/x/crypto/ssh"
 	"golang.org/x/sync/semaphore"
@@ -29,13 +28,6 @@ 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 func(started time.Time) {
-		metrics.SshdConnectionsInFlight.Dec()
-		metrics.SshdConnectionDuration.Observe(time.Since(started).Seconds())
-	}(time.Now())
-
 	for newChannel := range chans {
 		ctxlog.WithField("channel_type", newChannel.ChannelType()).Info("connection: handle: new channel requested")
 		if newChannel.ChannelType() != "session" {
diff --git a/internal/sshd/session.go b/internal/sshd/session.go
index ff8540b..beb529e 100644
--- a/internal/sshd/session.go
+++ b/internal/sshd/session.go
@@ -22,6 +22,7 @@ type session struct {
 	channel     ssh.Channel
 	gitlabKeyId string
 	remoteAddr  string
+	success     bool
 
 	// State managed by the session
 	execCmd            string
@@ -182,6 +183,8 @@ func (s *session) exit(ctx context.Context, status uint32) {
 	log.WithContextFields(ctx, log.Fields{"exit_status": status}).Info("session: exit: exiting")
 	req := exitStatusReq{ExitStatus: status}
 
+	s.success = status == 0
+
 	s.channel.CloseWrite()
 	s.channel.SendRequest("exit-status", false, ssh.Marshal(req))
 }
diff --git a/internal/sshd/session_test.go b/internal/sshd/session_test.go
index f135825..d0cc8d4 100644
--- a/internal/sshd/session_test.go
+++ b/internal/sshd/session_test.go
@@ -99,6 +99,7 @@ func TestHandleExec(t *testing.T) {
 		expectedExecCmd    string
 		sentRequestName    string
 		sentRequestPayload []byte
+		success            bool
 	}{
 		{
 			desc:            "invalid payload",
@@ -111,6 +112,7 @@ func TestHandleExec(t *testing.T) {
 			expectedExecCmd:    "discover",
 			sentRequestName:    "exit-status",
 			sentRequestPayload: ssh.Marshal(exitStatusReq{ExitStatus: 0}),
+			success:            true,
 		},
 	}
 
@@ -130,6 +132,7 @@ func TestHandleExec(t *testing.T) {
 			require.Equal(t, false, s.handleExec(context.Background(), r))
 			require.Equal(t, tc.sentRequestName, f.sentRequestName)
 			require.Equal(t, tc.sentRequestPayload, f.sentRequestPayload)
+			require.Equal(t, tc.success, s.success)
 		})
 	}
 }
@@ -141,6 +144,7 @@ func TestHandleShell(t *testing.T) {
 		errMsg           string
 		gitlabKeyId      string
 		expectedExitCode uint32
+		success          bool
 	}{
 		{
 			desc:             "fails to parse command",
diff --git a/internal/sshd/sshd.go b/internal/sshd/sshd.go
index b661233..8097e9b 100644
--- a/internal/sshd/sshd.go
+++ b/internal/sshd/sshd.go
@@ -12,6 +12,7 @@ import (
 	"golang.org/x/crypto/ssh"
 
 	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/metrics"
 
 	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/log"
@@ -145,6 +146,20 @@ func (s *Server) getStatus() status {
 }
 
 func (s *Server) handleConn(ctx context.Context, nconn net.Conn) {
+	success := false
+
+	metrics.SshdConnectionsInFlight.Inc()
+	started := time.Now()
+	defer func() {
+		metrics.SshdConnectionsInFlight.Dec()
+		metrics.SshdSessionDuration.Observe(time.Since(started).Seconds())
+
+		metrics.SliSshdSessionsTotal.Inc()
+		if !success {
+			metrics.SliSshdSessionsErrorsTotal.Inc()
+		}
+	}()
+
 	remoteAddr := nconn.RemoteAddr().String()
 
 	defer s.wg.Done()
@@ -172,8 +187,12 @@ func (s *Server) handleConn(ctx context.Context, nconn net.Conn) {
 
 	go ssh.DiscardRequests(reqs)
 
+	var establishSessionDuration float64
 	conn := newConnection(s.Config.Server.ConcurrentSessionsLimit, remoteAddr)
 	conn.handle(ctx, chans, func(ctx context.Context, channel ssh.Channel, requests <-chan *ssh.Request) {
+		establishSessionDuration = time.Since(started).Seconds()
+		metrics.SshdSessionEstablishedDuration.Observe(establishSessionDuration)
+
 		session := &session{
 			cfg:         s.Config,
 			channel:     channel,
@@ -182,9 +201,14 @@ func (s *Server) handleConn(ctx context.Context, nconn net.Conn) {
 		}
 
 		session.handle(ctx, requests)
+
+		success = session.success
 	})
 
-	ctxlog.Info("server: handleConn: done")
+	ctxlog.WithFields(log.Fields{
+		"duration_s":                   time.Since(started).Seconds(),
+		"establish_session_duration_s": establishSessionDuration,
+	}).Info("server: handleConn: done")
 }
 
 func (s *Server) initSSHConnection(ctx context.Context, nconn net.Conn) (sconn *ssh.ServerConn, chans <-chan ssh.NewChannel, reqs <-chan *ssh.Request, err error) {