miekg / caddy-prometheus

Prometheus metrics middleware for caddy
Apache License 2.0
65 stars 26 forks source link

Using this plugin with websockets causes a panic #43

Open wade-p opened 6 years ago

wade-p commented 6 years ago

from the error log: 26/May/2018:23:46:16 +0000 [PANIC /] caddyhttp/proxy/reverseproxy.go:352 - *metrics.timedResponseWriter is not a hijacker

I think that can be fixed by implementing the Hijacker interface: https://golang.org/src/net/http/server.go#L174

It looks like it could be similar to how WriteHeader works and just call and return Hijack after calling w.didWrite()

https://golang.org/src/net/http/server.go#L1883

hairyhenderson commented 6 years ago

I think #42 fixes this

genofire commented 5 years ago

Found it eigther

[PANIC] *metrics.timedResponseWriter is not a hijacker

Thats really annoying, that this request crashed.

@miekg Whats the status ?

miekg commented 5 years ago

[ Quoting notifications@github.com in "Re: [miekg/caddy-prometheus] Using ..." ]

Found it eigther

[PANIC] *metrics.timedResponseWriter is not a hijacker

Thats really annoying, that this request crashed.

@miekg Whats the status ?

zero - i don't have time to work on this

hairyhenderson commented 5 years ago

@genofire are you able to try the patch in #42 to see if it fixes the bug for you?

(note that I also don't have time to look deeply into this, but the description mentioned implementing http.Hijacker as a potential fix, which is what #42 is about)

wade-p commented 5 years ago

Additionally, here is the patch I wrote which has been working well for me.

index 6d40684..fde1810 100644
--- a/handler.go
+++ b/handler.go
@@ -1,6 +1,7 @@
 package metrics

 import (
+       "bufio"
        "net"
        "net/http"
        "strconv"
@@ -92,6 +93,7 @@ func isIPv6(addr string) bool {
 type timedResponseWriter struct {
        firstWrite time.Time
        http.ResponseWriter
+       http.Hijacker
 }

 func (w *timedResponseWriter) didWrite() {
@@ -111,3 +113,8 @@ func (w *timedResponseWriter) WriteHeader(statuscode int) {
        w.didWrite()
        w.ResponseWriter.WriteHeader(statuscode)
 }
+
+func (w *timedResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
+       w.didWrite()
+       return w.Hijacker.Hijack()
+}
BobCashStory commented 5 years ago

same for us

lou-lan commented 5 years ago

I am get

*metrics.timedResponseWriter is not a closeNotifier

Implement the CloseNotifier interface. To solve my problem.

mh720 commented 5 years ago

Adding feedback for this open issue, I hadn't previously experienced a problem using the prometheus module with websockets (works fine for Grafana), until I needed to downgrade the HTTP version to http/1.1 (working around an issue with Portainer's console access):

{$CADDY_URL}:9000 {
  log / stdout "{combined}"
  errors stderr
  #prometheus 0.0.0.0:9180 {
  #  hostname 9000
  #}
  tls {$CADDY_CERT} {$CADDY_KEY} {
    alpn http/1.1
  }

  proxy / portainer:9000 {
    transparent
    websocket
  }
}

Enabling the prometheus module above results in caddy log output:

swarmstack_caddy.1.ez5xlkrtvr5e@host.example.com | 2019/08/10 13:49:22 [PANIC] *metrics.timedResponseWriter is not a hijacker

lou-lan commented 5 years ago

hotfix, see https://github.com/miekg/caddy-prometheus/pull/42, it is work

mh720 commented 5 years ago

Tested in Docker (https://github.com/swarmstack/caddy). Summited a PR to #42 patch author to update github repos from mholt to caddyserver.