Merge branch '423-move-httptrace-to-transport' into 'master'

Move tracer to httptransport

See merge request gitlab-org/gitlab-pages!365

6 files changed, 49 insertions, 39 deletions

diff --git a/acceptance_test.go b/acceptance_test.go
index c946167e..87cd307c 100644
--- a/acceptance_test.go
+++ b/acceptance_test.go
@@ -487,6 +487,7 @@ func TestPrometheusMetricsCanBeScraped(t *testing.T) {
 	require.Contains(t, string(body), "gitlab_pages_serving_time_seconds_sum")
 	require.Contains(t, string(body), `gitlab_pages_domains_source_api_requests_total{status_code="200"}`)
 	require.Contains(t, string(body), `gitlab_pages_domains_source_api_call_duration_bucket`)
+	require.Contains(t, string(body), `gitlab_pages_domains_source_api_trace_duration`)
 	// httprange
 	require.Contains(t, string(body), `gitlab_pages_httprange_requests_total{status_code="206"}`)
 	require.Contains(t, string(body), "gitlab_pages_httprange_requests_duration_bucket")
diff --git a/internal/httprange/http_reader.go b/internal/httprange/http_reader.go
index efb3b981..44694e85 100644
--- a/internal/httprange/http_reader.go
+++ b/internal/httprange/http_reader.go
@@ -54,13 +54,12 @@ var _ vfs.SeekableFile = &Reader{}
 var httpClient = &http.Client{
 	// The longest time the request can be executed
 	Timeout: 30 * time.Minute,
-	Transport: &tracedTransport{
-		next: httptransport.NewTransportWithMetrics(
-			"object_storage_client",
-			metrics.HTTPRangeRequestDuration,
-			metrics.HTTPRangeRequestsTotal,
-		),
-	},
+	Transport: httptransport.NewTransportWithMetrics(
+		"httprange_client",
+		metrics.HTTPRangeTraceDuration,
+		metrics.HTTPRangeRequestDuration,
+		metrics.HTTPRangeRequestsTotal,
+	),
 }
 
 // ensureResponse is set before reading from it.
diff --git a/internal/httprange/transport.go b/internal/httptransport/trace.go
index 9bb6e805..9ece5fc4 100644
--- a/internal/httprange/transport.go
+++ b/internal/httptransport/trace.go
@@ -1,39 +1,25 @@
-package httprange
+package httptransport
 
 import (
 	"crypto/tls"
-	"net/http"
 	"net/http/httptrace"
 	"time"
 
 	"gitlab.com/gitlab-org/labkit/log"
-
-	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
-type tracedTransport struct {
-	next http.RoundTripper
-}
-
-// withRoundTripper takes an original RoundTripper, reports metrics based on the
-// gauge and counter collectors passed
-func (tr *tracedTransport) RoundTrip(r *http.Request) (*http.Response, error) {
-	r = r.WithContext(httptrace.WithClientTrace(r.Context(), newTracer(time.Now())))
-
-	return tr.next.RoundTrip(r)
-}
-
-func newTracer(start time.Time) *httptrace.ClientTrace {
+func (mrt *meteredRoundTripper) newTracer(start time.Time) *httptrace.
+	ClientTrace {
 	trace := &httptrace.ClientTrace{
 		GetConn: func(host string) {
-			httpTraceObserve("httptrace.ClientTrace.GetConn", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.GetConn", start)
 
 			log.WithFields(log.Fields{
 				"host": host,
 			}).Traceln("httptrace.ClientTrace.GetConn")
 		},
 		GotConn: func(connInfo httptrace.GotConnInfo) {
-			httpTraceObserve("httptrace.ClientTrace.GotConn", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.GotConn", start)
 
 			log.WithFields(log.Fields{
 				"reused":       connInfo.Reused,
@@ -42,19 +28,19 @@ func newTracer(start time.Time) *httptrace.ClientTrace {
 			}).Traceln("httptrace.ClientTrace.GotConn")
 		},
 		GotFirstResponseByte: func() {
-			httpTraceObserve("httptrace.ClientTrace.GotFirstResponseByte", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.GotFirstResponseByte", start)
 		},
 		DNSStart: func(d httptrace.DNSStartInfo) {
-			httpTraceObserve("httptrace.ClientTrace.DNSStart", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.DNSStart", start)
 		},
 		DNSDone: func(d httptrace.DNSDoneInfo) {
-			httpTraceObserve("httptrace.ClientTrace.DNSDone", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.DNSDone", start)
 
 			log.WithFields(log.Fields{}).WithError(d.Err).
 				Traceln("httptrace.ClientTrace.DNSDone")
 		},
 		ConnectStart: func(net, addr string) {
-			httpTraceObserve("httptrace.ClientTrace.ConnectStart", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.ConnectStart", start)
 
 			log.WithFields(log.Fields{
 				"network": net,
@@ -62,7 +48,7 @@ func newTracer(start time.Time) *httptrace.ClientTrace {
 			}).Traceln("httptrace.ClientTrace.ConnectStart")
 		},
 		ConnectDone: func(net string, addr string, err error) {
-			httpTraceObserve("httptrace.ClientTrace.ConnectDone", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.ConnectDone", start)
 
 			log.WithFields(log.Fields{
 				"network": net,
@@ -70,10 +56,10 @@ func newTracer(start time.Time) *httptrace.ClientTrace {
 			}).WithError(err).Traceln("httptrace.ClientTrace.ConnectDone")
 		},
 		TLSHandshakeStart: func() {
-			httpTraceObserve("httptrace.ClientTrace.TLSHandshakeStart", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.TLSHandshakeStart", start)
 		},
 		TLSHandshakeDone: func(connState tls.ConnectionState, err error) {
-			httpTraceObserve("httptrace.ClientTrace.TLSHandshakeDone", start)
+			mrt.httpTraceObserve("httptrace.ClientTrace.TLSHandshakeDone", start)
 
 			log.WithFields(log.Fields{
 				"version":            connState.Version,
@@ -85,7 +71,7 @@ func newTracer(start time.Time) *httptrace.ClientTrace {
 	return trace
 }
 
-func httpTraceObserve(label string, start time.Time) {
-	metrics.HTTPRangeTraceDuration.WithLabelValues(label).
+func (mrt *meteredRoundTripper) httpTraceObserve(label string, start time.Time) {
+	mrt.tracer.WithLabelValues(label).
 		Observe(time.Since(start).Seconds())
 }
diff --git a/internal/httptransport/transport.go b/internal/httptransport/transport.go
index 304ac1c6..bc871ea7 100644
--- a/internal/httptransport/transport.go
+++ b/internal/httptransport/transport.go
@@ -5,6 +5,7 @@ import (
 	"crypto/x509"
 	"net"
 	"net/http"
+	"net/http/httptrace"
 	"strconv"
 	"strings"
 	"sync"
@@ -27,6 +28,7 @@ var (
 type meteredRoundTripper struct {
 	next      http.RoundTripper
 	name      string
+	tracer    *prometheus.HistogramVec
 	durations *prometheus.HistogramVec
 	counter   *prometheus.CounterVec
 }
@@ -46,11 +48,13 @@ func newInternalTransport() *http.Transport {
 
 // 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(name string, histogramVec *prometheus.HistogramVec, counterVec *prometheus.CounterVec) http.RoundTripper {
+func NewTransportWithMetrics(name string, tracerVec, durationsVec *prometheus.
+	HistogramVec, counterVec *prometheus.CounterVec) http.RoundTripper {
 	return &meteredRoundTripper{
 		next:      InternalTransport,
 		name:      name,
-		durations: histogramVec,
+		tracer:    tracerVec,
+		durations: durationsVec,
 		counter:   counterVec,
 	}
 }
@@ -84,6 +88,8 @@ func loadPool() {
 func (mrt *meteredRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
 	start := time.Now()
 
+	r = r.WithContext(httptrace.WithClientTrace(r.Context(), mrt.newTracer(start)))
+
 	resp, err := mrt.next.RoundTrip(r)
 	if err != nil {
 		mrt.counter.WithLabelValues("error").Inc()
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 94a0cfe5..0e8235c0 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -55,8 +55,12 @@ func NewClient(baseURL string, secretKey []byte, connectionTimeout, jwtTokenExpi
 		secretKey: secretKey,
 		baseURL:   parsedURL,
 		httpClient: &http.Client{
-			Timeout:   connectionTimeout,
-			Transport: httptransport.NewTransportWithMetrics("gitlab_internal_api", metrics.DomainsSourceAPICallDuration, metrics.DomainsSourceAPIReqTotal),
+			Timeout: connectionTimeout,
+			Transport: httptransport.NewTransportWithMetrics(
+				"gitlab_internal_api",
+				metrics.DomainsSourceAPITraceDuration,
+				metrics.DomainsSourceAPICallDuration,
+				metrics.DomainsSourceAPIReqTotal),
 		},
 		jwtTokenExpiry: jwtTokenExpiry,
 	}, nil
diff --git a/metrics/metrics.go b/metrics/metrics.go
index 669b4423..d6dc8ce1 100644
--- a/metrics/metrics.go
+++ b/metrics/metrics.go
@@ -78,6 +78,19 @@ var (
 		Help: "The time (in seconds) it takes to get a response from the GitLab domains API",
 	}, []string{"status_code"})
 
+	// DomainsSourceAPITraceDuration requests trace duration in seconds for
+	// different stages of an http request (see httptrace.ClientTrace)
+	DomainsSourceAPITraceDuration = prometheus.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Name: "gitlab_pages_domains_source_api_trace_duration",
+			Help: "Domain source API request tracing duration in seconds for " +
+				"different connection stages (see Go's httptrace.ClientTrace)",
+			Buckets: []float64{0.001, 0.005, 0.01, 0.02, 0.05, 0.100, 0.250,
+				0.500, 1, 2, 5, 10, 20, 50},
+		},
+		[]string{"request_stage"},
+	)
+
 	// DiskServingFileSize metric for file size serving. Includes a vfs_name (local or zip).
 	DiskServingFileSize = prometheus.NewHistogramVec(prometheus.HistogramOpts{
 		Name: "gitlab_pages_disk_serving_file_size_bytes",
@@ -199,6 +212,7 @@ func MustRegister() {
 		DomainsSourceCacheMiss,
 		DomainsSourceAPIReqTotal,
 		DomainsSourceAPICallDuration,
+		DomainsSourceAPITraceDuration,
 		DomainsSourceFailures,
 		ServerlessRequests,
 		ServerlessLatency,