From 9e8f1ec77bececa226bcdb31dd19717dbb618fe8 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Fri, 2 Aug 2024 18:00:53 +0200 Subject: [PATCH] HTTP Semconv migration Part1 Server Metrics - v1.20.0 support (#5818) This change moves the metric creation and use into the semconv package. This is because metrics names are defined by the semantic convention, so to be able to change them seamlessly they should be within the scope of the semconv package. --------- Co-authored-by: Aaron Clawson Co-authored-by: Tyler Yahn --- instrumentation/net/http/otelhttp/common.go | 7 -- instrumentation/net/http/otelhttp/handler.go | 66 +++-------- .../otelhttp/internal/semconv/bench_test.go | 2 +- .../net/http/otelhttp/internal/semconv/env.go | 53 +++++++-- .../otelhttp/internal/semconv/env_test.go | 83 ++++++++++++++ .../internal/semconv/httpconv_test.go | 2 +- .../http/otelhttp/internal/semconv/util.go | 7 ++ .../http/otelhttp/internal/semconv/v1.20.0.go | 108 ++++++++++++++++++ .../otelhttp/internal/semconv/v1.20.0_test.go | 40 ++++++- 9 files changed, 300 insertions(+), 68 deletions(-) create mode 100644 instrumentation/net/http/otelhttp/internal/semconv/env_test.go diff --git a/instrumentation/net/http/otelhttp/common.go b/instrumentation/net/http/otelhttp/common.go index 214acaf581e..5d6e6156b7b 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) ) -// Server HTTP metrics. -const ( - serverRequestSize = "http.server.request.size" // Incoming request bytes total - serverResponseSize = "http.server.response.size" // Incoming response bytes total - serverDuration = "http.server.duration" // Incoming end to end duration, milliseconds -) - // Client HTTP metrics. const ( clientRequestSize = "http.client.request.size" // Outgoing request bytes total diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go index 5169ebf02eb..33580a35b77 100644 --- a/instrumentation/net/http/otelhttp/handler.go +++ b/instrumentation/net/http/otelhttp/handler.go @@ -11,10 +11,7 @@ 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/metric" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) @@ -25,7 +22,6 @@ type middleware struct { server string tracer trace.Tracer - meter metric.Meter propagators propagation.TextMapPropagator spanStartOptions []trace.SpanStartOption readEvent bool @@ -35,10 +31,7 @@ type middleware struct { publicEndpoint bool publicEndpointFn func(*http.Request) bool - traceSemconv semconv.HTTPServer - requestBytesCounter metric.Int64Counter - responseBytesCounter metric.Int64Counter - serverLatencyMeasure metric.Float64Histogram + semconv semconv.HTTPServer } func defaultHandlerFormatter(operation string, _ *http.Request) string { @@ -57,8 +50,6 @@ func NewHandler(handler http.Handler, operation string, opts ...Option) http.Han func NewMiddleware(operation string, opts ...Option) func(http.Handler) http.Handler { h := middleware{ operation: operation, - - traceSemconv: semconv.NewHTTPServer(), } defaultOpts := []Option{ @@ -68,7 +59,6 @@ func NewMiddleware(operation string, opts ...Option) func(http.Handler) http.Han c := newConfig(append(defaultOpts, opts...)...) h.configure(c) - h.createMeasures() return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -79,7 +69,6 @@ func NewMiddleware(operation string, opts ...Option) func(http.Handler) http.Han func (h *middleware) configure(c *config) { h.tracer = c.Tracer - h.meter = c.Meter h.propagators = c.Propagators h.spanStartOptions = c.SpanStartOptions h.readEvent = c.ReadEvent @@ -89,6 +78,7 @@ func (h *middleware) configure(c *config) { h.publicEndpoint = c.PublicEndpoint h.publicEndpointFn = c.PublicEndpointFn h.server = c.ServerName + h.semconv = semconv.NewHTTPServer(c.Meter) } func handleErr(err error) { @@ -97,30 +87,6 @@ func handleErr(err error) { } } -func (h *middleware) createMeasures() { - var err error - h.requestBytesCounter, err = h.meter.Int64Counter( - serverRequestSize, - metric.WithUnit("By"), - metric.WithDescription("Measures the size of HTTP request messages."), - ) - handleErr(err) - - h.responseBytesCounter, err = h.meter.Int64Counter( - serverResponseSize, - metric.WithUnit("By"), - metric.WithDescription("Measures the size of HTTP response messages."), - ) - handleErr(err) - - h.serverLatencyMeasure, err = h.meter.Float64Histogram( - serverDuration, - metric.WithUnit("ms"), - metric.WithDescription("Measures the duration of inbound HTTP requests."), - ) - handleErr(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) { @@ -135,7 +101,7 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http ctx := h.propagators.Extract(r.Context(), propagation.HeaderCarrier(r.Header)) opts := []trace.SpanStartOption{ - trace.WithAttributes(h.traceSemconv.RequestTraceAttrs(h.server, r)...), + trace.WithAttributes(h.semconv.RequestTraceAttrs(h.server, r)...), } opts = append(opts, h.spanStartOptions...) @@ -212,8 +178,8 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http statusCode := rww.StatusCode() bytesWritten := rww.BytesWritten() - span.SetStatus(semconv.ServerStatus(statusCode)) - span.SetAttributes(h.traceSemconv.ResponseTraceAttrs(semconv.ResponseTelemetry{ + span.SetStatus(h.semconv.Status(statusCode)) + span.SetAttributes(h.semconv.ResponseTraceAttrs(semconv.ResponseTelemetry{ StatusCode: statusCode, ReadBytes: bw.BytesRead(), ReadError: bw.Error(), @@ -221,26 +187,24 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http WriteError: rww.Error(), })...) - // Add metrics - attributes := append(labeler.Get(), semconvutil.HTTPServerRequestMetrics(h.server, r)...) - if statusCode > 0 { - attributes = append(attributes, semconv.HTTPStatusCode(statusCode)) - } - o := metric.WithAttributeSet(attribute.NewSet(attributes...)) - - h.requestBytesCounter.Add(ctx, bw.BytesRead(), o) - h.responseBytesCounter.Add(ctx, bytesWritten, o) - // Use floating point division here for higher precision (instead of Millisecond method). elapsedTime := float64(time.Since(requestStartTime)) / float64(time.Millisecond) - h.serverLatencyMeasure.Record(ctx, elapsedTime, o) + h.semconv.RecordMetrics(ctx, semconv.MetricData{ + ServerName: h.server, + Req: r, + StatusCode: statusCode, + AdditionalAttributes: labeler.Get(), + RequestSize: bw.BytesRead(), + ResponseSize: bytesWritten, + ElapsedTime: elapsedTime, + }) } // WithRouteTag annotates spans and metrics with the provided route name // with HTTP route attribute. func WithRouteTag(route string, h http.Handler) http.Handler { - attr := semconv.NewHTTPServer().Route(route) + attr := semconv.NewHTTPServer(nil).Route(route) return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { span := trace.SpanFromContext(r.Context()) span.SetAttributes(attr) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/bench_test.go b/instrumentation/net/http/otelhttp/internal/semconv/bench_test.go index ec2d2794154..b24abba4cbc 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/bench_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/bench_test.go @@ -35,7 +35,7 @@ func BenchmarkHTTPServerRequest(b *testing.B) { RemoteAddr: "127.0.0.1:38738", RequestURI: "/", } - serv := NewHTTPServer() + serv := NewHTTPServer(nil) b.ReportAllocs() b.ResetTimer() diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index 25970e3e9eb..d95391becec 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -4,6 +4,7 @@ package semconv // import "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" import ( + "context" "fmt" "net/http" "os" @@ -11,6 +12,7 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/metric" ) type ResponseTelemetry struct { @@ -23,6 +25,11 @@ type ResponseTelemetry struct { type HTTPServer struct { duplicate bool + + // Old metrics + requestBytesCounter metric.Int64Counter + responseBytesCounter metric.Int64Counter + serverLatencyMeasure metric.Float64Histogram } // RequestTraceAttrs returns trace attributes for an HTTP request received by a @@ -63,15 +70,10 @@ func (s HTTPServer) Route(route string) attribute.KeyValue { return oldHTTPServer{}.Route(route) } -func NewHTTPServer() HTTPServer { - env := strings.ToLower(os.Getenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE")) - return HTTPServer{duplicate: env == "http/dup"} -} - -// ServerStatus returns a span status code and message for an HTTP status code +// Status returns a span status code and message for an HTTP status code // value returned by a server. Status codes in the 400-499 range are not // returned as errors. -func ServerStatus(code int) (codes.Code, string) { +func (s HTTPServer) Status(code int) (codes.Code, string) { if code < 100 || code >= 600 { return codes.Error, fmt.Sprintf("Invalid HTTP status code %d", code) } @@ -81,6 +83,43 @@ func ServerStatus(code int) (codes.Code, string) { return codes.Unset, "" } +type MetricData struct { + ServerName string + Req *http.Request + StatusCode int + AdditionalAttributes []attribute.KeyValue + + RequestSize int64 + ResponseSize int64 + ElapsedTime float64 +} + +func (s HTTPServer) RecordMetrics(ctx context.Context, md MetricData) { + 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. + s.requestBytesCounter.Add(ctx, md.RequestSize, addOpts...) + s.responseBytesCounter.Add(ctx, md.ResponseSize, addOpts...) + s.serverLatencyMeasure.Record(ctx, md.ElapsedTime, o) + + // TODO: Duplicate Metrics +} + +func NewHTTPServer(meter metric.Meter) HTTPServer { + env := strings.ToLower(os.Getenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE")) + duplicate := env == "http/dup" + server := HTTPServer{ + duplicate: duplicate, + } + server.requestBytesCounter, server.responseBytesCounter, server.serverLatencyMeasure = oldHTTPServer{}.createMeasures(meter) + return server +} + type HTTPClient struct { duplicate bool } diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env_test.go b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go new file mode 100644 index 00000000000..5f4a3e391d6 --- /dev/null +++ b/instrumentation/net/http/otelhttp/internal/semconv/env_test.go @@ -0,0 +1,83 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package semconv + +import ( + "context" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/embedded" + "go.opentelemetry.io/otel/metric/noop" +) + +func TestHTTPServerDoesNotPanic(t *testing.T) { + testCases := []struct { + name string + server HTTPServer + }{ + { + name: "empty", + server: HTTPServer{}, + }, + { + name: "nil meter", + server: NewHTTPServer(nil), + }, + { + name: "with Meter", + server: NewHTTPServer(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.server.RequestTraceAttrs("stuff", req) + _ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200}) + tt.server.RecordMetrics(context.Background(), MetricData{ + ServerName: "stuff", + Req: req, + }) + }) + }) + } +} + +type testInst struct { + embedded.Int64Counter + embedded.Float64Histogram + + intValue int64 + floatValue float64 + attributes []attribute.KeyValue +} + +func (t *testInst) Add(ctx context.Context, incr int64, options ...metric.AddOption) { + t.intValue = incr + cfg := metric.NewAddConfig(options) + attr := cfg.Attributes() + t.attributes = attr.ToSlice() +} + +func (t *testInst) Record(ctx context.Context, value float64, options ...metric.RecordOption) { + t.floatValue = value + cfg := metric.NewRecordConfig(options) + attr := cfg.Attributes() + t.attributes = attr.ToSlice() +} + +func NewTestHTTPServer() HTTPServer { + return HTTPServer{ + requestBytesCounter: &testInst{}, + responseBytesCounter: &testInst{}, + serverLatencyMeasure: &testInst{}, + } +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index 49b70a04535..82bb0133168 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go @@ -18,7 +18,7 @@ import ( func TestNewTraceRequest(t *testing.T) { t.Setenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE", "http/dup") - serv := NewHTTPServer() + serv := NewHTTPServer(nil) want := func(req testServerReq) []attribute.KeyValue { return []attribute.KeyValue{ attribute.String("http.request.method", "GET"), diff --git a/instrumentation/net/http/otelhttp/internal/semconv/util.go b/instrumentation/net/http/otelhttp/internal/semconv/util.go index 6cede5a5f99..af862d1a7f0 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/util.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/util.go @@ -9,6 +9,7 @@ import ( "strconv" "strings" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" semconvNew "go.opentelemetry.io/otel/semconv/v1.26.0" ) @@ -89,3 +90,9 @@ var methodLookup = map[string]attribute.KeyValue{ http.MethodPut: semconvNew.HTTPRequestMethodPut, http.MethodTrace: semconvNew.HTTPRequestMethodTrace, } + +func handleErr(err error) { + if err != nil { + otel.Handle(err) + } +} 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 c2bc041ac50..c999b05e675 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go @@ -7,9 +7,13 @@ import ( "errors" "io" "net/http" + "slices" + "strings" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconvutil" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/noop" semconv "go.opentelemetry.io/otel/semconv/v1.20.0" ) @@ -73,6 +77,110 @@ func HTTPStatusCode(status int) attribute.KeyValue { return semconv.HTTPStatusCode(status) } +// Server HTTP metrics. +const ( + serverRequestSize = "http.server.request.size" // Incoming request bytes total + serverResponseSize = "http.server.response.size" // Incoming response bytes total + serverDuration = "http.server.duration" // Incoming end to end duration, milliseconds +) + +func (h oldHTTPServer) createMeasures(meter metric.Meter) (metric.Int64Counter, metric.Int64Counter, metric.Float64Histogram) { + if meter == nil { + return noop.Int64Counter{}, noop.Int64Counter{}, noop.Float64Histogram{} + } + var err error + requestBytesCounter, err := meter.Int64Counter( + serverRequestSize, + metric.WithUnit("By"), + metric.WithDescription("Measures the size of HTTP request messages."), + ) + handleErr(err) + + responseBytesCounter, err := meter.Int64Counter( + serverResponseSize, + metric.WithUnit("By"), + metric.WithDescription("Measures the size of HTTP response messages."), + ) + handleErr(err) + + serverLatencyMeasure, err := meter.Float64Histogram( + serverDuration, + metric.WithUnit("ms"), + metric.WithDescription("Measures the duration of inbound HTTP requests."), + ) + handleErr(err) + + return requestBytesCounter, responseBytesCounter, serverLatencyMeasure +} + +func (o oldHTTPServer) MetricAttributes(server string, req *http.Request, statusCode int, additionalAttributes []attribute.KeyValue) []attribute.KeyValue { + n := len(additionalAttributes) + 3 + var host string + var p int + if server == "" { + host, p = splitHostPort(req.Host) + } else { + // Prioritize the primary server name. + host, p = splitHostPort(server) + if p < 0 { + _, p = splitHostPort(req.Host) + } + } + hostPort := requiredHTTPPort(req.TLS != nil, p) + if hostPort > 0 { + n++ + } + protoName, protoVersion := netProtocol(req.Proto) + if protoName != "" { + n++ + } + if protoVersion != "" { + n++ + } + + if statusCode > 0 { + n++ + } + + attributes := slices.Grow(additionalAttributes, n) + attributes = append(attributes, + o.methodMetric(req.Method), + o.scheme(req.TLS != nil), + semconv.NetHostName(host)) + + if hostPort > 0 { + attributes = append(attributes, semconv.NetHostPort(hostPort)) + } + if protoName != "" { + attributes = append(attributes, semconv.NetProtocolName(protoName)) + } + if protoVersion != "" { + attributes = append(attributes, semconv.NetProtocolVersion(protoVersion)) + } + + if statusCode > 0 { + attributes = append(attributes, semconv.HTTPStatusCode(statusCode)) + } + 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 + } + return semconv.HTTPSchemeHTTP +} + type oldHTTPClient struct{} func (o oldHTTPClient) RequestTraceAttrs(req *http.Request) []attribute.KeyValue { 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 fcf116f94f5..7fc0e90eb45 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 @@ -4,6 +4,7 @@ package semconv import ( + "context" "fmt" "net/http" "strings" @@ -17,7 +18,7 @@ import ( func TestV120TraceRequest(t *testing.T) { // Anything but "http" or "http/dup" works. t.Setenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE", "old") - serv := NewHTTPServer() + serv := NewHTTPServer(nil) want := func(req testServerReq) []attribute.KeyValue { return []attribute.KeyValue{ attribute.String("http.method", "GET"), @@ -86,6 +87,43 @@ func TestV120TraceResponse(t *testing.T) { } } +func TestV120RecordMetrics(t *testing.T) { + server := NewTestHTTPServer() + 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, + ResponseSize: 200, + ElapsedTime: 300, + }) + + assert.Equal(t, int64(100), server.requestBytesCounter.(*testInst).intValue) + assert.Equal(t, int64(200), server.responseBytesCounter.(*testInst).intValue) + assert.Equal(t, float64(300), server.serverLatencyMeasure.(*testInst).floatValue) + + want := []attribute.KeyValue{ + attribute.String("http.scheme", "http"), + attribute.String("http.method", "POST"), + attribute.Int64("http.status_code", 301), + attribute.String("key", "value"), + attribute.String("net.host.name", "stuff"), + attribute.String("net.protocol.name", "http"), + attribute.String("net.protocol.version", "1.1"), + } + + assert.ElementsMatch(t, want, server.requestBytesCounter.(*testInst).attributes) + assert.ElementsMatch(t, want, server.responseBytesCounter.(*testInst).attributes) + assert.ElementsMatch(t, want, server.serverLatencyMeasure.(*testInst).attributes) +} + func TestV120ClientRequest(t *testing.T) { body := strings.NewReader("Hello, world!") url := "https://example.com:8888/foo/bar?stuff=morestuff"