From 800942f3ae5a5cada06aa407dec8025a8802c084 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Fri, 28 Feb 2025 15:40:33 +0100 Subject: [PATCH 01/17] first draft for otelhttpconv design document --- .../net/http/otelhttpconv/DESIGN.md | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 instrumentation/net/http/otelhttpconv/DESIGN.md diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md new file mode 100644 index 00000000000..a5045abc77e --- /dev/null +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -0,0 +1,111 @@ +# OTel HTTP Conv + +## Motivation + +We provide many instrumentation libraries for HTTP packages and frameworks. +Many of them need to reimplement similar features, as they have their own +internals (they don't all allow using `net/http.Handler`). +This is causing a lot of duplication across instrumentations, and makes +standardization harder. + +Folks have also expressed interest in being able to use the internal tools +provided by the internal `semconvutil` package within their own +instrumentations ([#4580](https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4580)). +This package aims to solve that by being publicly usable. + +This document outlines a proposal for a new instrumentation package called +`otelhttpconv` that will provide a way to reduce some of this code repetition, +especially with the setup of spans and metrics and their attributes. + +The goal of this document is also to make future semantic convention migrations +easier, and to allow flexibility with the use or not of unstable +attributes/metrics. + +## Design + +The proposed design aims to: + +* Work with every official instrumentation, and be available to external implementors. +* Provide flexibility in its use, to allow folks the use of unstable semantic conventions if they wish. + +### Interfaces + +We will provide two public interface that allow interacting with the +instrumentation package. +One for client. and one for servers. Implementations can use one, the other or both. + +#### The client + +```golang +// Client provides an interface for HTTP Client instrumentations to set the +// proper semantic convention attributes and metrics into their data. +type Client interface { + // RecordError records an error returned by the HTTP request. + RecordError(error) + + // RecordMetrics records the metrics from the provided HTTP request. + RecordMetrics(ctx context.Context, w *ResponseWrapper) + + // SpanAttributes sets the span attributes based on the HTTP request and response + SpanAttributes(ctx context.Context, req *http.Request, resp *http.Response) +} + +// ClientRecordMetricsOption applies options to the RecordMetrics method +type ClientRecordMetricsOption interface{} + +// RequestWrapper provides a layer on top of `*http.Response` that tracks +// additional information such as bytes read etc. +type ResponseWrapper interface { + // Duration contains the duration of the HTTP request + Duration() time.Duration + + // Duration contains the status code returned by the HTTP request + StatusCode() int + + // BytesRead contains the amount of bytes read from the response's body + BytesRead() int64 +} +``` + +#### The Server + +```golang + +``` + +#### Request and Response wrappers + +The request and response wrappers implementations will be provided by the +[internal/request](https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/request) +templates package. + +We may provide a new package, or actual implementations within the +`otelhttpconv` package later on. But doing so is not covered by this design +document at the moment. + +### Implementations + +We will provide one official implementation of the described interfaces. +This implementation will have the following requirements: + +* Support for the latest semantic conventions only. +* Support for stable metrics and attributes only. + + +We may provide additional implementations later on such as: + +* An implementation that serves as a proxy to allow combining multiple implementations together. +* An implementation that covers unstable metrics and attributes only. + +### Usage + +Instrumentations should use the official implementation mentioned above by +default. +They may provide an option for their users to override the used implementation +with a custom one. + +For example, with the `otelhttp` instrumentation for clients: + +```golang + otelhttp.NewTransport(http.DefaultTransport, otelhttp.WithHTTPConv(myCustomImplementation{})) +``` From e7e8c880153a2c4320c70d220982f7b40d3e2f88 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Fri, 28 Feb 2025 15:47:47 +0100 Subject: [PATCH 02/17] fix typo --- instrumentation/net/http/otelhttpconv/DESIGN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index a5045abc77e..f6899093c02 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -25,7 +25,7 @@ attributes/metrics. The proposed design aims to: -* Work with every official instrumentation, and be available to external implementors. +* Work with every official instrumentation, and be available to external implementers. * Provide flexibility in its use, to allow folks the use of unstable semantic conventions if they wish. ### Interfaces From 87a12f67a6fcb27eb7125f8b670ed87dc882448f Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 4 Mar 2025 12:12:25 +0100 Subject: [PATCH 03/17] add server design --- .../net/http/otelhttpconv/DESIGN.md | 78 +++++++++++++++---- 1 file changed, 61 insertions(+), 17 deletions(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index f6899093c02..00d01f30425 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -34,25 +34,9 @@ We will provide two public interface that allow interacting with the instrumentation package. One for client. and one for servers. Implementations can use one, the other or both. -#### The client +#### Shared between client and server ```golang -// Client provides an interface for HTTP Client instrumentations to set the -// proper semantic convention attributes and metrics into their data. -type Client interface { - // RecordError records an error returned by the HTTP request. - RecordError(error) - - // RecordMetrics records the metrics from the provided HTTP request. - RecordMetrics(ctx context.Context, w *ResponseWrapper) - - // SpanAttributes sets the span attributes based on the HTTP request and response - SpanAttributes(ctx context.Context, req *http.Request, resp *http.Response) -} - -// ClientRecordMetricsOption applies options to the RecordMetrics method -type ClientRecordMetricsOption interface{} - // RequestWrapper provides a layer on top of `*http.Response` that tracks // additional information such as bytes read etc. type ResponseWrapper interface { @@ -67,12 +51,72 @@ type ResponseWrapper interface { } ``` +#### The client + +```golang +// Client provides an interface for HTTP client instrumentations to set the +// proper semantic convention attributes and metrics into their data. +type Client interface { + // RecordError records an error returned by the HTTP request. + RecordError(error) + + // RecordMetrics records the metrics from the provided HTTP request. + RecordMetrics(ctx context.Context, w *ResponseWrapper, ...ClientRecordMetricsOption) + + // RecordSpan sets the span attributes and status code based on the HTTP + // request and response. + // This method does not create a new span. It retrieves the current one from + // the context. + // It remains the instrumentation's responsibility to start and end spans. + RecordSpan(ctx context.Context, req *http.Request, resp *http.Response) +} + +// ClientRecordMetricsOption applies options to the RecordMetrics method +type ClientRecordMetricsOption interface{} +``` + +The `ClientRecordMetricsOption` allows passing optional parameters such as +status code, additional attributes, duration of the request, ... +Those options will be used to generate the appropriate metrics. If they are not +provided, no metric will be generated with the empty data. + #### The Server ```golang +// Server provides an interface for HTTP server instrumentations to set the +// proper semantic convention attributes and metrics into their data. +type Server interface { + // RecordMetrics records the metrics from the provided HTTP request. + RecordMetrics(ctx context.Context, w *ResponseWrapper) + + // RecordSpan sets the span attributes and status code based on the HTTP + // request and response. + // This method does not create a new span. It retrieves the current one from + // the context. + // It remains the instrumentation's responsibility to start and end spans. + RecordSpan(ctx context.Context, req *http.Request, resp *http.Response, ...ServerRecordSpanOptions) +} + +// ClientRecordMetricsOption applies options to the RecordMetrics method +type ClientRecordMetricsOption interface{} + +// ServerRecordSpanOption applies options to the RecordSpan method +type ServerRecordSpanOption interface{} ``` +The `ServerRecordMetricsOption` and `ServerRecordSpanOption` functional options +allows passing optional parameters. + +Those options can be: + +* Status Code, additional attributes, duration of the request for metrics +* HTTP Route for the span + +When the data those options provide is not specified, no telemetry will be +emitted (the metrics will not be emitted, and the span attributes will not be +set). + #### Request and Response wrappers The request and response wrappers implementations will be provided by the From cdd2a5237c88454027d4fc357b61a95f413bb812 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 4 Mar 2025 12:17:30 +0100 Subject: [PATCH 04/17] fix indentation --- instrumentation/net/http/otelhttpconv/DESIGN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index 00d01f30425..e1408fae212 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -151,5 +151,5 @@ with a custom one. For example, with the `otelhttp` instrumentation for clients: ```golang - otelhttp.NewTransport(http.DefaultTransport, otelhttp.WithHTTPConv(myCustomImplementation{})) +otelhttp.NewTransport(http.DefaultTransport, otelhttp.WithHTTPConv(myCustomImplementation{})) ``` From 4895dc2b52574084b88d9cf92e00db5e93388300 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 4 Mar 2025 14:15:13 +0100 Subject: [PATCH 05/17] some copy editing --- .../net/http/otelhttpconv/DESIGN.md | 38 ++++++------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index e1408fae212..2fad057f745 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -61,7 +61,7 @@ type Client interface { RecordError(error) // RecordMetrics records the metrics from the provided HTTP request. - RecordMetrics(ctx context.Context, w *ResponseWrapper, ...ClientRecordMetricsOption) + RecordMetrics(ctx context.Context, w *ResponseWrapper) // RecordSpan sets the span attributes and status code based on the HTTP // request and response. @@ -70,16 +70,8 @@ type Client interface { // It remains the instrumentation's responsibility to start and end spans. RecordSpan(ctx context.Context, req *http.Request, resp *http.Response) } - -// ClientRecordMetricsOption applies options to the RecordMetrics method -type ClientRecordMetricsOption interface{} ``` -The `ClientRecordMetricsOption` allows passing optional parameters such as -status code, additional attributes, duration of the request, ... -Those options will be used to generate the appropriate metrics. If they are not -provided, no metric will be generated with the empty data. - #### The Server ```golang @@ -98,24 +90,14 @@ type Server interface { RecordSpan(ctx context.Context, req *http.Request, resp *http.Response, ...ServerRecordSpanOptions) } -// ClientRecordMetricsOption applies options to the RecordMetrics method -type ClientRecordMetricsOption interface{} - // ServerRecordSpanOption applies options to the RecordSpan method type ServerRecordSpanOption interface{} ``` -The `ServerRecordMetricsOption` and `ServerRecordSpanOption` functional options -allows passing optional parameters. +The `ServerRecordMetricsOption` functional option allows passing optional +parameters to be used within the `RecordSpan` method, such as the HTTP route. -Those options can be: - -* Status Code, additional attributes, duration of the request for metrics -* HTTP Route for the span - -When the data those options provide is not specified, no telemetry will be -emitted (the metrics will not be emitted, and the span attributes will not be -set). +When the data those options provide is not specified, the related span attributes will not be set. #### Request and Response wrappers @@ -127,24 +109,26 @@ We may provide a new package, or actual implementations within the `otelhttpconv` package later on. But doing so is not covered by this design document at the moment. +Therefore external implementations will currently have to implement this logic +themselves. + ### Implementations We will provide one official implementation of the described interfaces. This implementation will have the following requirements: * Support for the latest semantic conventions only. -* Support for stable metrics and attributes only. - +* Support for stable semantic conventions only. We may provide additional implementations later on such as: * An implementation that serves as a proxy to allow combining multiple implementations together. -* An implementation that covers unstable metrics and attributes only. +* An implementation that covers unstable semantic conventions. ### Usage -Instrumentations should use the official implementation mentioned above by -default. +By default, instrumentations should use the official implementation mentioned +above. They may provide an option for their users to override the used implementation with a custom one. From fbc47f12163c283b23b3aa460fa0c26c325e95dd Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Wed, 5 Mar 2025 13:51:03 +0100 Subject: [PATCH 06/17] Update instrumentation/net/http/otelhttpconv/DESIGN.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- instrumentation/net/http/otelhttpconv/DESIGN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index 2fad057f745..ea6ca8ba164 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -13,7 +13,7 @@ provided by the internal `semconvutil` package within their own instrumentations ([#4580](https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4580)). This package aims to solve that by being publicly usable. -This document outlines a proposal for a new instrumentation package called +This document outlines a proposal for a new instrumentation module called `otelhttpconv` that will provide a way to reduce some of this code repetition, especially with the setup of spans and metrics and their attributes. From 63f9061deb72a068f57d3424f9672875e9c2c84f Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Wed, 5 Mar 2025 14:04:32 +0100 Subject: [PATCH 07/17] specify the package will be used by other packages --- .../net/http/otelhttpconv/DESIGN.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index ea6ca8ba164..c2508a46a9f 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -17,6 +17,24 @@ This document outlines a proposal for a new instrumentation module called `otelhttpconv` that will provide a way to reduce some of this code repetition, especially with the setup of spans and metrics and their attributes. +This package will be used by every official instrumentation that handles HTTP +requests. It may be used by external implementers as well. +This package therefore needs to provide a clear, consistent and stable API that +instrumentations can use, hence this document. + +That API will have the following requirements: + +* Everything it provides must be done without the external use of internal packages. + Even though official instrumentations can technically import internal + packages from different modules, external ones cannot. And doing so leads to + unexpected breaking changes. +* Minimal number of breaking changes once the module is shipped. + While we can't publish a new module as stable directly (we may have missed + things), this module will need to become stable as soon as any of the HTTP + instrumentations become stable. + As our goal is to make `otelhttp` stable in 2025, stabilization should happen + within the same timeframe. + The goal of this document is also to make future semantic convention migrations easier, and to allow flexibility with the use or not of unstable attributes/metrics. From 4084bc380f250a1830088437770f27a33e07c81d Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 11 Mar 2025 10:31:13 +0100 Subject: [PATCH 08/17] add a sample use in an http.Handler --- .../net/http/otelhttpconv/DESIGN.md | 82 +++++++++++++++++-- 1 file changed, 77 insertions(+), 5 deletions(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index c2508a46a9f..4aba4863d45 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -58,6 +58,8 @@ One for client. and one for servers. Implementations can use one, the other or b // RequestWrapper provides a layer on top of `*http.Response` that tracks // additional information such as bytes read etc. type ResponseWrapper interface { + http.ResponseWriter + // Duration contains the duration of the HTTP request Duration() time.Duration @@ -86,8 +88,11 @@ type Client interface { // This method does not create a new span. It retrieves the current one from // the context. // It remains the instrumentation's responsibility to start and end spans. - RecordSpan(ctx context.Context, req *http.Request, resp *http.Response) + RecordSpan(ctx context.Context, req *http.Request, w *ResponseWrapper, cfg ...ClientRecordSpanOption) } + +// ClientRecordSpanOption applies options to the RecordSpan method +type ClientRecordSpanOption interface{} ``` #### The Server @@ -105,17 +110,19 @@ type Server interface { // This method does not create a new span. It retrieves the current one from // the context. // It remains the instrumentation's responsibility to start and end spans. - RecordSpan(ctx context.Context, req *http.Request, resp *http.Response, ...ServerRecordSpanOptions) + RecordSpan(ctx context.Context, req *http.Request, w *ResponseWrapper, cfg ...ServerRecordSpanOption) } // ServerRecordSpanOption applies options to the RecordSpan method type ServerRecordSpanOption interface{} ``` -The `ServerRecordMetricsOption` functional option allows passing optional -parameters to be used within the `RecordSpan` method, such as the HTTP route. +The `ClientRecordSpanOption` and `ServerRecordSpanOption` functional options +allows passing optional parameters to be used within the `RecordSpan` method, +such as the HTTP route. -When the data those options provide is not specified, the related span attributes will not be set. +When the data those options provide is not specified, the related span +attributes will not be set. #### Request and Response wrappers @@ -143,6 +150,71 @@ We may provide additional implementations later on such as: * An implementation that serves as a proxy to allow combining multiple implementations together. * An implementation that covers unstable semantic conventions. +#### Example implementation + +The implementation example here is kept simple for the purpose of +understandability. + +The following code sample provides a simple `http.Handler` that implements the +provided interface to instrument HTTP applications. + +Because both the client and server interface are very similar, a client +implementation would be similar too. + +```golang +type middleware struct { + operation string + + tracer trace.Tracer + propagators propagation.TextMapPropagator + meter metric.Meter + httpconv otelhttpconv.Server +} + +// NewMiddleware returns a tracing and metrics instrumentation middleware. +// The handler returned by the middleware wraps a handler +// in a span named after the operation and enriches it with metrics. +func NewMiddleware(operation string) func(http.Handler) http.Handler { + m := middleware{ + operation: operation, + tracer: otel.Tracer("http"), + propagators: otel.GetTextMapPropagator(), + meter: otel.Meter("http"), + httpconv: otelhttpconv.NewHTTPConv(otel.Tracer("httpconv"), otel.Meter("httpconv")), + } + + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + m.serveHTTP(w, r, next) + }) + } +} + +func (m *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http.Handler) { + ctx := m.propagators.Extract(r.Context(), propagation.HeaderCarrier(r.Header)) + + // We keep creating the span here, as that is something we want to do before + // the middleware stack is run + ctx, span := m.tracer.Start(ctx, fmt.Sprintf("%s %s", r.Method, r.Pattern)) + defer span.End() + + // NewResponseWrapper wraps additional data into the http.ResponseWriter, + // such as duration, status code and quantity of bytes read. + rww := otelhttpconv.NewResponseWrapper(w) + next.ServeHTTP(rww, r.WithContext(ctx)) + + // RecordMetrics emits the proper semantic convention metrics + // With data based on the provided response wrapper + m.httpconv.RecordMetrics(ctx, rww) + + // RecordSpan emits the proper semantic convention span attributes and events + // With data based on the provided response wrapper + // It must not create a new span. It retrieves the current one from the + // context. + m.httpconv.RecordSpan(ctx, r, rww) +} +``` + ### Usage By default, instrumentations should use the official implementation mentioned From 5efaca9784105431e5132e6a794ce1c1d4b2c88e Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 11 Mar 2025 15:07:42 +0100 Subject: [PATCH 09/17] reference responsewriter directly, not through pointer --- instrumentation/net/http/otelhttpconv/DESIGN.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index 4aba4863d45..cf3611c52ab 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -81,14 +81,14 @@ type Client interface { RecordError(error) // RecordMetrics records the metrics from the provided HTTP request. - RecordMetrics(ctx context.Context, w *ResponseWrapper) + RecordMetrics(ctx context.Context, w ResponseWrapper) // RecordSpan sets the span attributes and status code based on the HTTP // request and response. // This method does not create a new span. It retrieves the current one from // the context. // It remains the instrumentation's responsibility to start and end spans. - RecordSpan(ctx context.Context, req *http.Request, w *ResponseWrapper, cfg ...ClientRecordSpanOption) + RecordSpan(ctx context.Context, req *http.Request, w ResponseWrapper, cfg ...ClientRecordSpanOption) } // ClientRecordSpanOption applies options to the RecordSpan method @@ -103,14 +103,14 @@ type ClientRecordSpanOption interface{} type Server interface { // RecordMetrics records the metrics from the provided HTTP request. - RecordMetrics(ctx context.Context, w *ResponseWrapper) + RecordMetrics(ctx context.Context, w ResponseWrapper) // RecordSpan sets the span attributes and status code based on the HTTP // request and response. // This method does not create a new span. It retrieves the current one from // the context. // It remains the instrumentation's responsibility to start and end spans. - RecordSpan(ctx context.Context, req *http.Request, w *ResponseWrapper, cfg ...ServerRecordSpanOption) + RecordSpan(ctx context.Context, req *http.Request, w ResponseWrapper, cfg ...ServerRecordSpanOption) } // ServerRecordSpanOption applies options to the RecordSpan method From 2e58049e186cc5f9c8775609ea8ce8a6f7dfa20d Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 11 Mar 2025 15:12:17 +0100 Subject: [PATCH 10/17] add BytesWritten and Error --- instrumentation/net/http/otelhttpconv/DESIGN.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index cf3611c52ab..9b7348e0382 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -68,6 +68,12 @@ type ResponseWrapper interface { // BytesRead contains the amount of bytes read from the response's body BytesRead() int64 + + // BytesWritten contains the amount of bytes written to the response's body + BytesWritten() int64 + + // Error contains the last error that occured while writing + Error() error } ``` From f51a9fac38c53c28bc3a2bdb08b1766f6e377b82 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 11 Mar 2025 15:18:56 +0100 Subject: [PATCH 11/17] split ResponseWrapper and BodyWrapper into two interfaces --- .../net/http/otelhttpconv/DESIGN.md | 45 ++++++++++--------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index 9b7348e0382..dd83032881c 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -52,34 +52,21 @@ We will provide two public interface that allow interacting with the instrumentation package. One for client. and one for servers. Implementations can use one, the other or both. -#### Shared between client and server +#### The client ```golang -// RequestWrapper provides a layer on top of `*http.Response` that tracks +// BodyWrapper provides a layer on top of the request's body that tracks // additional information such as bytes read etc. -type ResponseWrapper interface { - http.ResponseWriter - - // Duration contains the duration of the HTTP request - Duration() time.Duration - - // Duration contains the status code returned by the HTTP request - StatusCode() int +type BodyWrapper interface { + io.ReadCloser // BytesRead contains the amount of bytes read from the response's body BytesRead() int64 - // BytesWritten contains the amount of bytes written to the response's body - BytesWritten() int64 - - // Error contains the last error that occured while writing + // Error contains the last error that occured while reading Error() error } -``` - -#### The client -```golang // Client provides an interface for HTTP client instrumentations to set the // proper semantic convention attributes and metrics into their data. type Client interface { @@ -87,14 +74,14 @@ type Client interface { RecordError(error) // RecordMetrics records the metrics from the provided HTTP request. - RecordMetrics(ctx context.Context, w ResponseWrapper) + RecordMetrics(ctx context.Context, w BodyWrapper) // RecordSpan sets the span attributes and status code based on the HTTP // request and response. // This method does not create a new span. It retrieves the current one from // the context. // It remains the instrumentation's responsibility to start and end spans. - RecordSpan(ctx context.Context, req *http.Request, w ResponseWrapper, cfg ...ClientRecordSpanOption) + RecordSpan(ctx context.Context, req *http.Request, w BodyWrapper, cfg ...ClientRecordSpanOption) } // ClientRecordSpanOption applies options to the RecordSpan method @@ -104,6 +91,24 @@ type ClientRecordSpanOption interface{} #### The Server ```golang +// ResponseWrapper provides a layer on top of `*http.Response` that tracks +// additional information such as bytes written etc. +type ResponseWrapper interface { + http.ResponseWriter + + // Duration contains the duration of the HTTP request + Duration() time.Duration + + // Duration contains the status code returned by the HTTP request + StatusCode() int + + // BytesWritten contains the amount of bytes written to the response's body + BytesWritten() int64 + + // Error contains the last error that occured while writing + Error() error +} + // Server provides an interface for HTTP server instrumentations to set the // proper semantic convention attributes and metrics into their data. type Server interface { From d96c7ec0f24f454172ee6a864638822f61c12702 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 11 Mar 2025 15:40:10 +0100 Subject: [PATCH 12/17] fix spelling --- instrumentation/net/http/otelhttpconv/DESIGN.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index dd83032881c..6f129f081a7 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -63,7 +63,7 @@ type BodyWrapper interface { // BytesRead contains the amount of bytes read from the response's body BytesRead() int64 - // Error contains the last error that occured while reading + // Error contains the last error that occurred while reading Error() error } @@ -105,7 +105,7 @@ type ResponseWrapper interface { // BytesWritten contains the amount of bytes written to the response's body BytesWritten() int64 - // Error contains the last error that occured while writing + // Error contains the last error that occurred while writing Error() error } From 1468bfd709364bd318a9d74c33f11d468311f2cc Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Fri, 14 Mar 2025 09:22:28 +0100 Subject: [PATCH 13/17] Update instrumentation/net/http/otelhttpconv/DESIGN.md Co-authored-by: Sam Xie --- instrumentation/net/http/otelhttpconv/DESIGN.md | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index 6f129f081a7..12bd56a6da4 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -112,7 +112,6 @@ type ResponseWrapper interface { // Server provides an interface for HTTP server instrumentations to set the // proper semantic convention attributes and metrics into their data. type Server interface { - // RecordMetrics records the metrics from the provided HTTP request. RecordMetrics(ctx context.Context, w ResponseWrapper) From fc9dd8c3e54a4344e0a351e9b26c485a2dda48d2 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 18 Mar 2025 11:47:16 +0100 Subject: [PATCH 14/17] internalize the request and response wrappers --- .../net/http/otelhttpconv/DESIGN.md | 82 ++++++++++--------- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index 12bd56a6da4..3ba6d671a4c 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -46,20 +46,23 @@ The proposed design aims to: * Work with every official instrumentation, and be available to external implementers. * Provide flexibility in its use, to allow folks the use of unstable semantic conventions if they wish. -### Interfaces - -We will provide two public interface that allow interacting with the -instrumentation package. -One for client. and one for servers. Implementations can use one, the other or both. +#### Request and Response wrappers -#### The client +We will provide one struct implementation which is not meant to be extensible, +for the response and body wrappers. +That implementation is the equivalent of the current +[internal/shared/request](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/internal/shared/request) +package. ```golang // BodyWrapper provides a layer on top of the request's body that tracks // additional information such as bytes read etc. -type BodyWrapper interface { +type BodyWrapper struct { io.ReadCloser + // OnRead is a callback that tracks the amount of data read from the body + OnRead func(n int64) + // BytesRead contains the amount of bytes read from the response's body BytesRead() int64 @@ -67,6 +70,40 @@ type BodyWrapper interface { Error() error } +// ResponseWrapper provides a layer on top of `*http.Response` that tracks +// additional information such as bytes written etc. +type ResponseWrapper struct { + http.ResponseWriter + + // OnWrite is a callback that tracks the amount of data written to the response + OnWrite func(n int64) + + // Duration contains the duration of the HTTP request + Duration() time.Duration + + // Duration contains the status code returned by the HTTP request + StatusCode() int + + // BytesWritten contains the amount of bytes written to the response's body + BytesWritten() int64 + + // Error contains the last error that occurred while writing + Error() error +} +``` + +Note: to keep this document concise, the full implementation is not provided +here. + +### Interfaces + +We will provide two public interface that allow interacting with the +instrumentation package. +One for client. and one for servers. Implementations can use one, the other or both. + +#### The client + +```golang // Client provides an interface for HTTP client instrumentations to set the // proper semantic convention attributes and metrics into their data. type Client interface { @@ -91,24 +128,6 @@ type ClientRecordSpanOption interface{} #### The Server ```golang -// ResponseWrapper provides a layer on top of `*http.Response` that tracks -// additional information such as bytes written etc. -type ResponseWrapper interface { - http.ResponseWriter - - // Duration contains the duration of the HTTP request - Duration() time.Duration - - // Duration contains the status code returned by the HTTP request - StatusCode() int - - // BytesWritten contains the amount of bytes written to the response's body - BytesWritten() int64 - - // Error contains the last error that occurred while writing - Error() error -} - // Server provides an interface for HTTP server instrumentations to set the // proper semantic convention attributes and metrics into their data. type Server interface { @@ -134,19 +153,6 @@ such as the HTTP route. When the data those options provide is not specified, the related span attributes will not be set. -#### Request and Response wrappers - -The request and response wrappers implementations will be provided by the -[internal/request](https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/request) -templates package. - -We may provide a new package, or actual implementations within the -`otelhttpconv` package later on. But doing so is not covered by this design -document at the moment. - -Therefore external implementations will currently have to implement this logic -themselves. - ### Implementations We will provide one official implementation of the described interfaces. From c7c2fe627e584e7d39e8c68a9d2f0aeb1e4ea2a5 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 18 Mar 2025 11:52:11 +0100 Subject: [PATCH 15/17] add compatibility section --- instrumentation/net/http/otelhttpconv/DESIGN.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index 3ba6d671a4c..e460843d61f 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -153,6 +153,23 @@ such as the HTTP route. When the data those options provide is not specified, the related span attributes will not be set. +### Compatibility + +These interfaces are kept relatively simple on purpose, so they can be used as +foundations if folks wish to extend them with their own actual implementations. + +Once stable, we will not be able to change those interfaces anymore (adding, +renaming or deleting methods), as that would break backwards compatibility with +implementations. + +However, because they remain lightweight, they allow folks to do custom logic, +such as: + +* A proxy that would allow combining multiple implementations together. +* An implementation that covers unstable semantic conventions. +* Implementations for older semantic conventions. +* Implementations that cover custom implementations if a customer doesn't wish to use semconvs. + ### Implementations We will provide one official implementation of the described interfaces. From 2788ed30b5fa9b91c45555671ac2723fe475fea4 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 18 Mar 2025 11:56:05 +0100 Subject: [PATCH 16/17] merge RecordMetrics and RecordSpan --- .../net/http/otelhttpconv/DESIGN.md | 41 ++++++++----------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index e460843d61f..332c92b45a9 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -110,19 +110,16 @@ type Client interface { // RecordError records an error returned by the HTTP request. RecordError(error) - // RecordMetrics records the metrics from the provided HTTP request. - RecordMetrics(ctx context.Context, w BodyWrapper) - - // RecordSpan sets the span attributes and status code based on the HTTP - // request and response. + // Record records the telemetry (spans and metrics) from the provided HTTP request. + // // This method does not create a new span. It retrieves the current one from // the context. // It remains the instrumentation's responsibility to start and end spans. - RecordSpan(ctx context.Context, req *http.Request, w BodyWrapper, cfg ...ClientRecordSpanOption) + Record(ctx context.Context, req *http.Request, w BodyWrapper, cfg ...ClientRecordOption) } -// ClientRecordSpanOption applies options to the RecordSpan method -type ClientRecordSpanOption interface{} +// RecordOption applies options to the Record method +type ClientRecordOption interface{} ``` #### The Server @@ -131,24 +128,22 @@ type ClientRecordSpanOption interface{} // Server provides an interface for HTTP server instrumentations to set the // proper semantic convention attributes and metrics into their data. type Server interface { - // RecordMetrics records the metrics from the provided HTTP request. - RecordMetrics(ctx context.Context, w ResponseWrapper) - - // RecordSpan sets the span attributes and status code based on the HTTP + // Record records the telemetry (spans and metrics) from the provided HTTP // request and response. + // // This method does not create a new span. It retrieves the current one from // the context. // It remains the instrumentation's responsibility to start and end spans. - RecordSpan(ctx context.Context, req *http.Request, w ResponseWrapper, cfg ...ServerRecordSpanOption) + Record(ctx context.Context, req *http.Request, w ResponseWrapper, cfg ...ServerRecordOption) } // ServerRecordSpanOption applies options to the RecordSpan method -type ServerRecordSpanOption interface{} +type ServerRecordOption interface{} ``` -The `ClientRecordSpanOption` and `ServerRecordSpanOption` functional options -allows passing optional parameters to be used within the `RecordSpan` method, -such as the HTTP route. +The `ClientRecordOption` and `ServerRecordOption` functional options allows +passing optional parameters to be used within the `Record` method, such as the +HTTP route. When the data those options provide is not specified, the related span attributes will not be set. @@ -236,15 +231,13 @@ func (m *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http rww := otelhttpconv.NewResponseWrapper(w) next.ServeHTTP(rww, r.WithContext(ctx)) - // RecordMetrics emits the proper semantic convention metrics - // With data based on the provided response wrapper - m.httpconv.RecordMetrics(ctx, rww) - - // RecordSpan emits the proper semantic convention span attributes and events - // With data based on the provided response wrapper + // RecordMetrics emits the proper semantic convention metrics, and embeds the + // span with the proper attributes and events from the provided request and + // response wrapper. + // // It must not create a new span. It retrieves the current one from the // context. - m.httpconv.RecordSpan(ctx, r, rww) + m.httpconv.Record(ctx, r, rww) } ``` From cfd8b48622c637497d69af406fe7505c25db6302 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 18 Mar 2025 12:02:40 +0100 Subject: [PATCH 17/17] add alternatives section --- .../net/http/otelhttpconv/DESIGN.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/instrumentation/net/http/otelhttpconv/DESIGN.md b/instrumentation/net/http/otelhttpconv/DESIGN.md index 332c92b45a9..38737d8dd10 100644 --- a/instrumentation/net/http/otelhttpconv/DESIGN.md +++ b/instrumentation/net/http/otelhttpconv/DESIGN.md @@ -253,3 +253,31 @@ For example, with the `otelhttp` instrumentation for clients: ```golang otelhttp.NewTransport(http.DefaultTransport, otelhttp.WithHTTPConv(myCustomImplementation{})) ``` + +## Alternatives + +### Do nothing + +We could do nothing and keep the current templatized implementation. + +That current implementation has a few issues though: + +* Each change in the templates requires a changelog entry that mentions **every** HTTP package, making them hard to grasp. +* Folks have been asking for a way to use the semconv package in their own instrumentation packages. +* The current implementation is a bit of a stability hazard that makes it hard to stabilize the otelhttp instrumentation. + +### Function instead of struct + +We could simplify things even more with a function instead of interfaces. + +```golang +func RecordClient(r *http.Request, params RecordClientParameters) + +type RecordClientParameters struct { + Tracer trace.Tracer + Meter metric.Meter + Error error + Response *http.Response + Duration time.Time +} +```