-
Notifications
You must be signed in to change notification settings - Fork 833
fix: hide HTTP scrape error details #2332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,21 @@ | ||
| package io.prometheus.metrics.it.exporter.test; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import java.io.IOException; | ||
| import java.net.URISyntaxException; | ||
|
|
||
| class HttpServerIT extends ExporterIT { | ||
| public HttpServerIT() throws IOException, URISyntaxException { | ||
| super("exporter-httpserver-sample"); | ||
| } | ||
|
|
||
| @Override | ||
| protected void assertErrorResponseBody(String body) { | ||
| assertThat(body) | ||
| .isEqualTo( | ||
| "An internal error occurred while scraping metrics. " | ||
| + "Configure an HTTP error reporter for details.\n") | ||
| .doesNotContain("Simulating an error."); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| package io.prometheus.metrics.exporter.httpserver; | ||
|
|
||
| import io.prometheus.metrics.annotations.StableApi; | ||
| import java.io.PrintWriter; | ||
| import java.io.StringWriter; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.util.function.Consumer; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| /** | ||
| * Controls how the {@link HTTPServer} handles exceptions raised while scraping metrics. | ||
| * | ||
| * <p>The default policy built by {@link #builder()} does not expose exception details and does not | ||
| * report the exception. Configure the builder to route diagnostic details to an | ||
| * application-appropriate sink. | ||
| */ | ||
| @StableApi | ||
| public final class HttpErrorHandlingPolicy { | ||
|
|
||
| private static final byte[] GENERIC_RESPONSE = | ||
| ("An internal error occurred while scraping metrics. " | ||
| + "Configure an HTTP error reporter for details.\n") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The out-of-the-box experience for a plain The default itself is defensible — agent safety is the harder constraint and it should win. But /** Reports scrape exceptions to {@code java.util.logging} at {@code SEVERE}. */
public static Consumer<Throwable> julReporter() { ... }so the override is |
||
| .getBytes(StandardCharsets.UTF_8); | ||
|
|
||
| private final boolean unsafeDebugResponse; | ||
| @Nullable private final Consumer<Throwable> errorReporter; | ||
|
|
||
| private HttpErrorHandlingPolicy( | ||
| boolean unsafeDebugResponse, @Nullable Consumer<Throwable> errorReporter) { | ||
| this.unsafeDebugResponse = unsafeDebugResponse; | ||
| this.errorReporter = errorReporter; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a builder for configuring scrape error handling. | ||
| * | ||
| * <p>The builder defaults to a generic HTTP 500 response with no error reporter. This avoids | ||
| * exposing exception details to scrape clients or adding an implicit dependency on an | ||
| * application's logging configuration. | ||
| */ | ||
| public static Builder builder() { | ||
| return new Builder(); | ||
| } | ||
|
|
||
| byte[] getErrorResponse(Exception exception) { | ||
| if (!unsafeDebugResponse) { | ||
| return GENERIC_RESPONSE; | ||
| } | ||
| StringWriter stringWriter = new StringWriter(); | ||
| PrintWriter printWriter = new PrintWriter(stringWriter); | ||
| printWriter.write("An Exception occurred while scraping metrics: "); | ||
| exception.printStackTrace(printWriter); | ||
| return stringWriter.toString().getBytes(StandardCharsets.UTF_8); | ||
| } | ||
|
|
||
| void report(Throwable error) { | ||
| if (errorReporter != null) { | ||
| errorReporter.accept(error); | ||
| } | ||
| } | ||
|
|
||
| /** Builder for {@link HttpErrorHandlingPolicy}. */ | ||
| public static final class Builder { | ||
|
|
||
| private boolean unsafeDebugResponse = false; | ||
| @Nullable private Consumer<Throwable> errorReporter; | ||
|
|
||
| private Builder() {} | ||
|
|
||
| /** | ||
| * Pass scrape exceptions to {@code errorReporter}. | ||
| * | ||
| * <p>The reporter runs synchronously on the HTTP request thread. It should return promptly and | ||
| * must be safe to call concurrently. Runtime exceptions thrown by the reporter are isolated | ||
| * from HTTP response handling. | ||
| */ | ||
| public Builder errorReporter(Consumer<Throwable> errorReporter) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only
Raising it now rather than as a nit because Related, same "frozen at merge" reasoning: a named |
||
| if (errorReporter == null) { | ||
| throw new NullPointerException("errorReporter"); | ||
| } | ||
| this.errorReporter = errorReporter; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Configure whether the HTTP 500 response includes the full exception stack trace. | ||
| * | ||
| * <p><strong>Security warning:</strong> Setting this to {@code true} exposes internal exception | ||
| * information to scrape clients. Do not enable it for endpoints reachable by untrusted clients. | ||
| * | ||
| * <p>This setting is independent of {@link #errorReporter(Consumer)}. | ||
| */ | ||
| public Builder unsafeDebugResponse(boolean unsafeDebugResponse) { | ||
| this.unsafeDebugResponse = unsafeDebugResponse; | ||
| return this; | ||
| } | ||
|
|
||
| /** Build the policy. */ | ||
| public HttpErrorHandlingPolicy build() { | ||
| return new HttpErrorHandlingPolicy(unsafeDebugResponse, errorReporter); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,7 @@ | |
| import io.prometheus.metrics.exporter.common.PrometheusHttpResponse; | ||
| import java.io.IOException; | ||
| import java.io.OutputStream; | ||
| import java.io.PrintWriter; | ||
| import java.io.StringWriter; | ||
| import java.net.URI; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.util.Collections; | ||
| import java.util.Enumeration; | ||
| import java.util.List; | ||
|
|
@@ -18,13 +15,21 @@ | |
|
|
||
| public class HttpExchangeAdapter implements PrometheusHttpExchange { | ||
|
|
||
| private static final Logger logger = Logger.getLogger(HttpExchangeAdapter.class.getName()); | ||
|
|
||
| private final HttpExchange httpExchange; | ||
| private final HttpErrorHandlingPolicy errorHandlingPolicy; | ||
| private final HttpRequest request = new HttpRequest(); | ||
| private final HttpResponse response = new HttpResponse(); | ||
| private volatile boolean responseSent = false; | ||
|
|
||
| public HttpExchangeAdapter(HttpExchange httpExchange) { | ||
| this(httpExchange, HttpErrorHandlingPolicy.builder().build()); | ||
| } | ||
|
|
||
| HttpExchangeAdapter(HttpExchange httpExchange, HttpErrorHandlingPolicy errorHandlingPolicy) { | ||
| this.httpExchange = httpExchange; | ||
| this.errorHandlingPolicy = errorHandlingPolicy; | ||
| } | ||
|
|
||
| public class HttpRequest implements PrometheusHttpRequest { | ||
|
|
@@ -92,52 +97,53 @@ public HttpResponse getResponse() { | |
|
|
||
| @Override | ||
| public void handleException(IOException e) throws IOException { | ||
| sendErrorResponseWithStackTrace(e); | ||
| sendErrorResponse(e); | ||
| } | ||
|
|
||
| @Override | ||
| public void handleException(RuntimeException e) { | ||
| sendErrorResponseWithStackTrace(e); | ||
| sendErrorResponse(e); | ||
| } | ||
|
|
||
| private void sendErrorResponseWithStackTrace(Exception requestHandlerException) { | ||
| private void sendErrorResponse(Exception requestHandlerException) { | ||
| if (!responseSent) { | ||
| responseSent = true; | ||
| reportException(requestHandlerException); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reporter runs before the 500 is written. The javadoc says it "should return promptly," but a If you do move it, the "Original Exception that caused the Prometheus scrape error" log at |
||
| byte[] errorResponse = errorHandlingPolicy.getErrorResponse(requestHandlerException); | ||
| try { | ||
| StringWriter stringWriter = new StringWriter(); | ||
| PrintWriter printWriter = new PrintWriter(stringWriter); | ||
| printWriter.write("An Exception occurred while scraping metrics: "); | ||
| requestHandlerException.printStackTrace(new PrintWriter(printWriter)); | ||
| byte[] stackTrace = stringWriter.toString().getBytes(StandardCharsets.UTF_8); | ||
| httpExchange.getResponseHeaders().set("Content-Type", "text/plain; charset=utf-8"); | ||
| httpExchange.sendResponseHeaders(500, stackTrace.length); | ||
| httpExchange.getResponseBody().write(stackTrace); | ||
| httpExchange.sendResponseHeaders(500, errorResponse.length); | ||
| httpExchange.getResponseBody().write(errorResponse); | ||
| } catch (IOException errorWriterException) { | ||
| // We want to avoid logging so that we don't mess with application logs when the HTTPServer | ||
|
zeitlinger marked this conversation as resolved.
|
||
| // is used in a Java agent. | ||
| // However, if we can't even send an error response to the client there's nothing we can do | ||
| // but logging a message. | ||
| Logger.getLogger(this.getClass().getName()) | ||
| .log( | ||
| Level.SEVERE, | ||
| "The Prometheus metrics HTTPServer caught an Exception during scrape and " | ||
| + "failed to send an error response to the client.", | ||
| errorWriterException); | ||
| Logger.getLogger(this.getClass().getName()) | ||
| .log( | ||
| Level.SEVERE, | ||
| "Original Exception that caused the Prometheus scrape error:", | ||
| requestHandlerException); | ||
| // If we can't even send an error response to the client, logging is the only remaining | ||
| // signal. | ||
| logger.log( | ||
| Level.SEVERE, | ||
| "The Prometheus metrics HTTPServer caught an Exception during scrape and " | ||
| + "failed to send an error response to the client.", | ||
| errorWriterException); | ||
| logger.log( | ||
| Level.SEVERE, | ||
| "Original Exception that caused the Prometheus scrape error:", | ||
| requestHandlerException); | ||
| } | ||
| } else { | ||
| // If the exception occurs after response headers have been sent, it's too late to respond | ||
| // with HTTP 500. | ||
| Logger.getLogger(this.getClass().getName()) | ||
| .log( | ||
| Level.SEVERE, | ||
| "The Prometheus metrics HTTPServer caught an Exception while trying to send " | ||
| + "the metrics response.", | ||
| requestHandlerException); | ||
| logger.log( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This branch never calls The policy leaks. An application that wired The Java-agent scenario this PR is built for is unfixed here. A collector that throws Suggested shape (needs a package-private } 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 There's no test covering this branch today, which is how it slipped through. A test that calls |
||
| Level.SEVERE, | ||
| "The Prometheus metrics HTTPServer caught an Exception while trying to send " | ||
| + "the metrics response.", | ||
| requestHandlerException); | ||
| } | ||
| } | ||
|
|
||
| private void reportException(Exception requestHandlerException) { | ||
| try { | ||
| errorHandlingPolicy.report(requestHandlerException); | ||
| } catch (RuntimeException ignored) { | ||
| // A caller-supplied reporter must not prevent the safe error response from being sent or | ||
| // implicitly fall back to application logging. | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"or logged" isn't accurate — the default policy does log, at
SEVERE, with a full throwable, intwo paths:
HttpExchangeAdapter:120-128(couldn't write the 500) and:133-137(headers alreadysent). 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.
(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.)