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 }