Merge branch 'git-archive-prom' into 'master'

Prometheus metrics for senddata and git archive cache

See merge request !163

internal/git/archive.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"log"
 	"net/http"
 	"os"
 	"os/exec"
@@ -19,6 +18,8 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/senddata"
+
+	"github.com/prometheus/client_golang/prometheus"
 )
 type archive struct{ senddata.Prefix }
@@ -29,7 +30,20 @@ type archiveParams struct {
 	CommitId      string
 }
-var SendArchive = &archive{"git-archive:"}
+var (
+	SendArchive     = &archive{"git-archive:"}
+	gitArchiveCache = prometheus.NewCounterVec(
+		prometheus.CounterOpts{
+			Name: "gitlab_workhorse_git_archive_cache",
+			Help: "Cache hits and misses for 'git archive' streaming",
+		},
+		[]string{"result"},
+	)
+)
+
+func init() {
+	prometheus.MustRegister(gitArchiveCache)
+}
 func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	var params archiveParams
@@ -50,7 +64,7 @@ func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string
 	if cachedArchive, err := os.Open(params.ArchivePath); err == nil {
 		defer cachedArchive.Close()
-		log.Printf("Serving cached file %q", params.ArchivePath)
+		gitArchiveCache.WithLabelValues("hit").Inc()
 		setArchiveHeaders(w, format, archiveFilename)
 		// Even if somebody deleted the cachedArchive from disk since we opened
 		// the file, Unix file semantics guarantee we can still read from the
@@ -59,6 +73,8 @@ func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string
 		return
 	}
+	gitArchiveCache.WithLabelValues("miss").Inc()
+
 	// We assume the tempFile has a unique name so that concurrent requests are
 	// safe. We create the tempfile in the same directory as the final cached
 	// archive we want to create so that we can use an atomic link(2) operation

internal/git/responsewriter.go

@@ -4,6 +4,8 @@ import (
 	"net/http"
 	"strconv"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
+
 	"github.com/prometheus/client_golang/prometheus"
 )
