HTTP Semconv migration Part1 Server - v1.20.0 support

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)
 

instrumentation/net/http/otelhttp/internal/semconv/common_test.go

@@ -0,0 +1,83 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package semconv
+
+import (
+	"fmt"
+	"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))
+}
+
+func testTraceResponse(t *testing.T, serv HTTPServer, want []attribute.KeyValue) {
+	t.Helper()
+	emptyResp := ResponseTelemetry{}
+	assert.Len(t, serv.ResponseTraceAttrs(emptyResp), 0)
+
+	resp := ResponseTelemetry{
+		StatusCode: 200,
+		ReadBytes:  701,
+		ReadError:  fmt.Errorf("read error"),
+		WriteBytes: 802,
+		WriteError: fmt.Errorf("write error"),
+	}
+	assert.ElementsMatch(t, want, serv.ResponseTraceAttrs(resp))
+}

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, ""
+}

instrumentation/net/http/otelhttp/internal/semconv/httpconv_benchmark_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)
+	}
+}

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)
+}

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)
+	}
+}