feat(instrumentation/http/otelhttp): move client metrics creation int…

…o internal semconv package (#6002) In preparation for implementing `OTEL_SEMCONV_STABILITY_OPT_IN` for HTTP client metrics, this is the first PR to emit only old client metrics. This PR will enable us to add the ability for otelhttp to emit both old and new client metrics. Address issue #5973 --------- Co-authored-by: David Ashpole <[email protected]> Co-authored-by: Damien Mathieu <[email protected]>
open-telemetry · Oct 10, 2024 · 87d0229 · 87d0229
1 parent 900fc4b
commit 87d0229
Show file tree

Hide file tree

Showing 8 changed files with 348 additions and 97 deletions.
diff --git a/instrumentation/net/http/otelhttp/common.go b/instrumentation/net/http/otelhttp/common.go
@@ -18,13 +18,6 @@ const (
 	WriteErrorKey = attribute.Key("http.write_error") // if an error occurred while writing a reply, the string of the error (io.EOF is not recorded)
 )
 
-// Client HTTP metrics.
-const (
-	clientRequestSize  = "http.client.request.size"  // Outgoing request bytes total
-	clientResponseSize = "http.client.response.size" // Outgoing response bytes total
-	clientDuration     = "http.client.duration"      // Outgoing end to end duration, milliseconds
-)
-
 // Filter is a predicate used to determine whether a given http.request should
 // be traced. A Filter must return true if the request should be traced.
 type Filter func(*http.Request) bool

diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go
@@ -81,12 +81,6 @@ func (h *middleware) configure(c *config) {
 	h.semconv = semconv.NewHTTPServer(c.Meter)
 }
 
-func handleErr(err error) {
-	if err != nil {
-		otel.Handle(err)
-	}
-}
-
 // serveHTTP sets up tracing and calls the given next http.Handler with the span
 // context injected into the request context.
 func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http.Handler) {
@@ -190,14 +184,18 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http
 	// Use floating point division here for higher precision (instead of Millisecond method).
 	elapsedTime := float64(time.Since(requestStartTime)) / float64(time.Millisecond)
 
-	h.semconv.RecordMetrics(ctx, semconv.MetricData{
-		ServerName:           h.server,
-		Req:                  r,
-		StatusCode:           statusCode,
-		AdditionalAttributes: labeler.Get(),
-		RequestSize:          bw.BytesRead(),
-		ResponseSize:         bytesWritten,
-		ElapsedTime:          elapsedTime,
+	h.semconv.RecordMetrics(ctx, semconv.ServerMetricData{
+		ServerName:   h.server,
+		ResponseSize: bytesWritten,
+		MetricAttributes: semconv.MetricAttributes{
+			Req:                  r,
+			StatusCode:           statusCode,
+			AdditionalAttributes: labeler.Get(),
+		},
+		MetricData: semconv.MetricData{
+			RequestSize: bw.BytesRead(),
+			ElapsedTime: elapsedTime,
+		},
 	})
 }
 

diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go
@@ -83,26 +83,34 @@ func (s HTTPServer) Status(code int) (codes.Code, string) {
 	return codes.Unset, ""
 }
 
-type MetricData struct {
-	ServerName           string
+type ServerMetricData struct {
+	ServerName   string
+	ResponseSize int64
+
+	MetricData
+	MetricAttributes
+}
+
+type MetricAttributes struct {
 	Req                  *http.Request
 	StatusCode           int
 	AdditionalAttributes []attribute.KeyValue
+}
 
-	RequestSize  int64
-	ResponseSize int64
-	ElapsedTime  float64
+type MetricData struct {
+	RequestSize int64
+	ElapsedTime float64
 }
 
-func (s HTTPServer) RecordMetrics(ctx context.Context, md MetricData) {
+func (s HTTPServer) RecordMetrics(ctx context.Context, md ServerMetricData) {
 	if s.requestBytesCounter == nil || s.responseBytesCounter == nil || s.serverLatencyMeasure == nil {
 		// This will happen if an HTTPServer{} is used insted of NewHTTPServer.
 		return
 	}
 
 	attributes := oldHTTPServer{}.MetricAttributes(md.ServerName, md.Req, md.StatusCode, md.AdditionalAttributes)
 	o := metric.WithAttributeSet(attribute.NewSet(attributes...))
-	addOpts := []metric.AddOption{o} // Allocate vararg slice once.
+	addOpts := []metric.AddOption{o}
 	s.requestBytesCounter.Add(ctx, md.RequestSize, addOpts...)
 	s.responseBytesCounter.Add(ctx, md.ResponseSize, addOpts...)
 	s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o)
@@ -122,11 +130,20 @@ func NewHTTPServer(meter metric.Meter) HTTPServer {
 
 type HTTPClient struct {
 	duplicate bool
+
+	// old metrics
+	requestBytesCounter  metric.Int64Counter
+	responseBytesCounter metric.Int64Counter
+	latencyMeasure       metric.Float64Histogram
 }
 
-func NewHTTPClient() HTTPClient {
+func NewHTTPClient(meter metric.Meter) HTTPClient {
 	env := strings.ToLower(os.Getenv("OTEL_SEMCONV_STABILITY_OPT_IN"))
-	return HTTPClient{duplicate: env == "http/dup"}
+	client := HTTPClient{
+		duplicate: env == "http/dup",
+	}
+	client.requestBytesCounter, client.responseBytesCounter, client.latencyMeasure = oldHTTPClient{}.createMeasures(meter)
+	return client
 }
 
 // RequestTraceAttrs returns attributes for an HTTP request made by a client.
@@ -163,3 +180,48 @@ func (c HTTPClient) ErrorType(err error) attribute.KeyValue {
 
 	return attribute.KeyValue{}
 }
+
+type MetricOpts struct {
+	measurement metric.MeasurementOption
+	addOptions  metric.AddOption
+}
+
+func (o MetricOpts) MeasurementOption() metric.MeasurementOption {
+	return o.measurement
+}
+
+func (o MetricOpts) AddOptions() metric.AddOption {
+	return o.addOptions
+}
+
+func (c HTTPClient) MetricOptions(ma MetricAttributes) MetricOpts {
+	attributes := oldHTTPClient{}.MetricAttributes(ma.Req, ma.StatusCode, ma.AdditionalAttributes)
+	// TODO: Duplicate Metrics
+	set := metric.WithAttributeSet(attribute.NewSet(attributes...))
+	return MetricOpts{
+		measurement: set,
+		addOptions:  set,
+	}
+}
+
+func (s HTTPClient) RecordMetrics(ctx context.Context, md MetricData, opts MetricOpts) {
+	if s.requestBytesCounter == nil || s.latencyMeasure == nil {
+		// This will happen if an HTTPClient{} is used insted of NewHTTPClient().
+		return
+	}
+
+	s.requestBytesCounter.Add(ctx, md.RequestSize, opts.AddOptions())
+	s.latencyMeasure.Record(ctx, md.ElapsedTime, opts.MeasurementOption())
+
+	// TODO: Duplicate Metrics
+}
+
+func (s HTTPClient) RecordResponseSize(ctx context.Context, responseData int64, opts metric.AddOption) {
+	if s.responseBytesCounter == nil {
+		// This will happen if an HTTPClient{} is used insted of NewHTTPClient().
+		return
+	}
+
+	s.responseBytesCounter.Add(ctx, responseData, opts)
+	// TODO: Duplicate Metrics
+}
diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go
@@ -42,10 +42,53 @@ func TestHTTPServerDoesNotPanic(t *testing.T) {
 
 				_ = tt.server.RequestTraceAttrs("stuff", req)
 				_ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200})
-				tt.server.RecordMetrics(context.Background(), MetricData{
+				tt.server.RecordMetrics(context.Background(), ServerMetricData{
 					ServerName: "stuff",
+					MetricAttributes: MetricAttributes{
+						Req: req,
+					},
+				})
+			})
+		})
+	}
+}
+
+func TestHTTPClientDoesNotPanic(t *testing.T) {
+	testCases := []struct {
+		name   string
+		client HTTPClient
+	}{
+		{
+			name:   "empty",
+			client: HTTPClient{},
+		},
+		{
+			name:   "nil meter",
+			client: NewHTTPClient(nil),
+		},
+		{
+			name:   "with Meter",
+			client: NewHTTPClient(noop.Meter{}),
+		},
+	}
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			require.NotPanics(t, func() {
+				req, err := http.NewRequest("GET", "http://example.com", nil)
+				require.NoError(t, err)
+
+				_ = tt.client.RequestTraceAttrs(req)
+				_ = tt.client.ResponseTraceAttrs(&http.Response{StatusCode: 200})
+
+				opts := tt.client.MetricOptions(MetricAttributes{
 					Req:        req,
+					StatusCode: 200,
 				})
+				tt.client.RecordResponseSize(context.Background(), 40, opts.AddOptions())
+				tt.client.RecordMetrics(context.Background(), MetricData{
+					RequestSize: 20,
+					ElapsedTime: 1,
+				}, opts)
 			})
 		})
 	}
