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 <[email protected]>

Author: beorn7

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) {

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(