Otelmux: span status are marked as an error; `Invalid HTTP status code 0`

[floyola]

Hi 👋

I'm using the otelmux library to instrument my incoming HTTP requests but for some reason 70% of them are marked as an error since its HTTP status code is 0.

After some research I found that the library set the default code as 0, but I don't understand why that code doesn't change.

Span #1
--
Trace ID       : #####
Parent ID      :
ID                 : #####
Name           :  my/route/
Kind             : SPAN_KIND_SERVER
Start time    : 2021-11-03 18:28:27.074053815 +0000 UTC
End time      : 2021-11-03 18:28:27.075496864 +0000 UTC
Status code : STATUS_CODE_ERROR
Status message : Invalid HTTP status code 0

Otelmux version: v0.25.0
Otel version: v1.1.0

[pellared]

At first glance I think that this how the code should look like:

func getRRW(writer http.ResponseWriter) *recordingResponseWriter {
	rrw := rrwPool.Get().(*recordingResponseWriter)
	rrw.written = false
	rrw.status = http.StatusOK
	rrw.writer = httpsnoop.Wrap(writer, httpsnoop.Hooks{
		Write: func(next httpsnoop.WriteFunc) httpsnoop.WriteFunc {
			return func(b []byte) (int, error) {
				if !rrw.written {
					rrw.written = true
				}
				return next(b)
			}
		},
		WriteHeader: func(next httpsnoop.WriteHeaderFunc) httpsnoop.WriteHeaderFunc {
			return func(statusCode int) {
				if !rrw.written {
					rrw.written = true
					rrw.status = statusCode
				}
				next(statusCode)
			}
		},
	})
	return rrw
}

I would also change existing tests by removing w.WriteHeader(http.StatusOK) calls. Maybe an additional unit test as well... Feel free to submit a PR as I don't have time to do it in the upcoming days.

[Aneurysm9]

The current implementation will set rww.written when either Write() or WriteHeader() are called. Does the instrumented handler not invoke either? It seems to me to be an error for an HTTP handler to not write any response. At the very least if there is no content to return then WriteHeader(http.StatusNoContent) should be used.

[floyola]

The weird thing is that the 30% percent of the traces have a valid status code 🤔
And yeah, I'm calling the WriteHeader

[pellared]

It seems to me to be an error for an HTTP handler to not write any response. At the very least if there is no content to return then WriteHeader(http.StatusNoContent) should be used.

I agree that WriteHeader(http.StatusNoContent) should be used in application code in such a case.

Anyway, I would prefer to double-check what http.Server does when none of Write() and WriteHeader() are called (and to the same).

And yeah, I'm calling the WriteHeader

Then maybe it's some other issue... 🤔

[pellared]

https://play.golang.org/p/wQPh-KXIQjm

Therefore by default, it should be http.StatusOK.

[floyola]

After a lot of debugging I found that there was a unhandled case in my implementation that it didn't call the WriteHeader() and thus, since the default code was 0 then it was marked as an error.
My apologies 😬

But I agree that the semantics of the default status code of the response span should be the same as in mux library

[pellared]

PR ready for review #1443