From f54fc6c7805342222601f1edfc27d94d642fc9a0 Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Mon, 8 Jan 2024 16:33:23 -0800 Subject: [PATCH 01/10] Add support for http client metrics --- CHANGELOG.md | 7 +- .../internal/semconvutil/httpconv.go | 40 ++++ .../internal/semconvutil/httpconv_test.go | 57 ++++++ .../otelgin/internal/semconvutil/httpconv.go | 40 ++++ .../internal/semconvutil/httpconv_test.go | 57 ++++++ .../otelmux/internal/semconvutil/httpconv.go | 40 ++++ .../internal/semconvutil/httpconv_test.go | 57 ++++++ .../otelecho/internal/semconvutil/httpconv.go | 40 ++++ .../internal/semconvutil/httpconv_test.go | 57 ++++++ .../internal/semconvutil/httpconv.go | 40 ++++ .../internal/semconvutil/httpconv_test.go | 57 ++++++ .../internal/semconvutil/httpconv.go | 40 ++++ .../internal/semconvutil/httpconv_test.go | 57 ++++++ instrumentation/net/http/otelhttp/common.go | 14 +- instrumentation/net/http/otelhttp/handler.go | 12 +- .../otelhttp/internal/semconvutil/httpconv.go | 40 ++++ .../internal/semconvutil/httpconv_test.go | 57 ++++++ .../net/http/otelhttp/test/handler_test.go | 29 ++- .../net/http/otelhttp/test/transport_test.go | 186 ++++++++++++++++++ .../net/http/otelhttp/transport.go | 95 ++++++++- .../net/http/otelhttp/transport_test.go | 46 +++-- internal/shared/semconvutil/httpconv.go.tmpl | 40 ++++ .../shared/semconvutil/httpconv_test.go.tmpl | 57 ++++++ 23 files changed, 1119 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de87a671117..17efe85348e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108) - Add `SDK.Shutdown` method in `"go.opentelemetry.io/contrib/config"`. (#4583) +- Add client metric support to `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) ### Changed @@ -23,9 +24,13 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace` are upgraded to v1.20.0. (#4320) - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace/example` are upgraded to v1.20.0. (#4320) - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/example` are upgraded to v1.20.0. (#4320) -- The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`are upgraded to v1.20.0. (#4320) +- The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are upgraded to v1.20.0. (#4320, #4707) - Updated configuration schema to include `schema_url` for resource definition and `without_type_suffix` and `without_units` for the Prometheus exporter. (#4727) +### Removed + +- Remove `RequestCount`, `RequestContentLength`, `ResponseContentLength`, `ServerLatency` constants from `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) + ### Fixed - Fix `NewServerHandler` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` to correctly set the span status depending on the gRPC status. (#4587) diff --git a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go b/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go index 4a184fec736..0394cadcfbf 100644 --- a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go +++ b/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go @@ -51,6 +51,14 @@ func HTTPClientRequest(req *http.Request) []attribute.KeyValue { return hc.ClientRequest(req) } +// HTTPClientRequestMetrics returns metric attributes for an HTTP request made by a client. +// The following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the +// related values are defined in req: "net.peer.port". +func HTTPClientRequestMetrics(req *http.Request) []attribute.KeyValue { + return hc.ClientRequestMetrics(req) +} + // HTTPClientStatus returns a span status code and message for an HTTP status code // value received by a client. func HTTPClientStatus(code int) (codes.Code, string) { @@ -286,6 +294,38 @@ func (c *httpConv) ClientRequest(req *http.Request) []attribute.KeyValue { return attrs } +// ClientRequestMetrics returns metric attributes for an HTTP request made by a client. The +// following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the related values +// are defined in req: "net.peer.port". +func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue { + /* The following semantic conventions are returned if present: + http.method string + net.peer.name string + net.peer.port int + */ + + n := 2 // method, peer name. + var h string + if req.URL != nil { + h = req.URL.Host + } + peer, p := firstHostPort(h, req.Header.Get("Host")) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) + if port > 0 { + n++ + } + + attrs := make([]attribute.KeyValue, 0, n) + attrs = append(attrs, c.method(req.Method), c.NetConv.PeerName(peer)) + + if port > 0 { + attrs = append(attrs, c.NetConv.PeerPort(port)) + } + + return attrs +} + // ServerRequest returns attributes for an HTTP request received by a server. // // The server must be the primary server name if it is known. For example this diff --git a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go index d36fe489667..ea59b0706ec 100644 --- a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go +++ b/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go @@ -68,6 +68,29 @@ func TestHTTPSClientRequest(t *testing.T) { ) } +func TestHTTPSClientRequestMetrics(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" @@ -105,6 +128,40 @@ func TestHTTPClientRequest(t *testing.T) { ) } +func TestHTTPClientRequestMetrics(t *testing.T) { + const ( + user = "alice" + n = 128 + agent = "Go-http-client/1.1" + ) + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "http", + Host: "127.0.0.1:8080", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + Header: http.Header{ + "User-Agent": []string{agent}, + }, + ContentLength: n, + } + req.SetBasicAuth(user, "pswrd") + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + attribute.Int("net.peer.port", 8080), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequestRequired(t *testing.T) { req := new(http.Request) var got []attribute.KeyValue diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go b/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go index 6ee1dc536a1..bfaf7b6dca1 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go @@ -51,6 +51,14 @@ func HTTPClientRequest(req *http.Request) []attribute.KeyValue { return hc.ClientRequest(req) } +// HTTPClientRequestMetrics returns metric attributes for an HTTP request made by a client. +// The following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the +// related values are defined in req: "net.peer.port". +func HTTPClientRequestMetrics(req *http.Request) []attribute.KeyValue { + return hc.ClientRequestMetrics(req) +} + // HTTPClientStatus returns a span status code and message for an HTTP status code // value received by a client. func HTTPClientStatus(code int) (codes.Code, string) { @@ -286,6 +294,38 @@ func (c *httpConv) ClientRequest(req *http.Request) []attribute.KeyValue { return attrs } +// ClientRequestMetrics returns metric attributes for an HTTP request made by a client. The +// following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the related values +// are defined in req: "net.peer.port". +func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue { + /* The following semantic conventions are returned if present: + http.method string + net.peer.name string + net.peer.port int + */ + + n := 2 // method, peer name. + var h string + if req.URL != nil { + h = req.URL.Host + } + peer, p := firstHostPort(h, req.Header.Get("Host")) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) + if port > 0 { + n++ + } + + attrs := make([]attribute.KeyValue, 0, n) + attrs = append(attrs, c.method(req.Method), c.NetConv.PeerName(peer)) + + if port > 0 { + attrs = append(attrs, c.NetConv.PeerPort(port)) + } + + return attrs +} + // ServerRequest returns attributes for an HTTP request received by a server. // // The server must be the primary server name if it is known. For example this diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go index d36fe489667..ea59b0706ec 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go @@ -68,6 +68,29 @@ func TestHTTPSClientRequest(t *testing.T) { ) } +func TestHTTPSClientRequestMetrics(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" @@ -105,6 +128,40 @@ func TestHTTPClientRequest(t *testing.T) { ) } +func TestHTTPClientRequestMetrics(t *testing.T) { + const ( + user = "alice" + n = 128 + agent = "Go-http-client/1.1" + ) + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "http", + Host: "127.0.0.1:8080", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + Header: http.Header{ + "User-Agent": []string{agent}, + }, + ContentLength: n, + } + req.SetBasicAuth(user, "pswrd") + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + attribute.Int("net.peer.port", 8080), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequestRequired(t *testing.T) { req := new(http.Request) var got []attribute.KeyValue diff --git a/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv.go b/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv.go index 79bf47e7b76..62a7e62e32d 100644 --- a/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv.go +++ b/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv.go @@ -51,6 +51,14 @@ func HTTPClientRequest(req *http.Request) []attribute.KeyValue { return hc.ClientRequest(req) } +// HTTPClientRequestMetrics returns metric attributes for an HTTP request made by a client. +// The following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the +// related values are defined in req: "net.peer.port". +func HTTPClientRequestMetrics(req *http.Request) []attribute.KeyValue { + return hc.ClientRequestMetrics(req) +} + // HTTPClientStatus returns a span status code and message for an HTTP status code // value received by a client. func HTTPClientStatus(code int) (codes.Code, string) { @@ -286,6 +294,38 @@ func (c *httpConv) ClientRequest(req *http.Request) []attribute.KeyValue { return attrs } +// ClientRequestMetrics returns metric attributes for an HTTP request made by a client. The +// following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the related values +// are defined in req: "net.peer.port". +func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue { + /* The following semantic conventions are returned if present: + http.method string + net.peer.name string + net.peer.port int + */ + + n := 2 // method, peer name. + var h string + if req.URL != nil { + h = req.URL.Host + } + peer, p := firstHostPort(h, req.Header.Get("Host")) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) + if port > 0 { + n++ + } + + attrs := make([]attribute.KeyValue, 0, n) + attrs = append(attrs, c.method(req.Method), c.NetConv.PeerName(peer)) + + if port > 0 { + attrs = append(attrs, c.NetConv.PeerPort(port)) + } + + return attrs +} + // ServerRequest returns attributes for an HTTP request received by a server. // // The server must be the primary server name if it is known. For example this diff --git a/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv_test.go index d36fe489667..ea59b0706ec 100644 --- a/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv_test.go +++ b/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv_test.go @@ -68,6 +68,29 @@ func TestHTTPSClientRequest(t *testing.T) { ) } +func TestHTTPSClientRequestMetrics(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" @@ -105,6 +128,40 @@ func TestHTTPClientRequest(t *testing.T) { ) } +func TestHTTPClientRequestMetrics(t *testing.T) { + const ( + user = "alice" + n = 128 + agent = "Go-http-client/1.1" + ) + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "http", + Host: "127.0.0.1:8080", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + Header: http.Header{ + "User-Agent": []string{agent}, + }, + ContentLength: n, + } + req.SetBasicAuth(user, "pswrd") + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + attribute.Int("net.peer.port", 8080), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequestRequired(t *testing.T) { req := new(http.Request) var got []attribute.KeyValue diff --git a/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv.go b/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv.go index f2b2b86dafe..0ad79ce6a8b 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv.go +++ b/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv.go @@ -51,6 +51,14 @@ func HTTPClientRequest(req *http.Request) []attribute.KeyValue { return hc.ClientRequest(req) } +// HTTPClientRequestMetrics returns metric attributes for an HTTP request made by a client. +// The following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the +// related values are defined in req: "net.peer.port". +func HTTPClientRequestMetrics(req *http.Request) []attribute.KeyValue { + return hc.ClientRequestMetrics(req) +} + // HTTPClientStatus returns a span status code and message for an HTTP status code // value received by a client. func HTTPClientStatus(code int) (codes.Code, string) { @@ -286,6 +294,38 @@ func (c *httpConv) ClientRequest(req *http.Request) []attribute.KeyValue { return attrs } +// ClientRequestMetrics returns metric attributes for an HTTP request made by a client. The +// following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the related values +// are defined in req: "net.peer.port". +func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue { + /* The following semantic conventions are returned if present: + http.method string + net.peer.name string + net.peer.port int + */ + + n := 2 // method, peer name. + var h string + if req.URL != nil { + h = req.URL.Host + } + peer, p := firstHostPort(h, req.Header.Get("Host")) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) + if port > 0 { + n++ + } + + attrs := make([]attribute.KeyValue, 0, n) + attrs = append(attrs, c.method(req.Method), c.NetConv.PeerName(peer)) + + if port > 0 { + attrs = append(attrs, c.NetConv.PeerPort(port)) + } + + return attrs +} + // ServerRequest returns attributes for an HTTP request received by a server. // // The server must be the primary server name if it is known. For example this diff --git a/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv_test.go index d36fe489667..ea59b0706ec 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv_test.go @@ -68,6 +68,29 @@ func TestHTTPSClientRequest(t *testing.T) { ) } +func TestHTTPSClientRequestMetrics(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" @@ -105,6 +128,40 @@ func TestHTTPClientRequest(t *testing.T) { ) } +func TestHTTPClientRequestMetrics(t *testing.T) { + const ( + user = "alice" + n = 128 + agent = "Go-http-client/1.1" + ) + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "http", + Host: "127.0.0.1:8080", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + Header: http.Header{ + "User-Agent": []string{agent}, + }, + ContentLength: n, + } + req.SetBasicAuth(user, "pswrd") + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + attribute.Int("net.peer.port", 8080), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequestRequired(t *testing.T) { req := new(http.Request) var got []attribute.KeyValue diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv.go index 53a85e74fba..9e8cfe84d14 100644 --- a/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv.go +++ b/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv.go @@ -51,6 +51,14 @@ func HTTPClientRequest(req *http.Request) []attribute.KeyValue { return hc.ClientRequest(req) } +// HTTPClientRequestMetrics returns metric attributes for an HTTP request made by a client. +// The following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the +// related values are defined in req: "net.peer.port". +func HTTPClientRequestMetrics(req *http.Request) []attribute.KeyValue { + return hc.ClientRequestMetrics(req) +} + // HTTPClientStatus returns a span status code and message for an HTTP status code // value received by a client. func HTTPClientStatus(code int) (codes.Code, string) { @@ -286,6 +294,38 @@ func (c *httpConv) ClientRequest(req *http.Request) []attribute.KeyValue { return attrs } +// ClientRequestMetrics returns metric attributes for an HTTP request made by a client. The +// following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the related values +// are defined in req: "net.peer.port". +func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue { + /* The following semantic conventions are returned if present: + http.method string + net.peer.name string + net.peer.port int + */ + + n := 2 // method, peer name. + var h string + if req.URL != nil { + h = req.URL.Host + } + peer, p := firstHostPort(h, req.Header.Get("Host")) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) + if port > 0 { + n++ + } + + attrs := make([]attribute.KeyValue, 0, n) + attrs = append(attrs, c.method(req.Method), c.NetConv.PeerName(peer)) + + if port > 0 { + attrs = append(attrs, c.NetConv.PeerPort(port)) + } + + return attrs +} + // ServerRequest returns attributes for an HTTP request received by a server. // // The server must be the primary server name if it is known. For example this diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv_test.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv_test.go index d36fe489667..ea59b0706ec 100644 --- a/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv_test.go +++ b/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv_test.go @@ -68,6 +68,29 @@ func TestHTTPSClientRequest(t *testing.T) { ) } +func TestHTTPSClientRequestMetrics(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" @@ -105,6 +128,40 @@ func TestHTTPClientRequest(t *testing.T) { ) } +func TestHTTPClientRequestMetrics(t *testing.T) { + const ( + user = "alice" + n = 128 + agent = "Go-http-client/1.1" + ) + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "http", + Host: "127.0.0.1:8080", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + Header: http.Header{ + "User-Agent": []string{agent}, + }, + ContentLength: n, + } + req.SetBasicAuth(user, "pswrd") + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + attribute.Int("net.peer.port", 8080), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequestRequired(t *testing.T) { req := new(http.Request) var got []attribute.KeyValue diff --git a/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv.go b/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv.go index 1049ed94082..b5a0831f35c 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv.go @@ -51,6 +51,14 @@ func HTTPClientRequest(req *http.Request) []attribute.KeyValue { return hc.ClientRequest(req) } +// HTTPClientRequestMetrics returns metric attributes for an HTTP request made by a client. +// The following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the +// related values are defined in req: "net.peer.port". +func HTTPClientRequestMetrics(req *http.Request) []attribute.KeyValue { + return hc.ClientRequestMetrics(req) +} + // HTTPClientStatus returns a span status code and message for an HTTP status code // value received by a client. func HTTPClientStatus(code int) (codes.Code, string) { @@ -286,6 +294,38 @@ func (c *httpConv) ClientRequest(req *http.Request) []attribute.KeyValue { return attrs } +// ClientRequestMetrics returns metric attributes for an HTTP request made by a client. The +// following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the related values +// are defined in req: "net.peer.port". +func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue { + /* The following semantic conventions are returned if present: + http.method string + net.peer.name string + net.peer.port int + */ + + n := 2 // method, peer name. + var h string + if req.URL != nil { + h = req.URL.Host + } + peer, p := firstHostPort(h, req.Header.Get("Host")) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) + if port > 0 { + n++ + } + + attrs := make([]attribute.KeyValue, 0, n) + attrs = append(attrs, c.method(req.Method), c.NetConv.PeerName(peer)) + + if port > 0 { + attrs = append(attrs, c.NetConv.PeerPort(port)) + } + + return attrs +} + // ServerRequest returns attributes for an HTTP request received by a server. // // The server must be the primary server name if it is known. For example this diff --git a/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv_test.go b/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv_test.go index d36fe489667..ea59b0706ec 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv_test.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv_test.go @@ -68,6 +68,29 @@ func TestHTTPSClientRequest(t *testing.T) { ) } +func TestHTTPSClientRequestMetrics(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" @@ -105,6 +128,40 @@ func TestHTTPClientRequest(t *testing.T) { ) } +func TestHTTPClientRequestMetrics(t *testing.T) { + const ( + user = "alice" + n = 128 + agent = "Go-http-client/1.1" + ) + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "http", + Host: "127.0.0.1:8080", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + Header: http.Header{ + "User-Agent": []string{agent}, + }, + ContentLength: n, + } + req.SetBasicAuth(user, "pswrd") + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + attribute.Int("net.peer.port", 8080), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequestRequired(t *testing.T) { req := new(http.Request) var got []attribute.KeyValue diff --git a/instrumentation/net/http/otelhttp/common.go b/instrumentation/net/http/otelhttp/common.go index 9509014e87c..cabf645a5b5 100644 --- a/instrumentation/net/http/otelhttp/common.go +++ b/instrumentation/net/http/otelhttp/common.go @@ -31,10 +31,16 @@ const ( // Server HTTP metrics. const ( - RequestCount = "http.server.request_count" // Incoming request count total - RequestContentLength = "http.server.request_content_length" // Incoming request bytes total - ResponseContentLength = "http.server.response_content_length" // Incoming response bytes total - ServerLatency = "http.server.duration" // Incoming end to end duration, milliseconds + 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 + 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 diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go index af84f0e4bb5..3d292dab6d3 100644 --- a/instrumentation/net/http/otelhttp/handler.go +++ b/instrumentation/net/http/otelhttp/handler.go @@ -108,23 +108,23 @@ func handleErr(err error) { func (h *middleware) createMeasures() { var err error h.requestBytesCounter, err = h.meter.Int64Counter( - RequestContentLength, + serverRequestSize, metric.WithUnit("By"), - metric.WithDescription("Measures the size of HTTP request content length (uncompressed)"), + metric.WithDescription("Measures the size of HTTP request messages."), ) handleErr(err) h.responseBytesCounter, err = h.meter.Int64Counter( - ResponseContentLength, + serverResponseSize, metric.WithUnit("By"), - metric.WithDescription("Measures the size of HTTP response content length (uncompressed)"), + metric.WithDescription("Measures the size of HTTP response messages."), ) handleErr(err) h.serverLatencyMeasure, err = h.meter.Float64Histogram( - ServerLatency, + serverDuration, metric.WithUnit("ms"), - metric.WithDescription("Measures the duration of HTTP request handling"), + metric.WithDescription("Measures the duration of inbound HTTP requests."), ) handleErr(err) } diff --git a/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go b/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go index 794d4c26a45..f4e496bf4bd 100644 --- a/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go +++ b/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go @@ -51,6 +51,14 @@ func HTTPClientRequest(req *http.Request) []attribute.KeyValue { return hc.ClientRequest(req) } +// HTTPClientRequestMetrics returns metric attributes for an HTTP request made by a client. +// The following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the +// related values are defined in req: "net.peer.port". +func HTTPClientRequestMetrics(req *http.Request) []attribute.KeyValue { + return hc.ClientRequestMetrics(req) +} + // HTTPClientStatus returns a span status code and message for an HTTP status code // value received by a client. func HTTPClientStatus(code int) (codes.Code, string) { @@ -286,6 +294,38 @@ func (c *httpConv) ClientRequest(req *http.Request) []attribute.KeyValue { return attrs } +// ClientRequestMetrics returns metric attributes for an HTTP request made by a client. The +// following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the related values +// are defined in req: "net.peer.port". +func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue { + /* The following semantic conventions are returned if present: + http.method string + net.peer.name string + net.peer.port int + */ + + n := 2 // method, peer name. + var h string + if req.URL != nil { + h = req.URL.Host + } + peer, p := firstHostPort(h, req.Header.Get("Host")) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) + if port > 0 { + n++ + } + + attrs := make([]attribute.KeyValue, 0, n) + attrs = append(attrs, c.method(req.Method), c.NetConv.PeerName(peer)) + + if port > 0 { + attrs = append(attrs, c.NetConv.PeerPort(port)) + } + + return attrs +} + // ServerRequest returns attributes for an HTTP request received by a server. // // The server must be the primary server name if it is known. For example this diff --git a/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv_test.go index d36fe489667..ea59b0706ec 100644 --- a/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv_test.go @@ -68,6 +68,29 @@ func TestHTTPSClientRequest(t *testing.T) { ) } +func TestHTTPSClientRequestMetrics(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" @@ -105,6 +128,40 @@ func TestHTTPClientRequest(t *testing.T) { ) } +func TestHTTPClientRequestMetrics(t *testing.T) { + const ( + user = "alice" + n = 128 + agent = "Go-http-client/1.1" + ) + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "http", + Host: "127.0.0.1:8080", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + Header: http.Header{ + "User-Agent": []string{agent}, + }, + ContentLength: n, + } + req.SetBasicAuth(user, "pswrd") + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + attribute.Int("net.peer.port", 8080), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequestRequired(t *testing.T) { req := new(http.Request) var got []attribute.KeyValue diff --git a/instrumentation/net/http/otelhttp/test/handler_test.go b/instrumentation/net/http/otelhttp/test/handler_test.go index bfda5ac83fb..541d0d96641 100644 --- a/instrumentation/net/http/otelhttp/test/handler_test.go +++ b/instrumentation/net/http/otelhttp/test/handler_test.go @@ -50,8 +50,8 @@ func assertScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribut require.Len(t, sm.Metrics, 3) want := metricdata.Metrics{ - Name: "http.server.request_content_length", - Description: "Measures the size of HTTP request content length (uncompressed)", + Name: "http.server.request.size", + Description: "Measures the size of HTTP request messages.", Unit: "By", Data: metricdata.Sum[int64]{ DataPoints: []metricdata.DataPoint[int64]{{Attributes: attrs, Value: 0}}, @@ -62,8 +62,8 @@ func assertScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribut metricdatatest.AssertEqual(t, want, sm.Metrics[0], metricdatatest.IgnoreTimestamp()) want = metricdata.Metrics{ - Name: "http.server.response_content_length", - Description: "Measures the size of HTTP response content length (uncompressed)", + Name: "http.server.response.size", + Description: "Measures the size of HTTP response messages.", Unit: "By", Data: metricdata.Sum[int64]{ DataPoints: []metricdata.DataPoint[int64]{{Attributes: attrs, Value: 11}}, @@ -73,17 +73,16 @@ func assertScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribut } metricdatatest.AssertEqual(t, want, sm.Metrics[1], metricdatatest.IgnoreTimestamp()) - // Duration value is not predictable. - dur := sm.Metrics[2] - assert.Equal(t, "http.server.duration", dur.Name) - require.IsType(t, dur.Data, metricdata.Histogram[float64]{}) - hist := dur.Data.(metricdata.Histogram[float64]) - assert.Equal(t, metricdata.CumulativeTemporality, hist.Temporality) - require.Len(t, hist.DataPoints, 1) - dPt := hist.DataPoints[0] - assert.Equal(t, attrs, dPt.Attributes, "attributes") - assert.Equal(t, uint64(1), dPt.Count, "count") - assert.Equal(t, []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, dPt.Bounds, "bounds") + want = metricdata.Metrics{ + Name: "http.server.duration", + Description: "Measures the duration of inbound HTTP requests.", + Unit: "ms", + Data: metricdata.Histogram[float64]{ + DataPoints: []metricdata.HistogramDataPoint[float64]{{Attributes: attrs}}, + Temporality: metricdata.CumulativeTemporality, + }, + } + metricdatatest.AssertEqual(t, want, sm.Metrics[2], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue()) } func TestHandlerBasics(t *testing.T) { diff --git a/instrumentation/net/http/otelhttp/test/transport_test.go b/instrumentation/net/http/otelhttp/test/transport_test.go index e9a62034aeb..539b06d6833 100644 --- a/instrumentation/net/http/otelhttp/test/transport_test.go +++ b/instrumentation/net/http/otelhttp/test/transport_test.go @@ -15,18 +15,29 @@ package test import ( + "bytes" "context" "io" + "net" "net/http" "net/http/httptest" "net/http/httptrace" "runtime" + "strconv" "strings" "testing" + "go.opentelemetry.io/otel/sdk/metric" + semconv "go.opentelemetry.io/otel/semconv/v1.20.0" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/sdk/instrumentation" + "go.opentelemetry.io/otel/sdk/metric/metricdata" + "go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest" + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/propagation" @@ -238,3 +249,178 @@ func TestWithHTTPTrace(t *testing.T) { assert.Equal(t, spans[2].SpanContext().SpanID(), spans[0].Parent().SpanID()) assert.Equal(t, spans[1].SpanContext().SpanID(), spans[2].Parent().SpanID()) } + +func TestTransportMetrics(t *testing.T) { + requestBody := []byte("john") + responseBody := []byte("Hello, world!") + + t.Run("make http request and read entire response at once", func(t *testing.T) { + reader := metric.NewManualReader() + meterProvider := metric.NewMeterProvider(metric.WithReader(reader)) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + if _, err := w.Write(responseBody); err != nil { + t.Fatal(err) + } + })) + defer ts.Close() + + r, err := http.NewRequest(http.MethodGet, ts.URL, bytes.NewReader(requestBody)) + if err != nil { + t.Fatal(err) + } + + tr := otelhttp.NewTransport( + http.DefaultTransport, + otelhttp.WithMeterProvider(meterProvider), + ) + + c := http.Client{Transport: tr} + res, err := c.Do(r) + if err != nil { + t.Fatal(err) + } + + // Must read the body or else we won't get response metrics + bodyBytes, err := io.ReadAll(res.Body) + if err != nil { + t.Fatal(err) + } + require.Len(t, bodyBytes, 13) + require.NoError(t, res.Body.Close()) + + host, portStr, _ := net.SplitHostPort(r.Host) + if host == "" { + host = "127.0.0.1" + } + port, err := strconv.Atoi(portStr) + if err != nil { + port = 0 + } + + rm := metricdata.ResourceMetrics{} + err = reader.Collect(context.Background(), &rm) + require.NoError(t, err) + require.Len(t, rm.ScopeMetrics, 1) + attrs := attribute.NewSet( + semconv.NetPeerName(host), + semconv.NetPeerPort(port), + semconv.HTTPMethod("GET"), + semconv.HTTPStatusCode(200), + ) + assertClientScopeMetrics(t, rm.ScopeMetrics[0], attrs) + }) + + t.Run("make http request and buffer response", func(t *testing.T) { + reader := metric.NewManualReader() + meterProvider := metric.NewMeterProvider(metric.WithReader(reader)) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + if _, err := w.Write(responseBody); err != nil { + t.Fatal(err) + } + })) + defer ts.Close() + + r, err := http.NewRequest(http.MethodGet, ts.URL, bytes.NewReader(requestBody)) + if err != nil { + t.Fatal(err) + } + + tr := otelhttp.NewTransport( + http.DefaultTransport, + otelhttp.WithMeterProvider(meterProvider), + ) + + c := http.Client{Transport: tr} + res, err := c.Do(r) + if err != nil { + t.Fatal(err) + } + + // Must read the body or else we won't get response metrics + smallBuf := make([]byte, 10) + + // Read first 10 bytes + bc, err := res.Body.Read(smallBuf) + if err != nil { + t.Fatal(err) + } + require.Equal(t, 10, bc) + + // reset byte array + // Read last 3 bytes + bc, err = res.Body.Read(smallBuf) + require.Equal(t, io.EOF, err) + require.Equal(t, 3, bc) + + require.NoError(t, res.Body.Close()) + + host, portStr, _ := net.SplitHostPort(r.Host) + if host == "" { + host = "127.0.0.1" + } + port, err := strconv.Atoi(portStr) + if err != nil { + port = 0 + } + + rm := metricdata.ResourceMetrics{} + err = reader.Collect(context.Background(), &rm) + require.NoError(t, err) + require.Len(t, rm.ScopeMetrics, 1) + attrs := attribute.NewSet( + semconv.NetPeerName(host), + semconv.NetPeerPort(port), + semconv.HTTPMethod("GET"), + semconv.HTTPStatusCode(200), + ) + assertClientScopeMetrics(t, rm.ScopeMetrics[0], attrs) + }) +} + +func assertClientScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribute.Set) { + assert.Equal(t, instrumentation.Scope{ + Name: "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp", + Version: Version(), + }, sm.Scope) + + require.Len(t, sm.Metrics, 3) + + want := metricdata.Metrics{ + Name: "http.client.request.size", + Data: metricdata.Sum[int64]{ + DataPoints: []metricdata.DataPoint[int64]{{Attributes: attrs, Value: 4}}, + Temporality: metricdata.CumulativeTemporality, + IsMonotonic: true, + }, + Description: "Measures the size of HTTP request messages.", + Unit: "By", + } + metricdatatest.AssertEqual(t, want, sm.Metrics[0], metricdatatest.IgnoreTimestamp()) + + want = metricdata.Metrics{ + Name: "http.client.response.size", + Data: metricdata.Sum[int64]{ + DataPoints: []metricdata.DataPoint[int64]{{Attributes: attrs, Value: 13}}, + Temporality: metricdata.CumulativeTemporality, + IsMonotonic: true, + }, + Description: "Measures the size of HTTP response messages.", + Unit: "By", + } + metricdatatest.AssertEqual(t, want, sm.Metrics[1], metricdatatest.IgnoreTimestamp()) + + want = metricdata.Metrics{ + Name: "http.client.duration", + Data: metricdata.Histogram[float64]{ + DataPoints: []metricdata.HistogramDataPoint[float64]{{Attributes: attrs}}, + Temporality: metricdata.CumulativeTemporality, + }, + Description: "Measures the duration of outbound HTTP requests.", + Unit: "ms", + } + metricdatatest.AssertEqual(t, want, sm.Metrics[2], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue()) +} diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index e835cac12e4..20b9ef285f6 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -19,31 +19,43 @@ import ( "io" "net/http" "net/http/httptrace" + "sync/atomic" + "time" + + "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconvutil" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" + + semconv "go.opentelemetry.io/otel/semconv/v1.20.0" ) // Transport implements the http.RoundTripper interface and wraps -// outbound HTTP(S) requests with a span. +// outbound HTTP(S) requests with a span and enriches it with metrics. type Transport struct { rt http.RoundTripper tracer trace.Tracer + meter metric.Meter propagators propagation.TextMapPropagator spanStartOptions []trace.SpanStartOption filters []Filter spanNameFormatter func(string, *http.Request) string clientTrace func(context.Context) *httptrace.ClientTrace + + requestBytesCounter metric.Int64Counter + responseBytesCounter metric.Int64Counter + latencyMeasure metric.Float64Histogram } var _ http.RoundTripper = &Transport{} // NewTransport wraps the provided http.RoundTripper with one that -// starts a span and injects the span context into the outbound request headers. +// starts a span, injects the span context into the outbound request headers, +// and enriches it with metrics. // // If the provided http.RoundTripper is nil, http.DefaultTransport will be used // as the base http.RoundTripper. @@ -63,12 +75,14 @@ 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 @@ -76,6 +90,30 @@ func (t *Transport) applyConfig(c *config) { t.clientTrace = c.ClientTrace } +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 } @@ -84,6 +122,7 @@ func defaultTransportFormatter(_ string, r *http.Request) string { // before handing the request to the configured base RoundTripper. The created span will // end when the response body is closed or when a read from the body returns io.EOF. func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { + requestStartTime := time.Now() for _, f := range t.filters { if !f(r) { // Simply pass through to the base RoundTripper if a filter rejects the request @@ -109,7 +148,22 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { ctx = httptrace.WithClientTrace(ctx, t.clientTrace(ctx)) } + labeler := &Labeler{} + ctx = injectLabeler(ctx, labeler) + r = r.Clone(ctx) // According to RoundTripper spec, we shouldn't modify the origin request. + + writeRecordFunc := func(int64) {} + var bw bodyWrapper + // if request body is nil or NoBody, we don't want to mutate the body as it + // will affect the identity of it in an unforeseeable way because we assert + // ReadCloser fulfills a certain interface and it is indeed nil or NoBody. + if r.Body != nil && r.Body != http.NoBody { + bw.ReadCloser = r.Body + bw.record = writeRecordFunc + r.Body = &bw + } + span.SetAttributes(semconvutil.HTTPClientRequest(r)...) t.propagators.Inject(ctx, propagation.HeaderCarrier(r.Header)) @@ -121,9 +175,28 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { return res, err } + // metrics + metricAttrs := append(labeler.Get(), semconvutil.HTTPClientRequestMetrics(r)...) + if res.StatusCode > 0 { + metricAttrs = append(metricAttrs, semconv.HTTPStatusCode(res.StatusCode)) + } + o := metric.WithAttributes(metricAttrs...) + t.requestBytesCounter.Add(ctx, bw.read, 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) + } + + // traces span.SetAttributes(semconvutil.HTTPClientResponse(res)...) span.SetStatus(semconvutil.HTTPClientStatus(res.StatusCode)) - res.Body = newWrappedBody(span, res.Body) + + res.Body = newWrappedBody(span, readRecordFunc, res.Body) + + // 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) return res, err } @@ -131,17 +204,17 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { // newWrappedBody returns a new and appropriately scoped *wrappedBody as an // io.ReadCloser. If the passed body implements io.Writer, the returned value // will implement io.ReadWriteCloser. -func newWrappedBody(span trace.Span, body io.ReadCloser) io.ReadCloser { +func newWrappedBody(span trace.Span, record func(n int64), body io.ReadCloser) io.ReadCloser { // The successful protocol switch responses will have a body that // implement an io.ReadWriteCloser. Ensure this interface type continues // to be satisfied if that is the case. if _, ok := body.(io.ReadWriteCloser); ok { - return &wrappedBody{span: span, body: body} + return &wrappedBody{span: span, record: record, body: body} } // Remove the implementation of the io.ReadWriteCloser and only implement // the io.ReadCloser. - return struct{ io.ReadCloser }{&wrappedBody{span: span, body: body}} + return struct{ io.ReadCloser }{&wrappedBody{span: span, record: record, body: body}} } // wrappedBody is the response body type returned by the transport @@ -153,8 +226,10 @@ func newWrappedBody(span trace.Span, body io.ReadCloser) io.ReadCloser { // If the response body implements the io.Writer interface (i.e. for // successful protocol switches), the wrapped body also will. type wrappedBody struct { - span trace.Span - body io.ReadCloser + span trace.Span + record func(n int64) + body io.ReadCloser + read atomic.Int64 } var _ io.ReadWriteCloser = &wrappedBody{} @@ -171,11 +246,15 @@ func (wb *wrappedBody) Write(p []byte) (int, error) { func (wb *wrappedBody) Read(b []byte) (int, error) { n, err := wb.body.Read(b) + // Locally record the number of bytes read + wb.read.Add(int64(n)) switch err { case nil: // nothing to do here but fall through to the return case io.EOF: + // Record the total number of bytes read + wb.record(wb.read.Load()) wb.span.End() default: wb.span.RecordError(err) diff --git a/instrumentation/net/http/otelhttp/transport_test.go b/instrumentation/net/http/otelhttp/transport_test.go index 7530955971b..efa29d5435e 100644 --- a/instrumentation/net/http/otelhttp/transport_test.go +++ b/instrumentation/net/http/otelhttp/transport_test.go @@ -247,52 +247,72 @@ func (s *span) assert(t *testing.T, ended bool, err error, c codes.Code, d strin func TestWrappedBodyRead(t *testing.T) { s := new(span) - wb := &wrappedBody{span: trace.Span(s), body: readCloser{}} + called := false + record := func(numBytes int64) { called = true } + wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{}} n, err := wb.Read([]byte{}) assert.Equal(t, readSize, n, "wrappedBody returned wrong bytes") assert.NoError(t, err) s.assert(t, false, nil, codes.Unset, "") + assert.False(t, called, "record should not have been called") } func TestWrappedBodyReadEOFError(t *testing.T) { s := new(span) - wb := &wrappedBody{span: trace.Span(s), body: readCloser{readErr: io.EOF}} + called := false + numRecorded := int64(0) + record := func(numBytes int64) { + called = true + numRecorded = numBytes + } + wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{readErr: io.EOF}} n, err := wb.Read([]byte{}) assert.Equal(t, readSize, n, "wrappedBody returned wrong bytes") assert.Equal(t, io.EOF, err) s.assert(t, true, nil, codes.Unset, "") + assert.True(t, called, "record should have been called") + assert.Equal(t, int64(readSize), numRecorded, "record recorded wrong number of bytes") } func TestWrappedBodyReadError(t *testing.T) { s := new(span) + called := false + record := func(int64) { called = true } expectedErr := errors.New("test") - wb := &wrappedBody{span: trace.Span(s), body: readCloser{readErr: expectedErr}} + wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{readErr: expectedErr}} n, err := wb.Read([]byte{}) assert.Equal(t, readSize, n, "wrappedBody returned wrong bytes") assert.Equal(t, expectedErr, err) s.assert(t, false, expectedErr, codes.Error, expectedErr.Error()) + assert.False(t, called, "record should not have been called") } func TestWrappedBodyClose(t *testing.T) { s := new(span) - wb := &wrappedBody{span: trace.Span(s), body: readCloser{}} + called := false + record := func(int64) { called = true } + wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{}} assert.NoError(t, wb.Close()) s.assert(t, true, nil, codes.Unset, "") + assert.False(t, called, "record should not have been called") } func TestWrappedBodyClosePanic(t *testing.T) { s := new(span) var body io.ReadCloser - wb := newWrappedBody(s, body) + wb := newWrappedBody(s, func(n int64) {}, body) assert.NotPanics(t, func() { wb.Close() }, "nil body should not panic on close") } func TestWrappedBodyCloseError(t *testing.T) { s := new(span) + called := false + record := func(int64) { called = true } expectedErr := errors.New("test") - wb := &wrappedBody{span: trace.Span(s), body: readCloser{closeErr: expectedErr}} + wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{closeErr: expectedErr}} assert.Equal(t, expectedErr, wb.Close()) s.assert(t, true, nil, codes.Unset, "") + assert.False(t, called, "record should not have been called") } type readWriteCloser struct { @@ -308,12 +328,12 @@ func (rwc readWriteCloser) Write([]byte) (int, error) { } func TestNewWrappedBodyReadWriteCloserImplementation(t *testing.T) { - wb := newWrappedBody(nil, readWriteCloser{}) + wb := newWrappedBody(nil, func(n int64) {}, readWriteCloser{}) assert.Implements(t, (*io.ReadWriteCloser)(nil), wb) } func TestNewWrappedBodyReadCloserImplementation(t *testing.T) { - wb := newWrappedBody(nil, readCloser{}) + wb := newWrappedBody(nil, func(n int64) {}, readCloser{}) assert.Implements(t, (*io.ReadCloser)(nil), wb) _, ok := wb.(io.ReadWriteCloser) @@ -324,7 +344,7 @@ func TestWrappedBodyWrite(t *testing.T) { s := new(span) var rwc io.ReadWriteCloser assert.NotPanics(t, func() { - rwc = newWrappedBody(s, readWriteCloser{}).(io.ReadWriteCloser) + rwc = newWrappedBody(s, func(n int64) {}, readWriteCloser{}).(io.ReadWriteCloser) }) n, err := rwc.Write([]byte{}) @@ -338,9 +358,11 @@ func TestWrappedBodyWriteError(t *testing.T) { expectedErr := errors.New("test") var rwc io.ReadWriteCloser assert.NotPanics(t, func() { - rwc = newWrappedBody(s, readWriteCloser{ - writeErr: expectedErr, - }).(io.ReadWriteCloser) + rwc = newWrappedBody(s, + func(n int64) {}, + readWriteCloser{ + writeErr: expectedErr, + }).(io.ReadWriteCloser) }) n, err := rwc.Write([]byte{}) assert.Equal(t, writeSize, n, "wrappedBody returned wrong bytes") diff --git a/internal/shared/semconvutil/httpconv.go.tmpl b/internal/shared/semconvutil/httpconv.go.tmpl index 8f299896ccb..86ade834224 100644 --- a/internal/shared/semconvutil/httpconv.go.tmpl +++ b/internal/shared/semconvutil/httpconv.go.tmpl @@ -51,6 +51,14 @@ func HTTPClientRequest(req *http.Request) []attribute.KeyValue { return hc.ClientRequest(req) } +// HTTPClientRequestMetrics returns metric attributes for an HTTP request made by a client. +// The following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the +// related values are defined in req: "net.peer.port". +func HTTPClientRequestMetrics(req *http.Request) []attribute.KeyValue { + return hc.ClientRequestMetrics(req) +} + // HTTPClientStatus returns a span status code and message for an HTTP status code // value received by a client. func HTTPClientStatus(code int) (codes.Code, string) { @@ -286,6 +294,38 @@ func (c *httpConv) ClientRequest(req *http.Request) []attribute.KeyValue { return attrs } +// ClientRequestMetrics returns metric attributes for an HTTP request made by a client. The +// following attributes are always returned: "http.method", "net.peer.name". +// The following attributes are returned if the related values +// are defined in req: "net.peer.port". +func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue { + /* The following semantic conventions are returned if present: + http.method string + net.peer.name string + net.peer.port int + */ + + n := 2 // method, peer name. + var h string + if req.URL != nil { + h = req.URL.Host + } + peer, p := firstHostPort(h, req.Header.Get("Host")) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) + if port > 0 { + n++ + } + + attrs := make([]attribute.KeyValue, 0, n) + attrs = append(attrs, c.method(req.Method), c.NetConv.PeerName(peer)) + + if port > 0 { + attrs = append(attrs, c.NetConv.PeerPort(port)) + } + + return attrs +} + // ServerRequest returns attributes for an HTTP request received by a server. // // The server must be the primary server name if it is known. For example this diff --git a/internal/shared/semconvutil/httpconv_test.go.tmpl b/internal/shared/semconvutil/httpconv_test.go.tmpl index d36fe489667..ea59b0706ec 100644 --- a/internal/shared/semconvutil/httpconv_test.go.tmpl +++ b/internal/shared/semconvutil/httpconv_test.go.tmpl @@ -68,6 +68,29 @@ func TestHTTPSClientRequest(t *testing.T) { ) } +func TestHTTPSClientRequestMetrics(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" @@ -105,6 +128,40 @@ func TestHTTPClientRequest(t *testing.T) { ) } +func TestHTTPClientRequestMetrics(t *testing.T) { + const ( + user = "alice" + n = 128 + agent = "Go-http-client/1.1" + ) + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "http", + Host: "127.0.0.1:8080", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + Header: http.Header{ + "User-Agent": []string{agent}, + }, + ContentLength: n, + } + req.SetBasicAuth(user, "pswrd") + + assert.ElementsMatch( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("net.peer.name", "127.0.0.1"), + attribute.Int("net.peer.port", 8080), + }, + HTTPClientRequestMetrics(req), + ) +} + func TestHTTPClientRequestRequired(t *testing.T) { req := new(http.Request) var got []attribute.KeyValue From 0411674549f801f9aa02f2c91a46dc6967854a03 Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Tue, 16 Jan 2024 18:57:59 -0800 Subject: [PATCH 02/10] Record the number of bytes read for partial reads --- .../net/http/otelhttp/test/transport_test.go | 71 +++++++++++++++++-- .../net/http/otelhttp/transport.go | 23 ++++-- 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/instrumentation/net/http/otelhttp/test/transport_test.go b/instrumentation/net/http/otelhttp/test/transport_test.go index 539b06d6833..607ffbd3e9b 100644 --- a/instrumentation/net/http/otelhttp/test/transport_test.go +++ b/instrumentation/net/http/otelhttp/test/transport_test.go @@ -309,7 +309,7 @@ func TestTransportMetrics(t *testing.T) { semconv.HTTPMethod("GET"), semconv.HTTPStatusCode(200), ) - assertClientScopeMetrics(t, rm.ScopeMetrics[0], attrs) + assertClientScopeMetrics(t, rm.ScopeMetrics[0], attrs, 13) }) t.Run("make http request and buffer response", func(t *testing.T) { @@ -377,11 +377,74 @@ func TestTransportMetrics(t *testing.T) { semconv.HTTPMethod("GET"), semconv.HTTPStatusCode(200), ) - assertClientScopeMetrics(t, rm.ScopeMetrics[0], attrs) + assertClientScopeMetrics(t, rm.ScopeMetrics[0], attrs, 13) + }) + + t.Run("make http request and close body before reading completely", func(t *testing.T) { + reader := metric.NewManualReader() + meterProvider := metric.NewMeterProvider(metric.WithReader(reader)) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + if _, err := w.Write(responseBody); err != nil { + t.Fatal(err) + } + })) + defer ts.Close() + + r, err := http.NewRequest(http.MethodGet, ts.URL, bytes.NewReader(requestBody)) + if err != nil { + t.Fatal(err) + } + + tr := otelhttp.NewTransport( + http.DefaultTransport, + otelhttp.WithMeterProvider(meterProvider), + ) + + c := http.Client{Transport: tr} + res, err := c.Do(r) + if err != nil { + t.Fatal(err) + } + + // Must read the body or else we won't get response metrics + smallBuf := make([]byte, 10) + + // Read first 10 bytes + bc, err := res.Body.Read(smallBuf) + if err != nil { + t.Fatal(err) + } + require.Equal(t, 10, bc) + + // close the response body early + require.NoError(t, res.Body.Close()) + + host, portStr, _ := net.SplitHostPort(r.Host) + if host == "" { + host = "127.0.0.1" + } + port, err := strconv.Atoi(portStr) + if err != nil { + port = 0 + } + + rm := metricdata.ResourceMetrics{} + err = reader.Collect(context.Background(), &rm) + require.NoError(t, err) + require.Len(t, rm.ScopeMetrics, 1) + attrs := attribute.NewSet( + semconv.NetPeerName(host), + semconv.NetPeerPort(port), + semconv.HTTPMethod("GET"), + semconv.HTTPStatusCode(200), + ) + assertClientScopeMetrics(t, rm.ScopeMetrics[0], attrs, 10) }) } -func assertClientScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribute.Set) { +func assertClientScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribute.Set, rxBytes int64) { assert.Equal(t, instrumentation.Scope{ Name: "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp", Version: Version(), @@ -404,7 +467,7 @@ func assertClientScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs at want = metricdata.Metrics{ Name: "http.client.response.size", Data: metricdata.Sum[int64]{ - DataPoints: []metricdata.DataPoint[int64]{{Attributes: attrs, Value: 13}}, + DataPoints: []metricdata.DataPoint[int64]{{Attributes: attrs, Value: rxBytes}}, Temporality: metricdata.CumulativeTemporality, IsMonotonic: true, }, diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index 20b9ef285f6..5268bfb723e 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -226,10 +226,11 @@ func newWrappedBody(span trace.Span, record func(n int64), body io.ReadCloser) i // If the response body implements the io.Writer interface (i.e. for // successful protocol switches), the wrapped body also will. type wrappedBody struct { - span trace.Span - record func(n int64) - body io.ReadCloser - read atomic.Int64 + span trace.Span + recorded atomic.Bool + record func(n int64) + body io.ReadCloser + read atomic.Int64 } var _ io.ReadWriteCloser = &wrappedBody{} @@ -246,15 +247,14 @@ func (wb *wrappedBody) Write(p []byte) (int, error) { func (wb *wrappedBody) Read(b []byte) (int, error) { n, err := wb.body.Read(b) - // Locally record the number of bytes read + // Record the number of bytes read wb.read.Add(int64(n)) switch err { case nil: // nothing to do here but fall through to the return case io.EOF: - // Record the total number of bytes read - wb.record(wb.read.Load()) + wb.recordBytesRead() wb.span.End() default: wb.span.RecordError(err) @@ -263,7 +263,16 @@ func (wb *wrappedBody) Read(b []byte) (int, error) { return n, err } +// recordBytesRead is a function that ensures the number of bytes read is recorded once and only once. +func (wb *wrappedBody) recordBytesRead() { + if wb.recorded.CompareAndSwap(false, true) { + // Record the total number of bytes read + wb.record(wb.read.Load()) + } +} + func (wb *wrappedBody) Close() error { + wb.recordBytesRead() wb.span.End() if wb.body != nil { return wb.body.Close() From ba53603781ed6a10b4a91a303183b49c49aff883 Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Tue, 16 Jan 2024 19:10:07 -0800 Subject: [PATCH 03/10] Update additional unit tests --- instrumentation/net/http/otelhttp/transport_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/net/http/otelhttp/transport_test.go b/instrumentation/net/http/otelhttp/transport_test.go index efa29d5435e..f274c2b9ff5 100644 --- a/instrumentation/net/http/otelhttp/transport_test.go +++ b/instrumentation/net/http/otelhttp/transport_test.go @@ -294,7 +294,7 @@ func TestWrappedBodyClose(t *testing.T) { wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{}} assert.NoError(t, wb.Close()) s.assert(t, true, nil, codes.Unset, "") - assert.False(t, called, "record should not have been called") + assert.True(t, called, "record should not have been called") } func TestWrappedBodyClosePanic(t *testing.T) { @@ -312,7 +312,7 @@ func TestWrappedBodyCloseError(t *testing.T) { wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{closeErr: expectedErr}} assert.Equal(t, expectedErr, wb.Close()) s.assert(t, true, nil, codes.Unset, "") - assert.False(t, called, "record should not have been called") + assert.True(t, called, "record should not have been called") } type readWriteCloser struct { From 33c7b2d75de013fd97b5bbe4f9a4529967ed501f Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Fri, 19 Jan 2024 14:15:42 -0800 Subject: [PATCH 04/10] Update CHANGELOG.md --- CHANGELOG.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 802f4ac3320..e0febd6d88f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,13 +11,21 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added - Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108) +- Add client metric support to `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) + +### Changed + +- The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are upgraded to v1.20.0. (#4320, #4707) + +### Removed + +- Remove `RequestCount`, `RequestContentLength`, `ResponseContentLength`, `ServerLatency` constants from `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) ## [1.22.0/0.47.0/0.16.0/0.2.0] - 2024-01-18 ### Added - Add `SDK.Shutdown` method in `"go.opentelemetry.io/contrib/config"`. (#4583) -- Add client metric support to `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) - `NewSDK` in `go.opentelemetry.io/contrib/config` now returns a configured SDK with a valid `TracerProvider`. (#4741) ### Changed @@ -30,7 +38,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace` are upgraded to v1.20.0. (#4320) - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace/example` are upgraded to v1.20.0. (#4320) - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/example` are upgraded to v1.20.0. (#4320) -- The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are upgraded to v1.20.0. (#4320, #4707) - Updated configuration schema to include `schema_url` for resource definition and `without_type_suffix` and `without_units` for the Prometheus exporter. (#4727) - The semantic conventions used by the `go.opentelemetry.io/contrib/detectors/aws/ecs` resource detector are upgraded to v1.24.0. (#4803) - The semantic conventions used by the `go.opentelemetry.io/contrib/detectors/aws/lambda` resource detector are upgraded to v1.24.0. (#4803) @@ -39,10 +46,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The semantic conventions used by the `go.opentelemetry.io/contrib/detectors/gcp` resource detector are upgraded to v1.24.0. (#4803) - The semantic conventions used in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-lambda-go/otellambda/test` are upgraded to v1.24.0. (#4803) -### Removed - -- Remove `RequestCount`, `RequestContentLength`, `ResponseContentLength`, `ServerLatency` constants from `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) - ### Fixed - Fix `NewServerHandler` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` to correctly set the span status depending on the gRPC status. (#4587) From 45bb0dd8d1e7f60c5b2c3cd05a4140b421150fa5 Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Fri, 19 Jan 2024 14:16:27 -0800 Subject: [PATCH 05/10] Update CHANGELOG.md --- CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0febd6d88f..ded7d9629bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,10 +13,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108) - Add client metric support to `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) -### Changed - -- The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are upgraded to v1.20.0. (#4320, #4707) - ### Removed - Remove `RequestCount`, `RequestContentLength`, `ResponseContentLength`, `ServerLatency` constants from `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) @@ -38,6 +34,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace` are upgraded to v1.20.0. (#4320) - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace/example` are upgraded to v1.20.0. (#4320) - The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/example` are upgraded to v1.20.0. (#4320) +- The semantic conventions used by `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`are upgraded to v1.20.0. (#4320) - Updated configuration schema to include `schema_url` for resource definition and `without_type_suffix` and `without_units` for the Prometheus exporter. (#4727) - The semantic conventions used by the `go.opentelemetry.io/contrib/detectors/aws/ecs` resource detector are upgraded to v1.24.0. (#4803) - The semantic conventions used by the `go.opentelemetry.io/contrib/detectors/aws/lambda` resource detector are upgraded to v1.24.0. (#4803) From f41446dcccb9871e983b0a0bd3e8e231deab7f81 Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Mon, 29 Jan 2024 12:32:01 -0800 Subject: [PATCH 06/10] Add clarifying comments --- instrumentation/net/http/otelhttp/transport.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index 5268bfb723e..ff4da1c5fa9 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -153,14 +153,15 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { r = r.Clone(ctx) // According to RoundTripper spec, we shouldn't modify the origin request. - writeRecordFunc := func(int64) {} + // use a body wrapper to determine the request size var bw bodyWrapper // if request body is nil or NoBody, we don't want to mutate the body as it // will affect the identity of it in an unforeseeable way because we assert // ReadCloser fulfills a certain interface and it is indeed nil or NoBody. if r.Body != nil && r.Body != http.NoBody { bw.ReadCloser = r.Body - bw.record = writeRecordFunc + // noop to prevent nil panic. not using this record fun yet. + bw.record = func(int64) {} r.Body = &bw } From 70c1791e8dbdcd7324c5b1c78645b3e4a2b1d695 Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Mon, 29 Jan 2024 12:57:19 -0800 Subject: [PATCH 07/10] Add back removed constants and make note about deprecation --- CHANGELOG.md | 4 ++-- instrumentation/net/http/otelhttp/common.go | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ded7d9629bd..39d79e0834a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108) - Add client metric support to `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) -### Removed +### Deprecated -- Remove `RequestCount`, `RequestContentLength`, `ResponseContentLength`, `ServerLatency` constants from `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#4707) +- The `RequestCount`, `RequestContentLength`, `ResponseContentLength`, `ServerLatency` constants in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are deprecated. (#4707) ## [1.22.0/0.47.0/0.16.0/0.2.0] - 2024-01-18 diff --git a/instrumentation/net/http/otelhttp/common.go b/instrumentation/net/http/otelhttp/common.go index cabf645a5b5..c6f438774f7 100644 --- a/instrumentation/net/http/otelhttp/common.go +++ b/instrumentation/net/http/otelhttp/common.go @@ -31,6 +31,14 @@ const ( // Server HTTP metrics. const ( + // Deprecated: This field is unused. + RequestCount = "http.server.request_count" // Incoming request count total + // Deprecated: Use of this field has been migrated to serverRequestSize. It will be removed in a future version. + RequestContentLength = "http.server.request_content_length" // Incoming request bytes total + // Deprecated: Use of this field has been migrated to serverResponseSize. It will be removed in a future version. + ResponseContentLength = "http.server.response_content_length" // Incoming response bytes total + // Deprecated: Use of this field has been migrated to serverDuration. It will be removed in a future version. + ServerLatency = "http.server.duration" // Incoming end to end duration, milliseconds 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 From 25dc08a6b242acbaf23e0dd3443f33a0e0328b81 Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Thu, 1 Feb 2024 23:13:40 -0800 Subject: [PATCH 08/10] Update instrumentation/net/http/otelhttp/transport_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- instrumentation/net/http/otelhttp/transport_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/net/http/otelhttp/transport_test.go b/instrumentation/net/http/otelhttp/transport_test.go index f274c2b9ff5..d77ad1b9450 100644 --- a/instrumentation/net/http/otelhttp/transport_test.go +++ b/instrumentation/net/http/otelhttp/transport_test.go @@ -294,7 +294,7 @@ func TestWrappedBodyClose(t *testing.T) { wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{}} assert.NoError(t, wb.Close()) s.assert(t, true, nil, codes.Unset, "") - assert.True(t, called, "record should not have been called") + assert.True(t, called, "record should have been called") } func TestWrappedBodyClosePanic(t *testing.T) { From 1251f7ec68bd47fad42645189c5a3f269d93651a Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Thu, 1 Feb 2024 23:13:49 -0800 Subject: [PATCH 09/10] Update instrumentation/net/http/otelhttp/transport_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- instrumentation/net/http/otelhttp/transport_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/net/http/otelhttp/transport_test.go b/instrumentation/net/http/otelhttp/transport_test.go index d77ad1b9450..1fcf76d650e 100644 --- a/instrumentation/net/http/otelhttp/transport_test.go +++ b/instrumentation/net/http/otelhttp/transport_test.go @@ -312,7 +312,7 @@ func TestWrappedBodyCloseError(t *testing.T) { wb := &wrappedBody{span: trace.Span(s), record: record, body: readCloser{closeErr: expectedErr}} assert.Equal(t, expectedErr, wb.Close()) s.assert(t, true, nil, codes.Unset, "") - assert.True(t, called, "record should not have been called") + assert.True(t, called, "record should have been called") } type readWriteCloser struct { From 38861442146dafb02345a3cd06d6bd96030c77f6 Mon Sep 17 00:00:00 2001 From: Jason Parraga Date: Thu, 1 Feb 2024 23:35:15 -0800 Subject: [PATCH 10/10] Add comment abour using atomic.Bool instead of sync.Once --- instrumentation/net/http/otelhttp/transport.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index ff4da1c5fa9..8d850df3baa 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -266,6 +266,10 @@ func (wb *wrappedBody) Read(b []byte) (int, error) { // recordBytesRead is a function that ensures the number of bytes read is recorded once and only once. func (wb *wrappedBody) recordBytesRead() { + // note: it is more performant (and equally correct) to use atomic.Bool over sync.Once here. In the event that + // two goroutines are racing to call this method, the number of bytes read will no longer increase. Using + // CompareAndSwap allows later goroutines to return quickly and not block waiting for the race winner to finish + // calling wb.record(wb.read.Load()). if wb.recorded.CompareAndSwap(false, true) { // Record the total number of bytes read wb.record(wb.read.Load())