Audit otelhttp API

[dmathieu]

We're starting to look into stabilizing the go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp package.

For that, we need to audit the public API of the package, not for missing things (as those could be added later) but for things that should be changed before we can't do breaking changes anymore.

For reference, this is the API as it is with the latest release:

func ContextWithLabeler(parent context.Context, l *Labeler) context.Context
func ContextWithStartTime(parent context.Context, start time.Time) context.Context
func Get(ctx context.Context, targetURL string) (resp *http.Response, err error)
func Head(ctx context.Context, targetURL string) (resp *http.Response, err error)
func NewHandler(handler http.Handler, operation string, opts ...Option) http.Handler
func NewMiddleware(operation string, opts ...Option) func(http.Handler) http.Handler
func Post(ctx context.Context, targetURL, contentType string, body io.Reader) (resp *http.Response, err error)
func PostForm(ctx context.Context, targetURL string, data url.Values) (resp *http.Response, err error)
func StartTimeFromContext(ctx context.Context) time.Time
func Version() string
func WithRouteTag(route string, h http.Handler) http.Handler
type Filter
type Labeler
    func LabelerFromContext(ctx context.Context) (*Labeler, bool)
    func (l *Labeler) Add(ls ...attribute.KeyValue)
    func (l *Labeler) Get() []attribute.KeyValue

type Option
    func WithClientTrace(f func(context.Context) *httptrace.ClientTrace) Option
    func WithFilter(f Filter) Option
    func WithMessageEvents(events ...event) Option
    func WithMeterProvider(provider metric.MeterProvider) Option
    func WithMetricAttributesFn(metricAttributesFn func(r *http.Request) []attribute.KeyValue) Option
    func WithPropagators(ps propagation.TextMapPropagator) Option
    func WithPublicEndpoint() Option
    func WithPublicEndpointFn(fn func(*http.Request) bool) Option
    func WithServerName(server string) Option
    func WithSpanNameFormatter(f func(operation string, r *http.Request) string) Option
    func WithSpanOptions(opts ...trace.SpanStartOption) Option
    func WithTracerProvider(provider trace.TracerProvider) Option

type Transport
    func NewTransport(base http.RoundTripper, opts ...Option) *Transport
    func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error)

This issue is open for any comment opinions before that stabilization.

Critical Issues (Must Fix Before Stabilization)

