diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go index 843b4fa2fb1..c64f8beca71 100644 --- a/instrumentation/net/http/otelhttp/handler.go +++ b/instrumentation/net/http/otelhttp/handler.go @@ -4,18 +4,16 @@ package otelhttp // import "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" import ( - "io" "net/http" "time" "github.com/felixge/httpsnoop" + "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" - semconv "go.opentelemetry.io/otel/semconv/v1.20.0" "go.opentelemetry.io/otel/trace" ) @@ -35,6 +33,7 @@ type middleware struct { publicEndpoint bool publicEndpointFn func(*http.Request) bool + traceSemconv semconv.HTTPServer requestBytesCounter metric.Int64Counter responseBytesCounter metric.Int64Counter serverLatencyMeasure metric.Float64Histogram @@ -56,6 +55,8 @@ 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{ @@ -132,12 +133,9 @@ 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(semconvutil.HTTPServerRequest(h.server, r)...), - } - if h.server != "" { - hostAttr := semconv.NetHostName(h.server) - opts = append(opts, trace.WithAttributes(hostAttr)) + trace.WithAttributes(h.traceSemconv.RequestTraceAttrs(h.server, r)...), } + opts = append(opts, h.spanStartOptions...) if h.publicEndpoint || (h.publicEndpointFn != nil && h.publicEndpointFn(r.WithContext(ctx))) { opts = append(opts, trace.WithNewRoot()) @@ -213,7 +211,14 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http next.ServeHTTP(w, r.WithContext(ctx)) - setAfterServeAttributes(span, bw.read.Load(), rww.written, rww.statusCode, bw.err, rww.err) + span.SetStatus(semconv.ServerStatus(rww.statusCode)) + span.SetAttributes(h.traceSemconv.ResponseTraceAttrs(semconv.ResponseTelemetry{ + StatusCode: rww.statusCode, + ReadBytes: bw.read.Load(), + ReadError: bw.err, + WriteBytes: rww.written, + WriteError: rww.err, + })...) // Add metrics attributes := append(labeler.Get(), semconvutil.HTTPServerRequestMetrics(h.server, r)...) @@ -230,37 +235,11 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http h.serverLatencyMeasure.Record(ctx, elapsedTime, o) } -func setAfterServeAttributes(span trace.Span, read, wrote int64, statusCode int, rerr, werr error) { - attributes := []attribute.KeyValue{} - - // TODO: Consider adding an event after each read and write, possibly as an - // option (defaulting to off), so as to not create needlessly verbose spans. - if read > 0 { - attributes = append(attributes, ReadBytesKey.Int64(read)) - } - if rerr != nil && rerr != io.EOF { - attributes = append(attributes, ReadErrorKey.String(rerr.Error())) - } - if wrote > 0 { - attributes = append(attributes, WroteBytesKey.Int64(wrote)) - } - if statusCode > 0 { - attributes = append(attributes, semconv.HTTPStatusCode(statusCode)) - } - span.SetStatus(semconvutil.HTTPServerStatus(statusCode)) - - if werr != nil && werr != io.EOF { - attributes = append(attributes, WriteErrorKey.String(werr.Error())) - } - span.SetAttributes(attributes...) -} - // 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) return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - attr := semconv.HTTPRouteKey.String(route) - 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 new file mode 100644 index 00000000000..ec2d2794154 --- /dev/null +++ b/instrumentation/net/http/otelhttp/internal/semconv/bench_test.go @@ -0,0 +1,45 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package semconv + +import ( + "net/http" + "net/url" + "testing" + + "go.opentelemetry.io/otel/attribute" +) + +var benchHTTPServerRequestResults []attribute.KeyValue + +// BenchmarkHTTPServerRequest allows comparison between different version of the HTTP server. +// To use an alternative start this test with OTEL_HTTP_CLIENT_COMPATIBILITY_MODE set to the +// version under test. +func BenchmarkHTTPServerRequest(b *testing.B) { + // Request was generated from TestHTTPServerRequest request. + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Path: "/", + }, + Proto: "HTTP/1.1", + ProtoMajor: 1, + ProtoMinor: 1, + Header: http.Header{ + "User-Agent": []string{"Go-http-client/1.1"}, + "Accept-Encoding": []string{"gzip"}, + }, + Body: http.NoBody, + Host: "127.0.0.1:39093", + RemoteAddr: "127.0.0.1:38738", + RequestURI: "/", + } + serv := NewHTTPServer() + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + benchHTTPServerRequestResults = serv.RequestTraceAttrs("", req) + } +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/common_test.go b/instrumentation/net/http/otelhttp/internal/semconv/common_test.go new file mode 100644 index 00000000000..904b774ccb2 --- /dev/null +++ b/instrumentation/net/http/otelhttp/internal/semconv/common_test.go @@ -0,0 +1,67 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package semconv + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/otel/attribute" +) + +type testServerReq struct { + hostname string + serverPort int + peerAddr string + peerPort int + clientIP string +} + +func testTraceRequest(t *testing.T, serv HTTPServer, want func(testServerReq) []attribute.KeyValue) { + t.Helper() + + got := make(chan *http.Request, 1) + handler := func(w http.ResponseWriter, r *http.Request) { + got <- r + close(got) + w.WriteHeader(http.StatusOK) + } + + srv := httptest.NewServer(http.HandlerFunc(handler)) + defer srv.Close() + + srvURL, err := url.Parse(srv.URL) + require.NoError(t, err) + srvPort, err := strconv.ParseInt(srvURL.Port(), 10, 32) + require.NoError(t, err) + + resp, err := srv.Client().Get(srv.URL) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + req := <-got + peer, peerPort := splitHostPort(req.RemoteAddr) + + const user = "alice" + req.SetBasicAuth(user, "pswrd") + + const clientIP = "127.0.0.5" + req.Header.Add("X-Forwarded-For", clientIP) + + srvReq := testServerReq{ + hostname: srvURL.Hostname(), + serverPort: int(srvPort), + peerAddr: peer, + peerPort: peerPort, + clientIP: clientIP, + } + + assert.ElementsMatch(t, want(srvReq), serv.RequestTraceAttrs("", req)) +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go new file mode 100644 index 00000000000..9be3feef29e --- /dev/null +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -0,0 +1,69 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package semconv // import "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" + +import ( + "fmt" + "net/http" + + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" +) + +type ResponseTelemetry struct { + StatusCode int + ReadBytes int64 + ReadError error + WriteBytes int64 + WriteError error +} + +type HTTPServer interface { + // RequestTraceAttrs returns trace attributes for an HTTP request received by a + // server. + // + // The server must be the primary server name if it is known. For example this + // would be the ServerName directive + // (https://httpd.apache.org/docs/2.4/mod/core.html#servername) for an Apache + // server, and the server_name directive + // (http://nginx.org/en/docs/http/ngx_http_core_module.html#server_name) for an + // nginx server. More generically, the primary server name would be the host + // header value that matches the default virtual host of an HTTP server. It + // should include the host identifier and if a port is used to route to the + // server that port identifier should be included as an appropriate port + // suffix. + // + // If the primary server name is not known, server should be an empty string. + // The req Host will be used to determine the server instead. + RequestTraceAttrs(server string, req *http.Request) []attribute.KeyValue + + // ResponseTraceAttrs returns trace attributes for telemetry from an HTTP response. + // + // If any of the fields in the ResponseTelemetry are not set the attribute will be omitted. + ResponseTraceAttrs(ResponseTelemetry) []attribute.KeyValue + + // Route returns the attribute for the route. + Route(string) attribute.KeyValue +} + +// var warnOnce = sync.Once{} + +func NewHTTPServer() HTTPServer { + // TODO (#5331): Detect version based on environment variable OTEL_HTTP_CLIENT_COMPATIBILITY_MODE. + // TODO (#5331): Add warning of use of a deprecated version of Semantic Versions. + return oldHTTPServer{} +} + +// ServerStatus 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) { + if code < 100 || code >= 600 { + return codes.Error, fmt.Sprintf("Invalid HTTP status code %d", code) + } + if code >= 500 { + return codes.Error, "" + } + return codes.Unset, "" +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/util.go b/instrumentation/net/http/otelhttp/internal/semconv/util.go new file mode 100644 index 00000000000..c92076bc3d9 --- /dev/null +++ b/instrumentation/net/http/otelhttp/internal/semconv/util.go @@ -0,0 +1,49 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package semconv // import "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" + +import ( + "net" + "strconv" + "strings" +) + +// splitHostPort splits a network address hostport of the form "host", +// "host%zone", "[host]", "[host%zone], "host:port", "host%zone:port", +// "[host]:port", "[host%zone]:port", or ":port" into host or host%zone and +// port. +// +// An empty host is returned if it is not provided or unparsable. A negative +// port is returned if it is not provided or unparsable. +func splitHostPort(hostport string) (host string, port int) { + port = -1 + + if strings.HasPrefix(hostport, "[") { + addrEnd := strings.LastIndex(hostport, "]") + if addrEnd < 0 { + // Invalid hostport. + return + } + if i := strings.LastIndex(hostport[addrEnd:], ":"); i < 0 { + host = hostport[1:addrEnd] + return + } + } else { + if i := strings.LastIndex(hostport, ":"); i < 0 { + host = hostport + return + } + } + + host, pStr, err := net.SplitHostPort(hostport) + if err != nil { + return + } + + p, err := strconv.ParseUint(pStr, 10, 16) + if err != nil { + return + } + return host, int(p) +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/util_test.go b/instrumentation/net/http/otelhttp/internal/semconv/util_test.go new file mode 100644 index 00000000000..c5310f90d80 --- /dev/null +++ b/instrumentation/net/http/otelhttp/internal/semconv/util_test.go @@ -0,0 +1,41 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package semconv + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSplitHostPort(t *testing.T) { + tests := []struct { + hostport string + host string + port int + }{ + {"", "", -1}, + {":8080", "", 8080}, + {"127.0.0.1", "127.0.0.1", -1}, + {"www.example.com", "www.example.com", -1}, + {"127.0.0.1%25en0", "127.0.0.1%25en0", -1}, + {"[]", "", -1}, // Ensure this doesn't panic. + {"[fe80::1", "", -1}, + {"[fe80::1]", "fe80::1", -1}, + {"[fe80::1%25en0]", "fe80::1%25en0", -1}, + {"[fe80::1]:8080", "fe80::1", 8080}, + {"[fe80::1]::", "", -1}, // Too many colons. + {"127.0.0.1:", "127.0.0.1", -1}, + {"127.0.0.1:port", "127.0.0.1", -1}, + {"127.0.0.1:8080", "127.0.0.1", 8080}, + {"www.example.com:8080", "www.example.com", 8080}, + {"127.0.0.1%25en0:8080", "127.0.0.1%25en0", 8080}, + } + + for _, test := range tests { + h, p := splitHostPort(test.hostport) + assert.Equal(t, test.host, h, test.hostport) + assert.Equal(t, test.port, p, test.hostport) + } +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go new file mode 100644 index 00000000000..d753083b7b4 --- /dev/null +++ b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go @@ -0,0 +1,75 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package semconv // import "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" + +import ( + "io" + "net/http" + + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconvutil" + "go.opentelemetry.io/otel/attribute" + semconv "go.opentelemetry.io/otel/semconv/v1.20.0" +) + +type oldHTTPServer struct{} + +var _ HTTPServer = oldHTTPServer{} + +// RequestTraceAttrs returns trace attributes for an HTTP request received by a +// server. +// +// The server must be the primary server name if it is known. For example this +// would be the ServerName directive +// (https://httpd.apache.org/docs/2.4/mod/core.html#servername) for an Apache +// server, and the server_name directive +// (http://nginx.org/en/docs/http/ngx_http_core_module.html#server_name) for an +// nginx server. More generically, the primary server name would be the host +// header value that matches the default virtual host of an HTTP server. It +// should include the host identifier and if a port is used to route to the +// server that port identifier should be included as an appropriate port +// suffix. +// +// If the primary server name is not known, server should be an empty string. +// The req Host will be used to determine the server instead. +func (o oldHTTPServer) RequestTraceAttrs(server string, req *http.Request) []attribute.KeyValue { + return semconvutil.HTTPServerRequest(server, req) +} + +// ResponseTraceAttrs returns trace attributes for telemetry from an HTTP response. +// +// If any of the fields in the ResponseTelemetry are not set the attribute will be omitted. +func (o oldHTTPServer) ResponseTraceAttrs(resp ResponseTelemetry) []attribute.KeyValue { + attributes := []attribute.KeyValue{} + + if resp.ReadBytes > 0 { + attributes = append(attributes, semconv.HTTPRequestContentLength(int(resp.ReadBytes))) + } + if resp.ReadError != nil && resp.ReadError != io.EOF { + // This is not in the semantic conventions, but is historically provided + attributes = append(attributes, attribute.String("http.read_error", resp.ReadError.Error())) + } + if resp.WriteBytes > 0 { + attributes = append(attributes, semconv.HTTPResponseContentLength(int(resp.WriteBytes))) + } + if resp.StatusCode > 0 { + attributes = append(attributes, semconv.HTTPStatusCode(resp.StatusCode)) + } + if resp.WriteError != nil && resp.WriteError != io.EOF { + // This is not in the semantic conventions, but is historically provided + attributes = append(attributes, attribute.String("http.write_error", resp.WriteError.Error())) + } + + return attributes +} + +// Route returns the attribute for the route. +func (o oldHTTPServer) Route(route string) attribute.KeyValue { + return semconv.HTTPRoute(route) +} + +// HTTPStatusCode returns the attribute for the HTTP status code. +// This is a temporary function needed by metrics. This will be removed when MetricsRequest is added. +func HTTPStatusCode(status int) attribute.KeyValue { + return semconv.HTTPStatusCode(status) +} 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 new file mode 100644 index 00000000000..3a84697ea4e --- /dev/null +++ b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0_test.go @@ -0,0 +1,85 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package semconv + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + + "go.opentelemetry.io/otel/attribute" +) + +func TestV120TraceRequest(t *testing.T) { + // Anything but "http" or "http/dup" works. + t.Setenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE", "old") + serv := NewHTTPServer() + want := func(req testServerReq) []attribute.KeyValue { + return []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("http.scheme", "http"), + attribute.String("net.host.name", req.hostname), + attribute.Int("net.host.port", req.serverPort), + attribute.String("net.sock.peer.addr", req.peerAddr), + attribute.Int("net.sock.peer.port", req.peerPort), + attribute.String("user_agent.original", "Go-http-client/1.1"), + attribute.String("http.client_ip", req.clientIP), + attribute.String("net.protocol.version", "1.1"), + attribute.String("http.target", "/"), + } + } + testTraceRequest(t, serv, want) +} + +func TestV120TraceResponse(t *testing.T) { + testCases := []struct { + name string + resp ResponseTelemetry + want []attribute.KeyValue + }{ + { + name: "empty", + resp: ResponseTelemetry{}, + want: nil, + }, + { + name: "no errors", + resp: ResponseTelemetry{ + StatusCode: 200, + ReadBytes: 701, + WriteBytes: 802, + }, + want: []attribute.KeyValue{ + attribute.Int("http.request_content_length", 701), + attribute.Int("http.response_content_length", 802), + attribute.Int("http.status_code", 200), + }, + }, + { + name: "with errors", + resp: ResponseTelemetry{ + StatusCode: 200, + ReadBytes: 701, + ReadError: fmt.Errorf("read error"), + WriteBytes: 802, + WriteError: fmt.Errorf("write error"), + }, + want: []attribute.KeyValue{ + attribute.Int("http.request_content_length", 701), + attribute.String("http.read_error", "read error"), + attribute.Int("http.response_content_length", 802), + attribute.String("http.write_error", "write error"), + attribute.Int("http.status_code", 200), + }, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + got := oldHTTPServer{}.ResponseTraceAttrs(tt.resp) + assert.ElementsMatch(t, tt.want, got) + }) + } +}