@@ -81,3 +124,11 @@ func NewTestHTTPServer() HTTPServer {
 		serverLatencyMeasure: &testInst{},
 	}
 }
+
+func NewTestHTTPClient() HTTPClient {
+	return HTTPClient{
+		requestBytesCounter:  &testInst{},
+		responseBytesCounter: &testInst{},
+		latencyMeasure:       &testInst{},
+	}
+}
diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go
@@ -151,7 +151,7 @@ func TestNewTraceRequest_Client(t *testing.T) {
 		attribute.String("user_agent.original", "go-test-agent"),
 		attribute.Int("http.request_content_length", 13),
 	}
-	client := NewHTTPClient()
+	client := NewHTTPClient(nil)
 	assert.ElementsMatch(t, want, client.RequestTraceAttrs(req))
 }
 
@@ -166,7 +166,7 @@ func TestNewTraceResponse_Client(t *testing.T) {
 	}
 
 	for _, tt := range testcases {
-		client := NewHTTPClient()
+		client := NewHTTPClient(nil)
 		assert.ElementsMatch(t, tt.want, client.ResponseTraceAttrs(&tt.resp))
 	}
 }

diff --git a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go
@@ -144,7 +144,7 @@ func (o oldHTTPServer) MetricAttributes(server string, req *http.Request, status
 
 	attributes := slices.Grow(additionalAttributes, n)
 	attributes = append(attributes,
-		o.methodMetric(req.Method),
+		standardizeHTTPMethodMetric(req.Method),
 		o.scheme(req.TLS != nil),
 		semconv.NetHostName(host))
 
@@ -164,16 +164,6 @@ func (o oldHTTPServer) MetricAttributes(server string, req *http.Request, status
 	return attributes
 }
 
-func (o oldHTTPServer) methodMetric(method string) attribute.KeyValue {
-	method = strings.ToUpper(method)
-	switch method {
-	case http.MethodConnect, http.MethodDelete, http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodPatch, http.MethodPost, http.MethodPut, http.MethodTrace:
-	default:
-		method = "_OTHER"
-	}
-	return semconv.HTTPMethod(method)
-}
-
 func (o oldHTTPServer) scheme(https bool) attribute.KeyValue { // nolint:revive
 	if https {
 		return semconv.HTTPSchemeHTTPS
@@ -190,3 +180,95 @@ func (o oldHTTPClient) RequestTraceAttrs(req *http.Request) []attribute.KeyValue
 func (o oldHTTPClient) ResponseTraceAttrs(resp *http.Response) []attribute.KeyValue {
 	return semconvutil.HTTPClientResponse(resp)
 }
+
+func (o oldHTTPClient) MetricAttributes(req *http.Request, statusCode int, additionalAttributes []attribute.KeyValue) []attribute.KeyValue {
+	/* The following semantic conventions are returned if present:
+	http.method                     string
+	http.status_code             int
+	net.peer.name                   string
+	net.peer.port                   int
+	*/
+
+	n := 2 // method, peer name.
+	var h string
+	if req.URL != nil {
+		h = req.URL.Host
+	}
+	var requestHost string
+	var requestPort int
+	for _, hostport := range []string{h, req.Header.Get("Host")} {
+		requestHost, requestPort = splitHostPort(hostport)
+		if requestHost != "" || requestPort > 0 {
+			break
+		}
+	}
+
+	port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", requestPort)
+	if port > 0 {
+		n++
+	}
+
+	if statusCode > 0 {
+		n++
+	}
+
+	attributes := slices.Grow(additionalAttributes, n)
+	attributes = append(attributes,
+		standardizeHTTPMethodMetric(req.Method),
+		semconv.NetPeerName(requestHost),
+	)
+
+	if port > 0 {
+		attributes = append(attributes, semconv.NetPeerPort(port))
+	}
+
+	if statusCode > 0 {
+		attributes = append(attributes, semconv.HTTPStatusCode(statusCode))
+	}
+	return attributes
+}
+
+// Client HTTP metrics.
+const (
+	clientRequestSize  = "http.client.request.size"  // Incoming request bytes total
+	clientResponseSize = "http.client.response.size" // Incoming response bytes total
+	clientDuration     = "http.client.duration"      // Incoming end to end duration, milliseconds
+)
+
+func (o oldHTTPClient) createMeasures(meter metric.Meter) (metric.Int64Counter, metric.Int64Counter, metric.Float64Histogram) {
+	if meter == nil {
+		return noop.Int64Counter{}, noop.Int64Counter{}, noop.Float64Histogram{}
+	}
+	requestBytesCounter, err := meter.Int64Counter(
+		clientRequestSize,
+		metric.WithUnit("By"),
+		metric.WithDescription("Measures the size of HTTP request messages."),
+	)
+	handleErr(err)
+
+	responseBytesCounter, err := meter.Int64Counter(
+		clientResponseSize,
+		metric.WithUnit("By"),
+		metric.WithDescription("Measures the size of HTTP response messages."),
+	)
+	handleErr(err)
+
+	latencyMeasure, err := meter.Float64Histogram(
+		clientDuration,
+		metric.WithUnit("ms"),
+		metric.WithDescription("Measures the duration of outbound HTTP requests."),
+	)
+	handleErr(err)
+
+	return requestBytesCounter, responseBytesCounter, latencyMeasure
+}
+
+func standardizeHTTPMethodMetric(method string) attribute.KeyValue {
+	method = strings.ToUpper(method)
+	switch method {
+	case http.MethodConnect, http.MethodDelete, http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodPatch, http.MethodPost, http.MethodPut, http.MethodTrace:
+	default:
+		method = "_OTHER"
+	}
+	return semconv.HTTPMethod(method)
+}