Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow respWriterWrapper to call ResponseWriter.WriteHeader multiple times #3580

Merged
merged 14 commits into from
Apr 26, 2023

Conversation

jarlefosen
Copy link
Contributor

@jarlefosen jarlefosen commented Mar 13, 2023

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
Copy link

linux-foundation-easycla bot commented Mar 13, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

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

@jarlefosen jarlefosen marked this pull request as ready for review March 13, 2023 14:19
@jarlefosen jarlefosen requested a review from a team March 13, 2023 14:19
@dmathieu
Copy link
Member

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

@codecov
Copy link

codecov bot commented Mar 13, 2023

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

Impacted file tree graph

@@           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
Copy link
Contributor Author

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

@jarlefosen jarlefosen force-pushed the ensure-writeheader-call-chain branch from 4346329 to 5454b80 Compare March 13, 2023 16:48
CHANGELOG.md Outdated Show resolved Hide resolved
instrumentation/net/http/otelhttp/wrap.go Show resolved Hide resolved
@jarlefosen
Copy link
Contributor Author

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
Copy link
Member

@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.

…imes

otelhttp respWriterWrapper no longer interrupts WriteHeader calls to underlying ResponseWriters.
This allows stdlib net/http to handle and log incorrect usage of WriteHeader.
We explicitly capture initial statusCode written while propagating all
consecutive calls to the wrapped ResponseWriter without altering state.
@jarlefosen jarlefosen force-pushed the ensure-writeheader-call-chain branch from a9ecdc7 to 563882f Compare March 17, 2023 14:18
@jarlefosen
Copy link
Contributor Author

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

CHANGELOG.md Outdated Show resolved Hide resolved
instrumentation/net/http/otelhttp/wrap.go Outdated Show resolved Hide resolved
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience and contribution 👍

@MrAlias MrAlias merged commit 830138c into open-telemetry:main Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants