HTTP instrumentation doesn't work properly with http/2

[fd]

• Tests
• Results

The http.ResponseWrite passed to the inner handler doesn't implement http.Flusher, http.CloseNotifier and http.Pusher when using http/2.

[beorn7]

InstrumentHandler is massively deprecated. There won't be any fixes or maintenance on it. We are already working on a replacement. I'll add this concern to the doc comment.

[fd]

@beorn7 thanks. Is there a place where I can investigate the replacement?

[beorn7]

It will show up in branches/PRs ASAP. The tracking issue is #224 .

[jeromegn]

The tracked issue has been closed, but I'm still seeing issues.

2017/05/31 14:22:43 http2: panic serving 127.0.0.1:59913: interface conversion: *promhttp.pushDelegator is not http.CloseNotifier: missing method CloseNotify
goroutine 148 [running]:
net/http.(*http2serverConn).runHandler.func1(0xc420162058, 0xc42050bfaf, 0xc4203348c0)
  /usr/local/opt/go/libexec/src/net/http/h2_bundle.go:4592 +0x190
panic(0x15d9be0, 0xc4201db440)
  /usr/local/opt/go/libexec/src/runtime/panic.go:489 +0x2cf
github.com/<my-repo>/vendor/github.com/bugsnag/bugsnag-go.(*Notifier).AutoNotify(0xc420194020, 0xc4202ce1a0, 0x2, 0x2)
  /Users/jerome/go/src/github.com/<my-repo>/vendor/github.com/bugsnag/bugsnag-go/notifier.go:72 +0x138
panic(0x15d9be0, 0xc4201db440)
  /usr/local/opt/go/libexec/src/runtime/panic.go:489 +0x2cf
github.com/<my-repo>/vendor/github.com/romainmenke/pusher/link.(*responseWriter).CloseNotify(0xc4202ce060, 0x19823a0)
  /Users/jerome/go/src/github.com/<my-repo>/vendor/github.com/romainmenke/pusher/link/response_writer_go18.go:70 +0x43
github.com/<my-repo>/proxy.(*ResponseWriter).CloseNotify(0xc42026a4b0, 0xc42020a540)
  /Users/jerome/go/src/github.com/<my-repo>/proxy/response_writer.go:63 +0x60
main.prepareOutRequest(0xc42026a4b0, 0xc42000be00, 0x1688bc9, 0x1c)
  /Users/jerome/go/src/github.com/<my-repo>/proxy_handler.go:426 +0xac
main.(*ProxyHandler).ServeHTTP(0xc420015680, 0x19823a0, 0xc4202ce060, 0xc42000be00)
  /Users/jerome/go/src/github.com/<my-repo>/proxy_handler.go:178 +0x30d
github.com/<my-repo>/vendor/github.com/NYTimes/gziphandler.NewGzipLevelAndMinSize.func1.1(0x19823a0, 0xc4202ce060, 0xc42000be00)
  /Users/jerome/go/src/github.com/<my-repo>/vendor/github.com/NYTimes/gziphandler/gzip.go:263 +0x213
net/http.HandlerFunc.ServeHTTP(0xc42014e3c0, 0x19823a0, 0xc4202ce060, 0xc42000be00)
  /usr/local/opt/go/libexec/src/net/http/server.go:1942 +0x44
github.com/<my-repo>/vendor/github.com/romainmenke/pusher/link.Handler.func1(0x1f42968, 0xc4201f6018, 0xc42000be00)
  /Users/jerome/go/src/github.com/<my-repo>/vendor/github.com/romainmenke/pusher/link/link_go18.go:28 +0xe3
net/http.HandlerFunc.ServeHTTP(0xc42014cb80, 0x1f42968, 0xc4201f6018, 0xc42000be00)
  /usr/local/opt/go/libexec/src/net/http/server.go:1942 +0x44
github.com/<my-repo>/vendor/github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerCounter.func1(0x19825a0, 0xc420162058, 0xc42000be00)
  /Users/jerome/go/src/github.com/<my-repo>/vendor/github.com/prometheus/client_golang/prometheus/promhttp/instrument_server.go:100 +0xdb
