Use felixge/httpsnoop approach to preserve additional interfaces

[mike-zorn]

Just like #25 and #11, but I've used httpsnoop.Wrap instead of httpsnoop.CaptureMetrics. There is a performance overhead here, so I added a benchmark to measure it. On my machine, I get the following results

Before

BenchmarkStatusCodeTrackingOverhead-8   	   10000	    285757 ns/op	   19122 B/op	     151 allocs/op

After (httpsnoop.Wrap)

This one is what I've actually implemented

BenchmarkStatusCodeTrackingOverhead-8   	   10000	    260020 ns/op	   19427 B/op	     159 allocs/op

After (httpsnoop.CaptureMetrics as in #11)

This is what is in #11 for comparison.

BenchmarkStatusCodeTrackingOverhead-8   	   10000	    380752 ns/op	   19498 B/op	     164 allocs/op

I'm not sure what the performance expectations of this package are, so if this is not sufficient, I think this could be refined further by copying the codegen approach in httpsnoop that underlies httpsnoop.Wrap. Let me know and I'd be happy to give that a shot.

[yurishkuro]

any idea where the extra allocations are coming from?

I don't think there are specific perm expectations, as I haven't seen any benchmarks before or specific user requests. The StatusCodeTracer clearly seemed like an overkill, and your benchmark proves it.

[mike-zorn]

Looking at the escape analysis logs, It looks like the two new function literals account for two of the allocations. The pointer to status escapes as well, but so does the statusCodeTracker in the previous implementation, so that one cancels out. In the implementation of httpsnoop.Wrap, it looks like the struct that unions all the interfaces escapes as well as the implementations of these interfaces.

For comparison, Opencensus has a probably more performant solution that is also less correct because it will pass type assertions that it really shouldn't in some scenarios.

[yurishkuro]

it appears OpenCensus code has been upgraded to use an approach similar to httpsnoop

https://github.com/census-instrumentation/opencensus-go/blob/1eb9a13c7dd02141e065a665f6bf5c99a090a16a/plugin/ochttp/server.go#L217-L439

[kujenga]

I closed out my PR #25 since it sounds like this is the favored approach. @yurishkuro what would you like to see here? Anything I could help with to move this forward?

[yurishkuro]

We can probably merge this. There's a way to improve performance, but not sure how critical it is for this lib.

[kujenga]

Great! Looking forward to having it brought in 🎉

[vtolstov]

any steps to get this merged?

[henrywallace]

What remains to have this merged, besides a rebase? I'd be happy to help if there's anything remaining :)

[yurishkuro]

My suggesting is to not use httpsnoop (unnecessary external dependency), and instead implement the same way as done in OpenCensus

https://github.com/census-instrumentation/opencensus-go/blob/1eb9a13c7dd02141e065a665f6bf5c99a090a16a/plugin/ochttp/server.go#L217-L439

[mike-zorn]

I updated this to use that implementation. New benchmark results follow.

BenchmarkStatusCodeTrackingOverhead-8              10000            101246 ns/op           19103 B/op        150 allocs/op

[vtolstov]

great! @yurishkuro please, merge it =)

[andremarianiello]

Gonna be using this branch until it's merged. Need this for grpc streaming responses.

[yurishkuro]

httpsnoop covers 5 interfaces, but this code only covers 4, any reason? Is it because of older versions of Go? I think we need to drop support for anything but N and N-1 versions, per Go language own policy.

// - http.Flusher
// - http.CloseNotifier
// - http.Hijacker
// - io.ReaderFrom
// - http.Pusher

[yurishkuro]

I see this is for "old" - I think we should drop this file altogether

[yurishkuro]

new PR - #36