From 98c3e01dc59b1a1374f648e0f4c2e599f20657d4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 3 Feb 2020 14:18:14 +0100 Subject: Add serverless serving metrics This adds: - serverless invocations counter - serverless request latency histogram --- internal/serving/serverless/serverless.go | 3 +++ internal/serving/serverless/transport.go | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) (limited to 'internal') diff --git a/internal/serving/serverless/serverless.go b/internal/serving/serverless/serverless.go index a8d090da..a73affeb 100644 --- a/internal/serving/serverless/serverless.go +++ b/internal/serving/serverless/serverless.go @@ -5,6 +5,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" + "gitlab.com/gitlab-org/gitlab-pages/metrics" ) // Serverless is a servering used to proxy requests between a client and @@ -26,6 +27,8 @@ func New(function Function, cluster Cluster) serving.Serving { // ServeFileHTTP handle an incoming request and proxies it to Knative cluster func (s *Serverless) ServeFileHTTP(h serving.Handler) bool { + metrics.ServerlessRequests.Inc() + s.proxy.ServeHTTP(h.Writer, h.Request) return true diff --git a/internal/serving/serverless/transport.go b/internal/serving/serverless/transport.go index 5a0f5165..b7fabb13 100644 --- a/internal/serving/serverless/transport.go +++ b/internal/serving/serverless/transport.go @@ -5,6 +5,8 @@ import ( "net" "net/http" "time" + + "gitlab.com/gitlab-org/gitlab-pages/metrics" ) // Transport is a struct that handle the proxy connection round trip to Knative @@ -39,8 +41,11 @@ func NewTransport(cluster Cluster) *Transport { // RoundTrip performs a connection to a Knative cluster and returns a response func (t *Transport) RoundTrip(request *http.Request) (*http.Response, error) { + start := time.Now() + response, err := t.transport.RoundTrip(request) - // TODO add prometheus metrics for round trip timing + metrics.ServerlessLatency.Observe(time.Since(start).Seconds()) + return response, err } -- cgit v1.2.3 From f6bb22c68b2dfb5b5162335b6ffda813b79e8426 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 3 Feb 2020 15:58:19 +0000 Subject: use gorilla/handlers.ProxyHeaders to get the X-Forwarded-* headers and set them in the appropriate http.Request fields --- internal/request/request.go | 25 ++++++++++++++-- internal/request/request_test.go | 57 +++++++++++++++++++++++++++++++++++++ internal/testhelpers/testhelpers.go | 16 +++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) (limited to 'internal') diff --git a/internal/request/request.go b/internal/request/request.go index 06a45071..ba56f45b 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -5,6 +5,8 @@ import ( "net" "net/http" + log "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" ) @@ -14,6 +16,11 @@ const ( ctxHTTPSKey ctxKey = "https" ctxHostKey ctxKey = "host" ctxDomainKey ctxKey = "domain" + + // SchemeHTTP name for the HTTP scheme + SchemeHTTP = "http" + // SchemeHTTPS name for the HTTPS scheme + SchemeHTTPS = "https" ) // WithHTTPSFlag saves https flag in request's context @@ -23,9 +30,23 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request { return r.WithContext(ctx) } -// IsHTTPS restores https flag from request's context +// IsHTTPS checks whether the request originated from HTTP or HTTPS. +// It reads the ctxHTTPSKey from the context and returns its value +// It also checks that r.URL.Scheme matches the value in ctxHTTPSKey for HTTPS requests +// TODO: remove the ctxHTTPSKey from the context https://gitlab.com/gitlab-org/gitlab-pages/issues/219 func IsHTTPS(r *http.Request) bool { - return r.Context().Value(ctxHTTPSKey).(bool) + https := r.Context().Value(ctxHTTPSKey).(bool) + + if https != (r.URL.Scheme == SchemeHTTPS) { + log.WithFields(log.Fields{ + "ctxHTTPSKey": https, + "scheme": r.URL.Scheme, + }).Warn("request: r.URL.Scheme does not match value in ctxHTTPSKey") + } + + // Returning the value of ctxHTTPSKey for now, can just switch to r.URL.Scheme == SchemeHTTPS later + // and later can remove IsHTTPS method altogether + return https } // WithHostAndDomain saves host name and domain in the request's context diff --git a/internal/request/request_test.go b/internal/request/request_test.go index 642209f0..3c0f970c 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -5,9 +5,11 @@ import ( "net/http/httptest" "testing" + "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) func TestWithHTTPSFlag(t *testing.T) { @@ -15,10 +17,65 @@ func TestWithHTTPSFlag(t *testing.T) { require.NoError(t, err) httpsRequest := WithHTTPSFlag(r, true) + httpsRequest.URL.Scheme = SchemeHTTPS require.True(t, IsHTTPS(httpsRequest)) httpRequest := WithHTTPSFlag(r, false) + httpsRequest.URL.Scheme = SchemeHTTP require.False(t, IsHTTPS(httpRequest)) + +} + +func TestIsHTTPS(t *testing.T) { + hook := test.NewGlobal() + + tests := []struct { + name string + flag bool + scheme string + wantLogEntries string + }{ + { + name: "https", + flag: true, + scheme: "https", + }, + { + name: "http", + flag: false, + scheme: "http", + }, + { + name: "scheme true but flag is false", + flag: false, + scheme: "https", + wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey", + }, + { + name: "scheme false but flag is true", + flag: true, + scheme: "http", + wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hook.Reset() + + r, err := http.NewRequest("GET", "/", nil) + require.NoError(t, err) + r.URL.Scheme = tt.scheme + + httpsRequest := WithHTTPSFlag(r, tt.flag) + + got := IsHTTPS(httpsRequest) + require.Equal(t, tt.flag, got) + + testhelpers.AssertLogContains(t, tt.wantLogEntries, hook.AllEntries()) + }) + } + } func TestPanics(t *testing.T) { diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index c0b87d93..d703769b 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -7,6 +7,7 @@ import ( "net/url" "testing" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" ) @@ -40,4 +41,19 @@ func AssertRedirectTo(t *testing.T, handler http.HandlerFunc, method string, handler(recorder, req) require.Equal(t, expectedURL, recorder.Header().Get("Location")) + +} + +// AssertLogContains checks that wantLogEntry is contained in at least one of the log entries +func AssertLogContains(t *testing.T, wantLogEntry string, entries []*logrus.Entry) { + t.Helper() + + if wantLogEntry != "" { + messages := make([]string, len(entries)) + for k, entry := range entries { + messages[k] = entry.Message + } + + require.Contains(t, messages, wantLogEntry) + } } -- cgit v1.2.3 From e04a2bb8752a31988d5d255932a9b2887a01ce4a Mon Sep 17 00:00:00 2001 From: Alessio Caiazza Date: Wed, 5 Feb 2020 15:18:27 +0100 Subject: Fix benchmarks Go benchmarks must loop over b.N --- internal/source/disk/map_test.go | 54 +++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 23 deletions(-) (limited to 'internal') diff --git a/internal/source/disk/map_test.go b/internal/source/disk/map_test.go index c15f29c6..d904e6aa 100644 --- a/internal/source/disk/map_test.go +++ b/internal/source/disk/map_test.go @@ -13,15 +13,7 @@ import ( "github.com/stretchr/testify/require" ) -func getEntries(t *testing.T) godirwalk.Dirents { - fis, err := godirwalk.ReadDirents(".", nil) - - require.NoError(t, err) - - return fis -} - -func getEntriesForBenchmark(t *testing.B) godirwalk.Dirents { +func getEntries(t require.TestingT) godirwalk.Dirents { fis, err := godirwalk.ReadDirents(".", nil) require.NoError(t, err) @@ -199,7 +191,7 @@ func buildFakeDomainsDirectory(t require.TestingT, nGroups, levels int) func() { domain = fmt.Sprintf("%d.%s", j, domain) buildFakeProjectsDirectory(t, parent, domain) } - if i%100 == 0 { + if testing.Verbose() && i%100 == 0 { fmt.Print(".") } } @@ -208,7 +200,11 @@ func buildFakeDomainsDirectory(t require.TestingT, nGroups, levels int) func() { return func() { defer cleanup() - fmt.Printf("cleaning up test directory %s\n", testRoot) + + if testing.Verbose() { + fmt.Printf("cleaning up test directory %s\n", testRoot) + } + os.RemoveAll(testRoot) } } @@ -223,18 +219,30 @@ func buildFakeProjectsDirectory(t require.TestingT, groupPath, domain string) { } } -func BenchmarkReadGroups(b *testing.B) { - nGroups := 10000 - b.Logf("creating fake domains directory with %d groups", nGroups) - cleanup := buildFakeDomainsDirectory(b, nGroups, 0) +// this is a safeguard against compiler optimizations +// we use this package variable to make sure the benchmarkReadGroups loop +// has side effects outside of the loop. +// Without this the compiler (with the optimizations enabled) may remove the whole loop +var result int + +func benchmarkReadGroups(b *testing.B, groups, levels int) { + cleanup := buildFakeDomainsDirectory(b, groups, levels) defer cleanup() - b.Run("ReadGroups", func(b *testing.B) { - var dm Map - for i := 0; i < 2; i++ { - dm = make(Map) - dm.ReadGroups("example.com", getEntriesForBenchmark(b)) - } - b.Logf("found %d domains", len(dm)) - }) + b.ResetTimer() + + domainsCnt := 0 + for i := 0; i < b.N; i++ { + dm := make(Map) + dm.ReadGroups("example.com", getEntries(b)) + domainsCnt = len(dm) + } + result = domainsCnt +} + +func BenchmarkReadGroups(b *testing.B) { + b.Run("10 groups 3 levels", func(b *testing.B) { benchmarkReadGroups(b, 10, 3) }) + b.Run("100 groups 3 levels", func(b *testing.B) { benchmarkReadGroups(b, 100, 3) }) + b.Run("1000 groups 3 levels", func(b *testing.B) { benchmarkReadGroups(b, 1000, 3) }) + b.Run("10000 groups 1 levels", func(b *testing.B) { benchmarkReadGroups(b, 10000, 1) }) } -- cgit v1.2.3 From 2b0661b4ff4fe112798f03f62861096104b9e373 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 11 Feb 2020 08:39:25 +0000 Subject: Add prometheus metrics for GitLab API client Refactor metrics initialization removing init function from the metrics package. --- internal/httptransport/transport.go | 39 +++++++++++++- internal/httptransport/transport_test.go | 91 ++++++++++++++++++++++++++++++++ internal/source/disk/map.go | 2 +- internal/source/gitlab/client/client.go | 11 ++-- 4 files changed, 137 insertions(+), 6 deletions(-) create mode 100644 internal/httptransport/transport_test.go (limited to 'internal') diff --git a/internal/httptransport/transport.go b/internal/httptransport/transport.go index 6d946ae0..ba2aa5eb 100644 --- a/internal/httptransport/transport.go +++ b/internal/httptransport/transport.go @@ -7,8 +7,11 @@ import ( "net" "net/http" "os" + "strconv" "sync" + "time" + "github.com/prometheus/client_golang/prometheus" log "github.com/sirupsen/logrus" ) @@ -16,7 +19,7 @@ var ( sysPoolOnce = &sync.Once{} sysPool *x509.CertPool - // Transport can be used with httpclient with TLS and certificates + // Transport can be used with http.Client with TLS and certificates Transport = &http.Transport{ DialTLS: func(network, addr string) (net.Conn, error) { return tls.Dial(network, addr, &tls.Config{RootCAs: pool()}) @@ -25,6 +28,22 @@ var ( } ) +type meteredRoundTripper struct { + next http.RoundTripper + durations *prometheus.GaugeVec + counter *prometheus.CounterVec +} + +// NewTransportWithMetrics will create a custom http.RoundTripper that can be used with an http.Client. +// The RoundTripper will report metrics based on the collectors passed. +func NewTransportWithMetrics(gaugeVec *prometheus.GaugeVec, counterVec *prometheus.CounterVec) http.RoundTripper { + return &meteredRoundTripper{ + next: Transport, + durations: gaugeVec, + counter: counterVec, + } +} + // This is here because macOS does not support the SSL_CERT_FILE // environment variable. We have arrange things to read SSL_CERT_FILE as // late as possible to avoid conflicts with file descriptor passing at @@ -55,3 +74,21 @@ func loadPool() { sysPool.AppendCertsFromPEM(certPem) } + +// withRoundTripper takes an original RoundTripper, reports metrics based on the +// gauge and counter collectors passed +func (mrt *meteredRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + start := time.Now() + + resp, err := mrt.next.RoundTrip(r) + if err != nil { + mrt.counter.WithLabelValues("error").Inc() + return nil, err + } + + statusCode := strconv.Itoa(resp.StatusCode) + mrt.durations.WithLabelValues(statusCode).Set(time.Since(start).Seconds()) + mrt.counter.WithLabelValues(statusCode).Inc() + + return resp, nil +} diff --git a/internal/httptransport/transport_test.go b/internal/httptransport/transport_test.go new file mode 100644 index 00000000..cfb8d708 --- /dev/null +++ b/internal/httptransport/transport_test.go @@ -0,0 +1,91 @@ +package httptransport + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/stretchr/testify/require" +) + +func Test_withRoundTripper(t *testing.T) { + + tests := []struct { + name string + statusCode int + err error + }{ + { + name: "successful_response", + statusCode: http.StatusNoContent, + }, + { + name: "error_response", + statusCode: http.StatusForbidden, + }, + { + name: "internal_error_response", + statusCode: http.StatusInternalServerError, + }, + { + name: "unhandled_status_response", + statusCode: http.StatusPermanentRedirect, + }, + { + name: "client_error", + err: fmt.Errorf("something went wrong"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gaugeVec := prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Name: t.Name(), + }, []string{"status_code"}) + + counterVec := prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: t.Name(), + }, []string{"status_code"}) + + next := &mockRoundTripper{ + res: &http.Response{ + StatusCode: tt.statusCode, + }, + err: tt.err, + } + + mtr := &meteredRoundTripper{next, gaugeVec, counterVec} + r := httptest.NewRequest("GET", "/", nil) + + res, err := mtr.RoundTrip(r) + if tt.err != nil { + counterCount := testutil.ToFloat64(counterVec.WithLabelValues("error")) + require.Equal(t, float64(1), counterCount, "error") + + return + } + require.NoError(t, err) + require.NotNil(t, res) + + statusCode := strconv.Itoa(res.StatusCode) + gaugeValue := testutil.ToFloat64(gaugeVec.WithLabelValues(statusCode)) + require.Greater(t, gaugeValue, float64(0)) + + counterCount := testutil.ToFloat64(counterVec.WithLabelValues(statusCode)) + require.Equal(t, float64(1), counterCount, statusCode) + }) + } +} + +type mockRoundTripper struct { + res *http.Response + err error +} + +func (mrt *mockRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + return mrt.res, mrt.err +} diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index bae4b764..5c053fcb 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -252,7 +252,7 @@ func Watch(rootDomain string, updater domainsUpdater, interval time.Duration) { fis, err := godirwalk.ReadDirents(".", nil) if err != nil { log.WithError(err).Warn("domain scan failed") - metrics.FailedDomainUpdates.Inc() + metrics.DomainFailedUpdates.Inc() continue } diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index 59776a8f..afe9da6f 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -11,10 +11,11 @@ import ( "net/url" "time" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" + "gitlab.com/gitlab-org/gitlab-pages/metrics" ) // Client is a HTTP client to access Pages internal API @@ -32,7 +33,7 @@ func NewClient(baseURL string, secretKey []byte, connectionTimeout, jwtTokenExpi return nil, errors.New("GitLab API URL or API secret has not been provided") } - url, err := url.Parse(baseURL) + parsedURL, err := url.Parse(baseURL) if err != nil { return nil, err } @@ -47,10 +48,10 @@ func NewClient(baseURL string, secretKey []byte, connectionTimeout, jwtTokenExpi return &Client{ secretKey: secretKey, - baseURL: url, + baseURL: parsedURL, httpClient: &http.Client{ Timeout: connectionTimeout, - Transport: httptransport.Transport, + Transport: httptransport.NewTransportWithMetrics(metrics.DomainsSourceAPICallDuration, metrics.DomainsSourceAPIReqTotal), }, jwtTokenExpiry: jwtTokenExpiry, }, nil @@ -115,6 +116,8 @@ func (gc *Client) get(ctx context.Context, path string, params url.Values) (*htt return resp, nil } + // nolint: errcheck + // best effort to discard and close the response body io.Copy(ioutil.Discard, resp.Body) resp.Body.Close() -- cgit v1.2.3