• Remove ReadBytesKey, ReadErrorKey, WroteBytesKey, WriteErrorKey from public API #8130
• Stabilization of error recording semantic conventions is a blocker - Need to wait for stabilization of error recording spec before proceeding.
• Decide on span events vs log events and WithMessageEvents() implications - Need to determine if span events should migrate to log events prior to stabilization, possibly behind a feature flag. This impacts the WithMessageEvents() option.
• Consider renaming NewMiddleware
• Consider renaming Transport
• otelhttp: make DefaultClient a non-mutable variable #8131
• otelhttp: WithMessageEvents` accepts an unexported type as a parameter. - Fix type declaration to not contain unexported types #8132

Important Issues (Should Fix)

• Deprecate and remove WithRouteTag option - This option can be replaced with WithMetricAttributesFn for controlling http.route in metrics.
  • otelhttp: deprecate the WithRouteTag option #8117
• Rethink Labeler type design and API integration - The Labeler type is for metrics but isn't clearly connected to the rest of the API. Its purpose and representation need to be reconsidered.
• Consider splitting Option types for Handler and Transport - Handler and Transport currently share the same option type, but options like WithClientTrace and WithSpanNameFormatter only apply to one. Should split if needed.
• Reconsider ContextWithStartTime / StartTimeFromContext as public API - These functions are strange to have as part of (just) HTTP instrumentation and may not belong in the public API.
• Address issues #726 and #4501 before stabilization - These issues significantly alter output behavior and should be resolved before declaring otelhttp stable.
• Consider renaming Labeler to MetricLabeler - Since the godoc states it only applies to metrics, the name could be more specific. Alternatively, consider adding attributes to spans as well.
• Consider renaming WithFilter to WithSpanFilter - Since the filter only applies to spans, a more specific name would be clearer.
• Consider refactoring the filters package - Move filters to options in otelhttp or as other declarations.
• Decide if Filter needs to accept other parameters - Address extensibility
• Ensure future extensibility - HTTP/2, HTTP/3, WebSockets
• Rethink WithMetricAttributesFunc - cache or redesign

Nice to Have

• Refactor or rename common.go file - The file is overly generic and not representative of what it contains.
• Comprehensive package documentation
• Public API benchmarks

Context Notes

• Issue Should we have expert, or generalist handlers? #2782 about generalist vs specific instrumentation is not resolved by this stabilization effort - any changes would introduce breaking changes and the issue hasn't generated momentum. Can be considered for a future v2.

[mx-psi]

From the core Collector side here is the relevant Github search: https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-collector+%2Fotelhttp%5C.%2F+language%3AGo&type=code

We use NewHandler and otelhttp.Option, as well as the otelhttp.ScopeName constant.

[dashpole]

My rough notes:

• otelhttp adds span events today. Can/should we migrate these to (log) events prior to stabilization? I don't see anything in the http semconv spec about span events, but we also don't have a stable event API to use. Maybe we put these behind a feature flag to give us flexibility? This also has implications for WithMessageEvents().
• Godoc for Labeler says it only applies to metrics. Should we consider renaming it to MetricLabeler, or adding the attributes to spans as well (I think this was rejected in the past)?
• Consider renaming WithFilter to WithSpanFilter, as it only applies to spans.
• I see we have some semantic conventions-esque things defined in the package: ReadBytesKey, ReadErrorKey, WroteBytesKey, WriteErrorKey. Should these be upstreamed, and removed from the package?

[dmathieu]

WithRouteTag can probably be deprecated and removed.
See https://cloud-native.slack.com/archives/C01NPAXACKT/p1761999249564689

[gbarr]

WithRouteTag can probably be deprecated and removed.

If removing WithRouteTag then an option should be added to control if http.route is added to metrics so that it does not create an undesired increase in metric cardinality

EDIT: As mentioned in the slack thread WithMetricAttributesFn can be used to add http.route to metrics instead of WithRouteTag

[seankhliao]

If this is locking in the api, is that meant as a resolution for #2782 ?

#726 and #4501 aren't code api changes, but do significantly alter the output that I think any change should be made before otelhttp is declared stable

[dmathieu]

If this is locking in the api, is that meant as a resolution for #2782 ?

This definitely does not resolve that issue. However, making a non-generalist decision at this point would introduce really non-trivial breaking changes. Since this issue hasn't really generated momentum, I think we can keep the idea around if we ever want to think about a v2.

[dmathieu]

See #7254 (comment) which recommends stabilization of https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/recording-errors.md#recording-errors-on-spans as a blocker.

[MrAlias]

• I see we have some semantic conventions-esque things defined in the package: ReadBytesKey, ReadErrorKey, WroteBytesKey, WriteErrorKey. Should these be upstreamed, and removed from the package?

👍

These need to be removed from the API before it is stabilized.

[MrAlias]

The common.go file needs to be renamed/refactored it is overly generic and not representative of what it contains.

[MrAlias]

The Labeler type is for metrics but isn't clearly connected to the rest of the API. It purpose and how it is represented in the API needs to be re-thought.

[MrAlias]

The options for Handler and Transport are the same type. Are we ever going to need to add an option that applies to one but not the other? If so, they should be split.

[MrAlias]

The options for Handler and Transport are the same type. Are we ever going to need to add an option that applies to one but not the other? If so, they should be split.

Hmm, this may already exist (i.e. WithClientTrace and WithSpanNameFormatter)

[seankhliao]

I still think ContextWithStartTime / StartTimeFromContext is a very strange thing to have as part of (just) http instrumentation or at all.