fix: hide HTTP scrape error details - #2332
Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
|
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
| "The Prometheus metrics HTTPServer caught an Exception while trying to send " | ||
| + "the metrics response.", | ||
| requestHandlerException); | ||
| logger.log( |
There was a problem hiding this comment.
This branch never calls errorHandlingPolicy.report(...) — it logs at SEVERE unconditionally
instead. Two things fall out of that:
The policy leaks. An application that wired errorReporter into its telemetry pipeline
silently misses this entire class of failure. It gets no callback, and the exception lands in
java.util.logging — exactly the destination the policy exists to avoid.
The Java-agent scenario this PR is built for is unfixed here. A collector that throws
deterministically during writer.write(...) reaches handleException with responseSent == true on every single scrape. That's a SEVERE + full stack trace into the host application's
logs every 15s, with no way to turn it off — which is the original complaint, on a different
code path.
Suggested shape (needs a package-private boolean hasErrorReporter() on the policy):
} else {
// If the exception occurs after response headers have been sent, it's too late to respond
// with HTTP 500.
if (errorHandlingPolicy.hasErrorReporter()) {
reportException(requestHandlerException);
} else {
logger.log(
Level.SEVERE,
"The Prometheus metrics HTTPServer caught an Exception while trying to send "
+ "the metrics response.",
requestHandlerException);
}
}Always-report-and-log-only-when-unconfigured works too — the requirement is just that a
configured reporter sees it.
There's no test covering this branch today, which is how it slipped through. A test that calls
sendHeadersAndGetBody(200, 0) and then handleException(...) would pin it.
| this(new PrometheusScrapeHandler(config, registry), HttpErrorHandlingPolicy.builder().build()); | ||
| } | ||
|
|
||
| MetricsHandler( |
There was a problem hiding this comment.
This overload is package-private, so only HTTPServer.Builder can reach it — but
MetricsHandler is @StableApi with four public constructors, and its whole reason to exist as
a public HttpHandler is registering it on a user-owned com.sun.net.httpserver.HttpServer
without going through HTTPServer.
For those users this PR is a strict regression rather than a configurable tradeoff: the old
behavior (stack trace to the client) is gone and the new behavior (reporter) is unreachable.
They get a bare 500 telling them to "Configure an HTTP error reporter for details" with no API
to do so.
Same applies to HttpExchangeAdapter's package-private constructor
(HttpExchangeAdapter.java:30), which is the documented seam for wiring
PrometheusScrapeHandler into any HttpExchange-based server.
Both are purely additive and would show up cleanly in the api-diff:
public MetricsHandler(
PrometheusProperties config,
PrometheusRegistry registry,
HttpErrorHandlingPolicy errorHandlingPolicy) { ... }
public HttpExchangeAdapter(HttpExchange httpExchange, HttpErrorHandlingPolicy policy) { ... }| By default, scrape failures return a generic HTTP 500 response. Exception details are not | ||
| included in the response or logged, because the server may run inside an application or a | ||
| Java agent with its own diagnostic pipeline. |
There was a problem hiding this comment.
"or logged" isn't accurate — the default policy does log, at SEVERE, with a full throwable, in
two paths: HttpExchangeAdapter:120-128 (couldn't write the 500) and :133-137 (headers already
sent). If we don't take the change in my other comment, the second one is routine rather than
exceptional, which makes this sentence actively misleading for the agent use case it's written
for.
| By default, scrape failures return a generic HTTP 500 response. Exception details are not | |
| included in the response or logged, because the server may run inside an application or a | |
| Java agent with its own diagnostic pipeline. | |
| By default, scrape failures return a generic HTTP 500 response. Exception details are not | |
| included in the response, and are not logged when the error response can be delivered, because | |
| the server may run inside an application or a Java agent with its own diagnostic pipeline. |
(If the response itself can't be delivered, logging is the only remaining signal — worth saying
so explicitly rather than leaving it as a surprise.)
| * must be safe to call concurrently. Runtime exceptions thrown by the reporter are isolated | ||
| * from HTTP response handling. | ||
| */ | ||
| public Builder errorReporter(Consumer<Throwable> errorReporter) { |
There was a problem hiding this comment.
Only Exception is ever delivered here — handleException has exactly two overloads
(IOException, RuntimeException) and PrometheusScrapeHandler:99-103 catches exactly those
two. A bare Throwable can't reach this consumer.
Consumer<? super Exception> would be both more honest about that contract and strictly more
permissive: it accepts Consumer<Exception>, Consumer<Throwable>, and Consumer<Object>,
where today someone holding a Consumer<Exception> needs a cast or a wrapper lambda.
Raising it now rather than as a nit because @StableApi on line 17 freezes this signature at
merge. Existing-style callers keep compiling (contravariance covers
Consumer<Throwable> r = ...; policy.errorReporter(r), and erasure is identical), so it's
cheap now and impossible later.
Related, same "frozen at merge" reasoning: a named @FunctionalInterface ScrapeErrorReporter
instead of Consumer would leave room to add a second parameter later (the failing request, a
scrape context). A Consumer can never grow one. Not saying we need that but a thought.
|
|
||
| private static final byte[] GENERIC_RESPONSE = | ||
| ("An internal error occurred while scraping metrics. " | ||
| + "Configure an HTTP error reporter for details.\n") |
There was a problem hiding this comment.
The out-of-the-box experience for a plain HTTPServer.builder().buildAndStart() in a standalone
app is now: scrape fails → bare 500 → nothing anywhere. This message tells the operator to
configure a reporter, which requires a code change and a redeploy to act on.
The default itself is defensible — agent safety is the harder constraint and it should win. But
the opt-in ergonomics mean everyone writes the same five-line lambda. Worth shipping it:
/** Reports scrape exceptions to {@code java.util.logging} at {@code SEVERE}. */
public static Consumer<Throwable> julReporter() { ... }so the override is .errorReporter(HttpErrorHandlingPolicy.julReporter()). Makes "we
deliberately don't do this for you" a one-liner to reverse instead of a research task.
| private void sendErrorResponse(Exception requestHandlerException) { | ||
| if (!responseSent) { | ||
| responseSent = true; | ||
| reportException(requestHandlerException); |
There was a problem hiding this comment.
The reporter runs before the 500 is written. The javadoc says it "should return promptly," but a
reporter that blocks — synchronous telemetry export, saturated appender — delays the client's
500, and one that hangs prevents it entirely. Moving this call below the try/catch guarantees
the response goes out first and costs nothing.
If you do move it, the "Original Exception that caused the Prometheus scrape error" log at
:125-128 becomes redundant whenever a reporter is configured — it has already seen the
exception by then. Logging only errorWriterException there would keep that path quiet for
agent users.
Summary
This is the focused replacement for the #2283 portion of #2297.
Fixes #2283
Implementation plan
HttpErrorHandlingPolicybuilder-based and pass the built policy through theHTTPServerbuilder to the exchange adapter. Response verbosity and error reporting remain orthogonal choices.Alternatives considered
HttpErrorHandlingPolicy, so re-enabling unsafe debug responses is deliberate and carries a clear security warning.Validation
mise run lint:fixmise run build./mvnw test -pl prometheus-metrics-exporter-httpserver -Dcoverage.skip=true -Dcheckstyle.skip=trueRelease note
Release Please will use this override for the generated changelog and GitHub release notes after a squash merge:
BEGIN_COMMIT_OVERRIDE
fix(httpserver): make scrape error responses secure and configurable
Scrape failures now return a generic HTTP 500 response by default. Applications can configure a server-side error reporter or explicitly enable an unsafe debug response containing exception details. See the HTTPServer scrape error handling documentation.
END_COMMIT_OVERRIDE