Skip to content

http: stop writing errors back to disconnected metrics clients - #280

Merged
sravotto merged 1 commit into
mainfrom
sr8/fix-metrics-client-disconnect
Jul 14, 2026
Merged

http: stop writing errors back to disconnected metrics clients#280
sravotto merged 1 commit into
mainfrom
sr8/fix-metrics-client-disconnect

Conversation

@sravotto

Copy link
Copy Markdown
Contributor

Problem

Customers on v1.3.1 see repeated Error gathering metrics: client disconnected at error level in the logs. The client disconnected text is errClientDisconnected from golang.org/x/net/http2, returned when visus writes to the HTTP/2 response after a scrape client (Prometheus) has already closed the connection, typically a scrape timeout or cancel.

The metrics handler mishandled this:

  • It called errorResponse from inside the streaming loop, after response bytes had already been written. Once the client is gone, sending a status and body back is futile and just fails again.
  • The loop had no return, so a single disconnect logged one error line per remaining metric family, making one benign disconnect look like many failures.
  • The disconnect was logged at error level, though it is an expected event.

Changes

  • Stop calling errorResponse from the streaming path. On a write failure to the client, log at debug and return after the first error. The registry.Gather() failure path still uses errorResponse, since it fails before any body is written and a 500 there is legitimate.
  • Handle two previously ignored errors on the --prometheus proxy path:
    • metricsWriter.Copy's return value in the handler, logged at error level since a Copy failure usually means the upstream source is unreachable or returned malformed metrics.
    • translator.Translate's return value inside Writer.Copy.
  • Extract the handler into metricsHandler so it can be unit tested.

Note on log levels: the client-write loop logs at debug because a disconnect there is expected and benign (this is the path that produced the customer's noise). The Copy path logs at error because a failure there is usually an actionable upstream-source problem. We deliberately do not string-match the disconnect error, since http2's errClientDisconnected is unexported.

Tests

Added handler tests in internal/http/server_test.go:

  • TestMetricsHandler: a normal scrape returns 200 with the gathered metric.
  • TestMetricsHandlerClientDisconnect: on a failing writer, the handler returns without panicking and never calls WriteHeader, confirming it no longer tries to send an error status to a disconnected client.
  • TestMetricsHandlerCopyError: a bad upstream source makes Copy fail and the handler returns before writing gathered metrics.

The metrics handler called errorResponse after streaming of the response
had already begun. Once a scrape client (for example Prometheus) closes
the connection mid-response, the HTTP/2 server returns "client
disconnected" on the next write, so trying to send an error status and
body back to that client is futile and just fails again. The write loop
also lacked a return, so a single disconnect logged one error line per
remaining metric family, making one benign disconnect look like many
failures.

Stop calling errorResponse from the streaming path. On a write failure to
the client, log at debug (a disconnect is an expected event) and return
after the first error. The registry.Gather() failure path still uses
errorResponse, since it fails before any body is written and a 500 there
is legitimate.

While here, handle two previously ignored errors on the --prometheus
proxy path: metricsWriter.Copy's return value in the handler (logged at
error level, since a Copy failure usually means the upstream source is
unreachable or returned malformed metrics) and translator.Translate's
return value inside Writer.Copy.

Extract the handler into metricsHandler so it can be unit tested, and add
tests for a normal scrape, a client disconnect, and a Copy failure.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@sravotto
sravotto marked this pull request as ready for review July 14, 2026 15:12
@sravotto
sravotto merged commit 2923029 into main Jul 14, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant