From 87d02293473900b80d127c1001bd38a5caf41260 Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Thu, 10 Oct 2024 05:24:59 -0400 Subject: [PATCH] feat(instrumentation/http/otelhttp): move client metrics creation into 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 Co-authored-by: Damien Mathieu <42@dmathieu.com> --- instrumentation/net/http/otelhttp/common.go | 7 -- instrumentation/net/http/otelhttp/handler.go | 26 ++-- .../net/http/otelhttp/internal/semconv/env.go | 80 ++++++++++-- .../otelhttp/internal/semconv/env_test.go | 53 +++++++- .../internal/semconv/httpconv_test.go | 4 +- .../http/otelhttp/internal/semconv/v1.20.0.go | 104 ++++++++++++++-- .../otelhttp/internal/semconv/v1.20.0_test.go | 115 ++++++++++++++++-- .../net/http/otelhttp/transport.go | 56 ++------- 8 files changed, 348 insertions(+), 97 deletions(-) diff --git a/instrumentation/net/http/otelhttp/common.go b/instrumentation/net/http/otelhttp/common.go index 5d6e6156b7b..a83a026274a 100644 --- 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 index 33580a35b77..e4236ab398c 100644 --- 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 index 9cae4cab86a..fb893b25042 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -83,18 +83,26 @@ 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 @@ -102,7 +110,7 @@ func (s HTTPServer) RecordMetrics(ctx context.Context, md MetricData) { 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 index 5f4a3e391d6..3a02a777373 100644 --- 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 index 91a499b07bc..6a3f6c09a4f 100644 --- 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 index c999b05e675..5367732ec5d 100644 --- 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) +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go index a35033f49c1..eef382a5047 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go @@ -92,17 +92,20 @@ func TestV120RecordMetrics(t *testing.T) { req, err := http.NewRequest("POST", "http://example.com", nil) assert.NoError(t, err) - server.RecordMetrics(context.Background(), MetricData{ - ServerName: "stuff", - Req: req, - StatusCode: 301, - AdditionalAttributes: []attribute.KeyValue{ - attribute.String("key", "value"), - }, - - RequestSize: 100, + server.RecordMetrics(context.Background(), ServerMetricData{ + ServerName: "stuff", ResponseSize: 200, - ElapsedTime: 300, + MetricAttributes: MetricAttributes{ + Req: req, + StatusCode: 301, + AdditionalAttributes: []attribute.KeyValue{ + attribute.String("key", "value"), + }, + }, + MetricData: MetricData{ + RequestSize: 100, + ElapsedTime: 300, + }, }) assert.Equal(t, int64(100), server.requestBytesCounter.(*testInst).intValue) @@ -157,3 +160,95 @@ func TestV120ClientResponse(t *testing.T) { got := oldHTTPClient{}.ResponseTraceAttrs(&resp) assert.ElementsMatch(t, want, got) } + +func TestV120ClientMetrics(t *testing.T) { + client := NewTestHTTPClient() + req, err := http.NewRequest("POST", "http://example.com", nil) + assert.NoError(t, err) + + opts := client.MetricOptions(MetricAttributes{ + Req: req, + StatusCode: 301, + AdditionalAttributes: []attribute.KeyValue{ + attribute.String("key", "value"), + }, + }) + + ctx := context.Background() + + client.RecordResponseSize(ctx, 200, opts.AddOptions()) + + client.RecordMetrics(ctx, MetricData{ + RequestSize: 100, + ElapsedTime: 300, + }, opts) + + assert.Equal(t, int64(100), client.requestBytesCounter.(*testInst).intValue) + assert.Equal(t, int64(200), client.responseBytesCounter.(*testInst).intValue) + assert.Equal(t, float64(300), client.latencyMeasure.(*testInst).floatValue) + + want := []attribute.KeyValue{ + attribute.String("http.method", "POST"), + attribute.Int64("http.status_code", 301), + attribute.String("key", "value"), + attribute.String("net.peer.name", "example.com"), + } + + assert.ElementsMatch(t, want, client.requestBytesCounter.(*testInst).attributes) + assert.ElementsMatch(t, want, client.responseBytesCounter.(*testInst).attributes) + assert.ElementsMatch(t, want, client.latencyMeasure.(*testInst).attributes) +} + +func TestStandardizeHTTPMethodMetric(t *testing.T) { + testCases := []struct { + method string + want attribute.KeyValue + }{ + { + method: "GET", + want: attribute.String("http.method", "GET"), + }, + { + method: "POST", + want: attribute.String("http.method", "POST"), + }, + { + method: "PUT", + want: attribute.String("http.method", "PUT"), + }, + { + method: "DELETE", + want: attribute.String("http.method", "DELETE"), + }, + { + method: "HEAD", + want: attribute.String("http.method", "HEAD"), + }, + { + method: "OPTIONS", + want: attribute.String("http.method", "OPTIONS"), + }, + { + method: "CONNECT", + want: attribute.String("http.method", "CONNECT"), + }, + { + method: "TRACE", + want: attribute.String("http.method", "TRACE"), + }, + { + method: "PATCH", + want: attribute.String("http.method", "PATCH"), + }, + { + method: "test", + want: attribute.String("http.method", "_OTHER"), + }, + } + for _, tt := range testCases { + t.Run(tt.method, func(t *testing.T) { + got := standardizeHTTPMethodMetric(tt.method) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index cc918367511..39681ad4b09 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -13,11 +13,9 @@ import ( "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/request" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" - "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconvutil" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" - "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" @@ -29,7 +27,6 @@ type Transport struct { rt http.RoundTripper tracer trace.Tracer - meter metric.Meter propagators propagation.TextMapPropagator spanStartOptions []trace.SpanStartOption filters []Filter @@ -37,10 +34,7 @@ type Transport struct { clientTrace func(context.Context) *httptrace.ClientTrace metricAttributesFn func(*http.Request) []attribute.KeyValue - semconv semconv.HTTPClient - requestBytesCounter metric.Int64Counter - responseBytesCounter metric.Int64Counter - latencyMeasure metric.Float64Histogram + semconv semconv.HTTPClient } var _ http.RoundTripper = &Transport{} @@ -57,8 +51,7 @@ func NewTransport(base http.RoundTripper, opts ...Option) *Transport { } t := Transport{ - rt: base, - semconv: semconv.NewHTTPClient(), + rt: base, } defaultOpts := []Option{ @@ -68,46 +61,21 @@ func NewTransport(base http.RoundTripper, opts ...Option) *Transport { c := newConfig(append(defaultOpts, opts...)...) t.applyConfig(c) - t.createMeasures() return &t } func (t *Transport) applyConfig(c *config) { t.tracer = c.Tracer - t.meter = c.Meter t.propagators = c.Propagators t.spanStartOptions = c.SpanStartOptions t.filters = c.Filters t.spanNameFormatter = c.SpanNameFormatter t.clientTrace = c.ClientTrace + t.semconv = semconv.NewHTTPClient(c.Meter) t.metricAttributesFn = c.MetricAttributesFn } -func (t *Transport) createMeasures() { - var err error - t.requestBytesCounter, err = t.meter.Int64Counter( - clientRequestSize, - metric.WithUnit("By"), - metric.WithDescription("Measures the size of HTTP request messages."), - ) - handleErr(err) - - t.responseBytesCounter, err = t.meter.Int64Counter( - clientResponseSize, - metric.WithUnit("By"), - metric.WithDescription("Measures the size of HTTP response messages."), - ) - handleErr(err) - - t.latencyMeasure, err = t.meter.Float64Histogram( - clientDuration, - metric.WithUnit("ms"), - metric.WithDescription("Measures the duration of outbound HTTP requests."), - ) - handleErr(err) -} - func defaultTransportFormatter(_ string, r *http.Request) string { return "HTTP " + r.Method } @@ -177,16 +145,15 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { } // metrics - metricAttrs := append(append(labeler.Get(), semconvutil.HTTPClientRequestMetrics(r)...), t.metricAttributesFromRequest(r)...) - if res.StatusCode > 0 { - metricAttrs = append(metricAttrs, semconv.HTTPStatusCode(res.StatusCode)) - } - o := metric.WithAttributeSet(attribute.NewSet(metricAttrs...)) + metricOpts := t.semconv.MetricOptions(semconv.MetricAttributes{ + Req: r, + StatusCode: res.StatusCode, + AdditionalAttributes: append(labeler.Get(), t.metricAttributesFromRequest(r)...), + }) - t.requestBytesCounter.Add(ctx, bw.BytesRead(), o) // For handling response bytes we leverage a callback when the client reads the http response readRecordFunc := func(n int64) { - t.responseBytesCounter.Add(ctx, n, o) + t.semconv.RecordResponseSize(ctx, n, metricOpts.AddOptions()) } // traces @@ -198,7 +165,10 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { // Use floating point division here for higher precision (instead of Millisecond method). elapsedTime := float64(time.Since(requestStartTime)) / float64(time.Millisecond) - t.latencyMeasure.Record(ctx, elapsedTime, o) + t.semconv.RecordMetrics(ctx, semconv.MetricData{ + RequestSize: bw.BytesRead(), + ElapsedTime: elapsedTime, + }, metricOpts) return res, nil }