Expose optional ResponseWriter interfaces.

[andy-retailnext]

http.ResponseWriters may implement additional interfaces
(http.CloseNotifier, http.Flusher, http.Hijacker, http.Pusher,
io.ReaderFrom) that get lost when the ResponseWriter is wrapped in
another object. This change uses the httpsnoop package to wrap the
ResponseWriter so that the resulting object implements any of the
optional interfaces that the original ResponseWriter implements as
well as using the replacement ResponseWriter methods that gather
information for tracing.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Andy Schweig (74d3355)

[MrAlias]

This PR doesn't seem in line with the purpose of this instrumentation. It does not use OpenTelemetry metric instruments to capture metric events (it doesn't seem to do any tracing either, not sure if I missed that based on the PR description).

If the user desires to expose these interfaces for a handler I don't see why they couldn't wrap their handler directly. It doesn't seem appropriate to make that decision for all users looking to use this instrumentation.

[muirdm]

This PR is not about instrumentation or metrics. It fixes a bug in the otel http handler where the wrapped response writer object loses all the optional interfaces implemented by the underlying http.ResponseWriter. It is not possible for the user to recover or reimplement the missing interfaces.

Currently if you use the otel http handler, the wrapped http.ResponseWriter that gets passed to the user's handlers does not implement any of the optional http interfaces (e.g. http.Hijacker, http.Flusher, etc). This is not acceptable since these interfaces are required for various HTTP applications. There is already a TODO comment in the code admitting this:

// TODO: The wrapped http.ResponseWriter doesn't implement any of the optional
// types (http.Hijacker, http.Pusher, http.CloseNotifier, http.Flusher, etc)
// that may be useful when using it in real life situations.
type respWriterWrapper struct {

Properly wrapping a ResponseWriter to implement all the optional interfaces is not easy. It involves a combinatorial explosion of all the optional interfaces. The "httpsnoop" package handles it for you, and uses code generation to manage the complexity.

[Aneurysm9]

I think this is an important deficiency of the current othttp.Handler to address and I'm generally in favor of the approach. My only concern would be that @MrAlias has previously expressed concern about the use of dependencies with the MIT license. Are there alternatives that are Apache 2.0 licensed?

Also, please ensure that all relevant go.mod and go.sum files are updated, this appears to be the cause of the CI build failures.

[muirdm]

concern about the use of dependencies with the MIT license

What is troublesome about the MIT license?

Are there alternatives that are Apache 2.0 licensed?

We don't know of any alternatives. Some packages just implement the wrapping logic themselves, so that is always an option.

[Aneurysm9]

What is troublesome about the MIT license?

I'm not aware of any problem utilizing MIT-licensed dependencies in an Apache-licensed project, but I'm not capable of speaking for the CNCF or project governance committee. I just know that a maintainer of this project has previously said "[t]he MIT license can be troublesome for dependencies". @MrAlias or @lizthegrey do you have any clarity to share?

[MrAlias]

What is troublesome about the MIT license?

@Aneurysm9 thanks for pointing this out, but I'm pretty sure I was mistaking MPL-2.0 with MIT in that linked comment (the alternate github.com/hashicorp/go-multierror has that license and it looks like I was getting my signals crossed). Definitely my goof 🤦.

@muirdm thanks for the clarification, I need to take a deeper look at this. Do you have any links to where others have wrapped the logic themselves?

[muirdm]

Do you have any links to where others have wrapped the logic themselves?

Opencensus implements something similar to httpsnoop:
https://github.com/census-instrumentation/opencensus-go/blob/d7677d6af5953e0506ac4c08f349c62b917a443a/plugin/ochttp/server.go#L230

The AWS SDK xray implements something similar to httpsnoop:
https://github.com/aws/aws-xray-sdk-go/blob/aa6a33f684fd0b1760ac6e9b0bf0590f0dfb9bad/xray/response_capturer.go#L29

The gorilla "compress" handler attempts to wrap, but it does it incorrectly since it implements all the optional interfaces even if the underlying ResponseWriter doesn't (which causes panics):
https://github.com/gorilla/handlers/blob/f08afc1876ad882db8074bcb8a4cc96107d3a5f4/compress.go#L132

The gorilla "logging" handler implements it for a couple interfaces:
https://github.com/gorilla/handlers/blob/f08afc1876ad882db8074bcb8a4cc96107d3a5f4/logging.go#L61

[MrAlias]

Opencensus implements something similar to httpsnoop:
https://github.com/census-instrumentation/opencensus-go/blob/d7677d6af5953e0506ac4c08f349c62b917a443a/plugin/ochttp/server.go#L230

The AWS SDK xray implements something similar to httpsnoop:
https://github.com/aws/aws-xray-sdk-go/blob/aa6a33f684fd0b1760ac6e9b0bf0590f0dfb9bad/xray/response_capturer.go#L29

The gorilla "compress" handler attempts to wrap, but it does it incorrectly since it implements all the optional interfaces even if the underlying ResponseWriter doesn't (which causes panics):
https://github.com/gorilla/handlers/blob/f08afc1876ad882db8074bcb8a4cc96107d3a5f4/compress.go#L132

The gorilla "logging" handler implements it for a couple interfaces:
https://github.com/gorilla/handlers/blob/f08afc1876ad882db8074bcb8a4cc96107d3a5f4/logging.go#L61

Similar this prior art ^, and to what was pointed out here, would it be a better approach to avoid the external dependency and implement here?

[MrAlias]

Guessing this isn't going to be resolved before we want this in GA.

[muirdm]

would it be a better approach to avoid the external dependency and implement here?

Personally I think the dependency is worth it because it is hard to implement correctly, and it will require maintenance as they add more http interfaces to the standard library. For example, the httpsnoop package supports Go versions from before http.Pusher was added.

On the other hand, I don't really like httpsnoop's interface, and can appreciate not wanting third party dependencies in a library.

Up to you. We will be happy as long as the wrapping is fixed one way or another.

[MrAlias]

I think the dependency is worth it because it is hard to implement correctly, and it will require maintenance as they add more http interfaces to the standard library.

I think this is adequate motivation, and I agree. I think we should move in this direction.

@andy-retailnext can you sign the CLA, make the fixes @Aneurysm9 mentioned, and add the fix to the change log?

[andy-retailnext]

We're working out the details of signing the CLA, but I'll sign it as soon as that's taken care of. I updated the commit with the additional go.sum changes. The build passes now.

[MrAlias]

@andy-retailnext looks like edits by maintainers is off. Can you update the Changelog with this fix and sync with the base branch?

[jmacd]

I think the dependency looks OK. The OpenCensus code didn't look bad either but also looks like a lot to maintain.

[andy-retailnext]

@MrAlias I updated the change log and rebased my branch onto the latest master.

[MrAlias]

@andy-retailnext looks like the build is failing from uncommitted changes to some go.sum files. Please run make precommit and commit the changes.

[andy-retailnext]

@MrAlias Fixed.

[MrAlias]

@andy-retailnext if you can sync with master I can include it in #990

[andy-retailnext]

@MrAlias Done.