opentelemetry-go-contrib icon indicating copy to clipboard operation
opentelemetry-go-contrib copied to clipboard

Request to include request headers into span attributes for net/http

Open danielmmetz opened this issue 1 year ago • 6 comments

Background

Package Link: go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp

Many Go http services use the stdlib provided net/http for which go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp provides an easy to use middleware.

Looking into the Python and Java equivalents, each supports an environment variable OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST which allows for specified HTTP Headers to be automatically included in spans as attributes. However the Go implementation does not yet support this.

Why can this instrumentation not be included in the package itself?

Instrumentation is presently outside of the scope of the Go standard library. net/http/otelhttp already exists; it's reasonable to simply extend it further.

Why can this instrumentation not be hosted in a dedicated repository?

Same answer as above.

Proposed Solution

I think there are at least a few options:

  1. Provide a new otelhttp.Option that configures a (*otelhttp.Config).requestAttributes field (with a type like []func(*http.Request) attribute.KeyValue, which gets incorporated where the other span attributes are set. Users would be able to provide this as an explicit option when invoking otelhttp.NewHandler. E.g.
        // new code
        for _, requestAttribute := range h.requestAttributes {
            opts = append(opts, trace.WithAttributes(requestAttribute(r))
        }
        // existing code
	opts = append([]trace.SpanStartOption{
		trace.WithAttributes(semconv.NetAttributesFromHTTPRequest("tcp", r)...),
		trace.WithAttributes(semconv.EndUserAttributesFromHTTPRequest(r)...),
		trace.WithAttributes(semconv.HTTPServerAttributesFromHTTPRequest(h.operation, "", r)...),
	}, opts...) // start with the configured options
  1. Similarly, implement support for an environment variable OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST, which could be read and processed as part of otelhttp.NewHandler. An upside is parity between languages, but at the cost of introducing a side-effect in code that's otherwise pretty explicit. This could likely be balanced by hiding this behind an Option which could potentially be explicitly provided by users.
  2. allow it to somehow provided as part of setting up the TracerProvider
    • in support: this is where many global settings are currently configured, and it would allow the setting to be explicit
    • however: this would mean somehow registering a function that accepts an *http.Request, and potentially needing to smuggle it through a context.Context, for which there may not be precedent and/or it might be a pain

Tracing

  • attributes:
    • the headers would be incorporated in span attributes using the semantic conventions of http.request.header.<key> with values of type []string
  • events:
    • n/a
  • links:
    • n/a

Metrics

n/a

Prior Art

For Python, see https://github.com/open-telemetry/opentelemetry-python-contrib/blob/cca90db86f177d3b9affa54a2089d3a28c58e888/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/init.py#L529-L533

For Java see https://opentelemetry.io/docs/instrumentation/java/automatic/agent-config/#capturing-http-request-and-response-headers

Tasks

  • Code complete:
    • [ ] Comprehensive unit tests.
    • [ ] End-to-end integration tests.
    • [ ] Tests all passing.
    • [ ] Instrumentation functionality verified.
  • Documented
  • Examples
    • [ ] Dockerfile file to build example application.
    • [ ] docker-compose.yml to run example in a docker environment to demonstrate instrumentation.

danielmmetz avatar Jan 23 '23 17:01 danielmmetz

Thank you for your interest and proposal. I think I could support the explicit configuration proposed in option 1, but I'd really prefer if this were worked through the semantic conventions working group to provide a consistent view on how to handle these attributes across all OTel instrumentations.

Aneurysm9 avatar Jan 23 '23 17:01 Aneurysm9

I'd really prefer if this were worked through the semantic conventions working group to provide a consistent view on how to handle these attributes across all OTel instrumentations.

Sounds great. Is that something I should plan to attend to raise the issue, or is it enough to drop a link to this issue into the agenda of the next meeting?

danielmmetz avatar Jan 30 '23 16:01 danielmmetz

FWIW the Node implementation has the same (called headersToSpanAttributes).

https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/opentelemetry-instrumentation-http#http-instrumentation-options

SimenB avatar Feb 16 '23 09:02 SimenB

At the risk of me-tooing, I just ran into this issue myself and was disappointed (and surprised!) to see that the Go library was the odd man out here. @danielmmetz was there any progress to report from bringing this to the WG?

n-oden avatar Oct 11 '23 14:10 n-oden

I ran into this issue just today. May I ask why does this require a WG decision when there is a clear trend what the users want? I really don't see the point of waiting for a WG stamp in an ivory tower, while this feature has been implemented across multiple platforms and languages. In my opinion what the WG group can do at this point is just bowing their head and admit that this feature is necessary.

zergeborg avatar May 29 '24 03:05 zergeborg

The Go SIG is not in an "ivory tower". We review every pull request, keep the project on track and up to date, and we do that in the open since weekly meetings can be attended by everyone and (per definition), the project is open. I am currently the sole code owner of the otelhttp instrumentation. You're more than welcome to start contributing, and give a hand.

Note that this is achievable within your code (and in a more powerful way, since you could do any computing you want from the request, and add this as attributes).

func extendHTTPSpan(next http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		next.ServeHTTP(w, r)

		span := trace.SpanFromContext(r.Context())
		span.SetAttributes(
			attribute.String("authorization", r.Header.Get("Authorization"),
		)
	})
}

Then, to use it (order of the handlers matter):

handler := [...] // whatever your application does when serving HTTP requests.
handler = otelhttp.NewHandler(extendHTTPSpan(http.HandlerFunc(handler), "HTTP"))

http.Handle("/", handler)
_ = http.ListenAndServe(":7777", nil)

This is not to say the option can't be added. You are welcome to open a PR adding.

dmathieu avatar May 29 '24 07:05 dmathieu