Set http.status_code span attribute for handlers that do not write explicit response

[Kwintenvdb]

Background

The otelhttp.NewHandler wraps a handler with a span. After the wrapped handler's ServeHTTP method is called, the setAfterServeAttributes function is called, setting several span attributes are set.

One such attribute is the http.status_code attribute, which is set if and only if a status code was explicitly set via the ResponseWriter (see documentation). This means that the http.status_code attribute will only be set if the handler function explicitly makes a call to ResponseWriter.WriteHeader or ResponseWriter.Write. Allow me to illustrate with the following very minimal code sample:

func main() {
	tp := sdktrace.NewTracerProvider()
	otel.SetTracerProvider(tp)

	handler := otelhttp.NewHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Println("handler invoked")
		// Uncommenting either of the two lines below will produce a span that contains
		// the http.status_code attribute. When commented, the span does not have this attribute.
		// w.WriteHeader(200)
		// fmt.Fprintf(w, "OK")
	}), "myHandler")
	http.Handle("/", handler)
	http.ListenAndServe(":8080", nil)
}

The issue and proposed solution

In the code snippet above, a minimal HTTP server is created using Go's standard library. Even if the handler function does not explicitly call w.WriteHeader(200) or fmt.Fprintf(w, "OK"), the server will still respond with a 200 status code. In other words, it appears the built-in HTTP server will always set the 200 status code on the response if the handler function returns without panicking or setting a different status code.

It therefore appears that the OpenTelemetry instrumentation is slightly too restrictive when it comes to setting the respective http.status_code span attribute. Would it not be safe to, under the same assumptions as the HTTP server itself, set this attribute to 200 if the handler function did not explicitly set a different status code? I can easily imagine a scenario in which a user might be under the presumption that this attribute is included in the span, since the request itself did successfully return with a 200 status code. This seems especially significant for HTTP handlers which do not return content, and only a status code.

[dmathieu]

I think we're missing implementation of Flusher in respWriterWrapper.

In the net/http implementation of ResponseWriter, we can see Flush does write the header if that hasn't happened yet: https://cs.opensource.google/go/go/+/refs/tags/go1.19.1:src/net/http/server.go;l=1702

[Kwintenvdb]

I think we're missing implementation of Flusher in respWriterWrapper.

In the net/http implementation of ResponseWriter, we can see Flush does write the header if that hasn't happened yet: https://cs.opensource.google/go/go/+/refs/tags/go1.19.1:src/net/http/server.go;l=1702

Good catch. That does indeed appear to be the issue. The Flusher doc comment even makes mention of this:

// The default HTTP/1.x and HTTP/2 ResponseWriter implementations
// support Flusher, but ResponseWriter wrappers may not. Handlers
// should always test for this ability at runtime.

Would it be reasonable to expect this to be added to the respWriterWrapper in a future version? If desired, I can open a PR with a contribution as well of course.

[dmathieu]

I think this would definitely be reasonable. We would definitely be happy to see a PR doing that.

[Kwintenvdb]

@dmathieu I have started digging into this a bit, and it seems simply implementing the Flusher interface respWriterWrapper has not much use.

In essence, the respWriterWrapper is passed only to the handler function itself, and has no effect on the surrounding context. In other words, the surrounding HTTP server implementation makes no use of the wrapper, and it continues the use the wrapped response object, for example.

handler := otelhttp.NewHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	// The wrapped ResponseWriter will ONLY have bearing on whatever happens inside this function.
	fmt.Println("handler invoked")
	// Uncommenting either of the two lines below will produce a span that contains
	// the http.status_code attribute. When commented, the span does not have this attribute.
	// w.WriteHeader(200)
	// fmt.Fprintf(w, "OK")
}), "myHandler")

Therefore, implementing the Flush() function would only have any effect if I would manually call w.(http.Flusher).Flush() inside this handler function. It does not matter if Flush() or WriteHeader() is invoked at any point after that by the HTTP server implementation afterwards, as these calls are not delegated to this wrapper and therefore escapes the instrumentation.

It looks like the httpsnoop library that is used for wrapping the ResponseWriter acknowledges this limitation and also sets a default status code of 200 for one of the methods it exposes. See here and here.

In conclusion, I would say that implementing the Flusher interface on the respWriterWrapper has no further benefits here, and that the OpenTelemetry instrumentation should also assume a default 200 status code if no other status code was manually set within the handler, as per my original suggestion. What do you think?

[dmathieu]

You are absolutely right. I've given this a go in #2822.