From 586178b4ab42bbe57811049a16f1aae5d164feb8 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Fri, 13 Mar 2020 00:10:32 +0100 Subject: [PATCH] Fix promhttp error handling Essentially, just don't try to set a status code and send any error message in the body once metrics gathering has succeeded. At that point, the most likely reason for errors is anyway that the client has disconnected, in which sending an error is moot. The other possible reason for an error is a problem during metrics encoding. This is unlikely to happen (it's a coding error here in client_golang in any case), and if it is happening, the odds are we have already sent something to the ResponseWriter, which means we cannot set a status code anymore. The doc comment for HTTPErrorOnError now describes these circumstances explicitly and recommends to set a logger to report that kind of error. This should fix the reason for the infamous `superfluous response.WriteHeader call` message. Signed-off-by: beorn7 --- prometheus/promhttp/delegator.go | 10 +++++++--- prometheus/promhttp/http.go | 27 +++++++++++++-------------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/prometheus/promhttp/delegator.go b/prometheus/promhttp/delegator.go index d1354b101..5070e72e2 100644 --- a/prometheus/promhttp/delegator.go +++ b/prometheus/promhttp/delegator.go @@ -53,12 +53,16 @@ func (r *responseWriterDelegator) Written() int64 { } func (r *responseWriterDelegator) WriteHeader(code int) { + if r.observeWriteHeader != nil && !r.wroteHeader { + // Only call observeWriteHeader for the 1st time. It's a bug if + // WriteHeader is called more than once, but we want to protect + // against it here. Note that we still delegate the WriteHeader + // to the original ResponseWriter to not mask the bug from it. + r.observeWriteHeader(code) + } r.status = code r.wroteHeader = true r.ResponseWriter.WriteHeader(code) - if r.observeWriteHeader != nil { - r.observeWriteHeader(code) - } } func (r *responseWriterDelegator) Write(b []byte) (int, error) { diff --git a/prometheus/promhttp/http.go b/prometheus/promhttp/http.go index b0ee4678e..5e1c4546c 100644 --- a/prometheus/promhttp/http.go +++ b/prometheus/promhttp/http.go @@ -167,15 +167,12 @@ func HandlerFor(reg prometheus.Gatherer, opts HandlerOpts) http.Handler { enc := expfmt.NewEncoder(w, contentType) - var lastErr error - // handleError handles the error according to opts.ErrorHandling // and returns true if we have to abort after the handling. handleError := func(err error) bool { if err == nil { return false } - lastErr = err if opts.ErrorLog != nil { opts.ErrorLog.Println("error encoding and sending metric family:", err) } @@ -184,7 +181,10 @@ func HandlerFor(reg prometheus.Gatherer, opts HandlerOpts) http.Handler { case PanicOnError: panic(err) case HTTPErrorOnError: - httpError(rsp, err) + // We cannot really send an HTTP error at this + // point because we most likely have written + // something to rsp already. But at least we can + // stop sending. return true } // Do nothing in all other cases, including ContinueOnError. @@ -202,10 +202,6 @@ func HandlerFor(reg prometheus.Gatherer, opts HandlerOpts) http.Handler { return } } - - if lastErr != nil { - httpError(rsp, lastErr) - } }) if opts.Timeout <= 0 { @@ -276,7 +272,12 @@ type HandlerErrorHandling int // errors are encountered. const ( // Serve an HTTP status code 500 upon the first error - // encountered. Report the error message in the body. + // encountered. Report the error message in the body. Note that HTTP + // errors cannot be served anymore once the beginning of a regular + // payload has been sent. Thus, in the (unlikely) case that encoding the + // payload into the negotiated wire format fails, serving the response + // will simply be aborted. Set an ErrorLog in HandlerOpts to detect + // those errors. HTTPErrorOnError HandlerErrorHandling = iota // Ignore errors and try to serve as many metrics as possible. However, // if no metrics can be served, serve an HTTP status code 500 and the @@ -365,11 +366,9 @@ func gzipAccepted(header http.Header) bool { } // httpError removes any content-encoding header and then calls http.Error with -// the provided error and http.StatusInternalServerErrer. Error contents is -// supposed to be uncompressed plain text. However, same as with a plain -// http.Error, any header settings will be void if the header has already been -// sent. The error message will still be written to the writer, but it will -// probably be of limited use. +// the provided error and http.StatusInternalServerError. Error contents is +// supposed to be uncompressed plain text. Same as with a plain http.Error, this +// must not be called if the header or any payload has already been sent. func httpError(rsp http.ResponseWriter, err error) { rsp.Header().Del(contentEncodingHeader) http.Error(