net/http.HandlerFunc.ServeHTTP(0xc4201fe030, 0x19825a0, 0xc420162058, 0xc42000be00)
  /usr/local/opt/go/libexec/src/net/http/server.go:1942 +0x44
github.com/<my-repo>/vendor/github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerInFlight.func1(0x19825a0, 0xc420162058, 0xc42000be00)
  /Users/jerome/go/src/github.com/<my-repo>/vendor/github.com/prometheus/client_golang/prometheus/promhttp/instrument_server.go:42 +0xab
net/http.HandlerFunc.ServeHTTP(0xc4201fe060, 0x19825a0, 0xc420162058, 0xc42000be00)
  /usr/local/opt/go/libexec/src/net/http/server.go:1942 +0x44
github.com/<my-repo>/vendor/github.com/bugsnag/bugsnag-go.Handler.func1(0x19825a0, 0xc420162058, 0xc42000be00)
  /Users/jerome/go/src/github.com/<my-repo>/vendor/github.com/bugsnag/bugsnag-go/bugsnag.go:88 +0xce
net/http.HandlerFunc.ServeHTTP(0xc420194040, 0x19825a0, 0xc420162058, 0xc42000be00)
  /usr/local/opt/go/libexec/src/net/http/server.go:1942 +0x44
net/http.serverHandler.ServeHTTP(0xc42012ae70, 0x19825a0, 0xc420162058, 0xc42000be00)
  /usr/local/opt/go/libexec/src/net/http/server.go:2568 +0x92
net/http.initNPNRequest.ServeHTTP(0xc420024380, 0xc42012ae70, 0x19825a0, 0xc420162058, 0xc42000be00)
  /usr/local/opt/go/libexec/src/net/http/server.go:3088 +0x93
net/http.(*initNPNRequest).ServeHTTP(0xc4201dc170, 0x19825a0, 0xc420162058, 0xc42000be00)
  <autogenerated>:312 +0x74
net/http.(Handler).ServeHTTP-fm(0x19825a0, 0xc420162058, 0xc42000be00)
  /usr/local/opt/go/libexec/src/net/http/h2_bundle.go:4319 +0x4d
net/http.(*http2serverConn).runHandler(0xc4203348c0, 0xc420162058, 0xc42000be00, 0xc4202802a0)
  /usr/local/opt/go/libexec/src/net/http/h2_bundle.go:4599 +0x89
created by net/http.(*http2serverConn).startPush.func1
  /usr/local/opt/go/libexec/src/net/http/h2_bundle.go:5246 +0x33e

[beorn7]

Thanks for the catch. I'll work an a fix ASAP (or @stuartnelson3 might… ;).

[stuartnelson3]

Hm, I might need some guidance from @beorn7 after a bit of initial investigation ...

Since we're seeing a panic in a *pushDelegator{}, that would lead me to believe that this code path was executed: https://github.com/prometheus/client_golang/blob/master/prometheus/promhttp/delegator_1_8.go#L49-L51

It seems like we should have gone into the other branch with the fancyPushDelegator if the incoming delegator could be upgraded properly ?

[beorn7]

We do not accommodate all possible combination of interface upgrades (that would be 32 different combinations…), but only four: no upgrade, only Pusher, CloseNotifier & Flusher & Hijacker & ReaderFrom, and all five, assuming these are the common combinations.

But I guess we have now hit another case. The wrapped writer implements Pusher, but not all of the four others, so we end up with the pushDelegator. I guess we need to do some research to find the actually occurring combinations (or bite the bullet and implement all 32… I'm sure somebody wrote some code generation script for that ;).

[beorn7]

@jeromegn Branch https://github.com/prometheus/client_golang/tree/beorn7/http should have the fixed version. Perhaps you have time to verify in your scenario.

[jeromegn]

@beorn7 thanks for bitting the bullet! It does work now.

[beorn7]

\o/

Change is merged. If you build from master, things should work now.