Allow respWriterWrapper to call ResponseWriter.WriteHeader multiple times

[jarlefosen]

otelhttp respWriterWrapper blocks multiple calls to underlying WriteHeader which makes it harder to detect bugs in code where multiple calls to WriteHeader would be caught and logged by net/http.

This PR attempts to expose incorrect code by not blocking multiple calls to the underlying ResponseWriter.WriteHeader.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jarlefosen / name: Jarle Fosen (e24fc52)

[dmathieu]

This also seems to change behavior. Could you write a test?

[codecov]

Codecov Report

Merging #3580 (ed5e665) into main (f398558) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #3580     +/-   ##
=======================================
- Coverage   70.1%   70.1%   -0.1%     
=======================================
  Files        147     147             
  Lines       6974    6973      -1     
=======================================
- Hits        4895    4891      -4     
- Misses      1957    1959      +2     
- Partials     122     123      +1     

Impacted Files	Coverage Δ
instrumentation/net/http/otelhttp/wrap.go	85.1% <100.0%> (+10.1%)	⬆️

... and 2 files with indirect coverage changes

[jarlefosen]

You're right. I've implemented tests to ensure we propagate WriteHeader calls to underlying ResponseWriters regardless of respWriterWrapper state.

[jarlefosen]

Thanks for the comment @pellared. I'll answer in the main thread to avoid losing context if the comment is resolved

The primary goal for me is to have our system behave the same with and without otelhttp.

Today my code can work nicely with multiple calls to WriteHeader since otelhttp is a middleware that ensures the ResponseWriter never gets multiple WriteHeader calls.

This is not the responsibility of OpenTelemetry in my opinion, and can lead to unknown bad implementations of HTTP Handlers where WriteHeader is called multiple times, or it is called after ResponseWriter.Write without the developer noticing it. Which incidentally is what happened in our codebase without our knowledge for some time.

I believe that otelhttp should report the initially written statusCode, as it does today, and not change reported status based on bad implementations. Changing the reported statusCode on consecutive calls to WriteHeader would lead to discrepancies between the attributes on the span vs what was actually sent to the client.

The goal of otelhttp would be to report the first written statusCode on the span without filtering out future WriteHeader calls.

If it is something we believe should be implemented I'll go ahead and change the code to document behavior in more detail.

[pellared]

@jarlefosen I think it makes sense especially that now I see that I do not believe anyone expects the ResponseWriter to handle multiple calls to WriteHeader properly. For example from https://pkg.go.dev/net/http#NewResponseController, we have:

The ResponseWriter should be the original value passed to the Handler.ServeHTTP method, or have an Unwrap method returning the original ResponseWriter.

Documenting behavior in more detail seems fine.

[jarlefosen]

@pellared I have finally gotten back to this and updated comments and documentation.
PR is updated and branch is rebased on main.

[pellared]

Thanks for your patience and contribution 👍