@@ -42,39 +44,14 @@ func init() {
 }
 type GitHttpResponseWriter struct {
-	rw      http.ResponseWriter
-	status  int
-	written int64
+	helper.CountingResponseWriter
 }
 func NewGitHttpResponseWriter(rw http.ResponseWriter) *GitHttpResponseWriter {
 	gitHTTPSessionsActive.Inc()
 	return &GitHttpResponseWriter{
-		rw: rw,
-	}
-}
-
-func (w *GitHttpResponseWriter) Header() http.Header {
-	return w.rw.Header()
-}
-
-func (w *GitHttpResponseWriter) Write(data []byte) (n int, err error) {
-	if w.status == 0 {
-		w.WriteHeader(http.StatusOK)
+		CountingResponseWriter: helper.NewCountingResponseWriter(rw),
 	}
-
-	n, err = w.rw.Write(data)
-	w.written += int64(n)
-	return n, err
-}
-
-func (w *GitHttpResponseWriter) WriteHeader(status int) {
-	if w.status != 0 {
-		return
-	}
-
-	w.status = status
-	w.rw.WriteHeader(status)
 }
 func (w *GitHttpResponseWriter) Log(r *http.Request, writtenIn int64) {
@@ -82,11 +59,11 @@ func (w *GitHttpResponseWriter) Log(r *http.Request, writtenIn int64) {
 	agent := getRequestAgent(r)
 	gitHTTPSessionsActive.Dec()
-	gitHTTPRequests.WithLabelValues(r.Method, strconv.Itoa(w.status), service, agent).Inc()
-	gitHTTPBytes.WithLabelValues(r.Method, strconv.Itoa(w.status), service, agent, directionIn).
+	gitHTTPRequests.WithLabelValues(r.Method, strconv.Itoa(w.Status()), service, agent).Inc()
+	gitHTTPBytes.WithLabelValues(r.Method, strconv.Itoa(w.Status()), service, agent, directionIn).
 		Add(float64(writtenIn))
-	gitHTTPBytes.WithLabelValues(r.Method, strconv.Itoa(w.status), service, agent, directionOut).
-		Add(float64(w.written))
+	gitHTTPBytes.WithLabelValues(r.Method, strconv.Itoa(w.Status()), service, agent, directionOut).
+		Add(float64(w.Count()))
 }
 func getRequestAgent(r *http.Request) string {

internal/helper/countingresponsewriter.go

+package helper
+
+import (
+	"net/http"
+)
+
+type CountingResponseWriter interface {
+	http.ResponseWriter
+	Count() int64
+	Status() int
+}
+
+type countingResponseWriter struct {
+	rw     http.ResponseWriter
+	status int
+	count  int64
+}
+
+func NewCountingResponseWriter(rw http.ResponseWriter) CountingResponseWriter {
+	return &countingResponseWriter{rw: rw}
+}
+
+func (c *countingResponseWriter) Header() http.Header {
+	return c.rw.Header()
+}
+
+func (c *countingResponseWriter) Write(data []byte) (n int, err error) {
+	if c.status == 0 {
+		c.WriteHeader(http.StatusOK)
+	}
+
+	n, err = c.rw.Write(data)
+	c.count += int64(n)
+	return n, err
+}
+
+func (c *countingResponseWriter) WriteHeader(status int) {
+	if c.status != 0 {
+		return
+	}
+
+	c.status = status
+	c.rw.WriteHeader(status)
+}
+
+// Count returns the number of bytes written to the ResponseWriter. This
+// function is not thread-safe.
+func (c *countingResponseWriter) Count() int64 {
+	return c.count
+}
+
+// Status returns the first HTTP status value that was written to the
+// ResponseWriter. This function is not thread-safe.
+func (c *countingResponseWriter) Status() int {
+	return c.status
+}

internal/helper/countingresponsewriter_test.go

+package helper
+
+import (
+	"bytes"
+	"io"
+	"net/http"
+	"testing"
+	"testing/iotest"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type testResponseWriter struct {
+	data []byte
+}
+
+func (*testResponseWriter) WriteHeader(int)     {}
+func (*testResponseWriter) Header() http.Header { return nil }
+
+func (trw *testResponseWriter) Write(p []byte) (int, error) {
+	trw.data = append(trw.data, p...)
+	return len(p), nil
+}
+
+func TestCountingResponseWriterStatus(t *testing.T) {
+	crw := NewCountingResponseWriter(&testResponseWriter{})
+	crw.WriteHeader(123)
+	crw.WriteHeader(456)
+	assert.Equal(t, 123, crw.Status())
+}
+
+func TestCountingResponseWriterCount(t *testing.T) {
+	crw := NewCountingResponseWriter(&testResponseWriter{})
+	for _, n := range []int{1, 2, 4, 8, 16, 32} {
+		_, err := crw.Write(bytes.Repeat([]byte{'.'}, n))
+		require.NoError(t, err)
+	}
+	assert.Equal(t, int64(63), crw.Count())
+}
+
+func TestCountingResponseWriterWrite(t *testing.T) {
+	trw := &testResponseWriter{}
+	crw := NewCountingResponseWriter(trw)
+
+	testData := []byte("test data")
+	_, err := io.Copy(crw, iotest.OneByteReader(bytes.NewReader(testData)))
+	require.NoError(t, err)
+
+	assert.Equal(t, string(testData), string(trw.data))
+}

internal/senddata/injecter.go

@@ -10,6 +10,7 @@ import (
 type Injecter interface {
 	Match(string) bool
 	Inject(http.ResponseWriter, *http.Request, string)
+	Name() string
 }
 type Prefix string
@@ -30,3 +31,7 @@ func (p Prefix) Unpack(result interface{}, sendData string) error {
 	}
 	return nil
 }
+
+func (p Prefix) Name() string {
+	return strings.TrimSuffix(string(p), ":")
+}

internal/senddata/senddata.go

@@ -4,8 +4,32 @@ import (
 	"net/http"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
+
+	"github.com/prometheus/client_golang/prometheus"
+)
+
+var (
+	sendDataResponses = prometheus.NewCounterVec(
+		prometheus.CounterOpts{
+			Name: "gitlab_workhorse_senddata_responses",
+			Help: "How many HTTP responses have been hijacked by a workhorse senddata injecter",
+		},
+		[]string{"injecter"},
+	)
+	sendDataResponseBytes = prometheus.NewCounterVec(
+		prometheus.CounterOpts{
+			Name: "gitlab_workhorse_senddata_response_bytes",
+			Help: "How many bytes have been written by workhorse senddata response injecters",
+		},
+		[]string{"injecter"},
+	)
 )
+func init() {
+	prometheus.MustRegister(sendDataResponses)
+	prometheus.MustRegister(sendDataResponseBytes)
+}
+
 type sendDataResponseWriter struct {
 	rw        http.ResponseWriter
 	status    int
@@ -65,7 +89,10 @@ func (s *sendDataResponseWriter) tryInject() bool {
 		if injecter.Match(header) {
 			s.hijacked = true
 			helper.DisableResponseBuffering(s.rw)
-			injecter.Inject(s.rw, s.req, header)
+			crw := helper.NewCountingResponseWriter(s.rw)
+			injecter.Inject(crw, s.req, header)
+			sendDataResponses.WithLabelValues(injecter.Name()).Inc()
+			sendDataResponseBytes.WithLabelValues(injecter.Name()).Add(float64(crw.Count()))
 			return true
 		}
 	}