-
Notifications
You must be signed in to change notification settings - Fork 830
Prevent exception in HttpServer exporter when using custom root metrics path #1772
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?
Prevent exception in HttpServer exporter when using custom root metrics path #1772
Conversation
|
it looks as though this is not backwards compatible - maybe add a setting to opt in? |
|
What exactly do you think breaks the backwards compatibility? With Java 11 it should already behave exactly like this (replacing the previously registered default handler). And with a newer Java runtime the current implementation doesn't work at all. |
...orter-httpserver/src/test/java/io/prometheus/metrics/exporter/httpserver/HTTPServerTest.java
Show resolved
Hide resolved
b5ecdd1 to
2377a3f
Compare
That registration would not work the first time What about this change? Index: prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java b/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java
--- a/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java (revision 7f93e015dc64160204b51d13aebf75c83044a364)
+++ b/prometheus-metrics-exporter-httpserver/src/main/java/io/prometheus/metrics/exporter/httpserver/HTTPServer.java (date 1768917816428)
@@ -9,6 +9,9 @@
import com.sun.net.httpserver.HttpsServer;
import io.prometheus.metrics.config.PrometheusProperties;
import io.prometheus.metrics.model.registry.PrometheusRegistry;
+
+import javax.annotation.Nullable;
+import javax.security.auth.Subject;
import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;
@@ -21,8 +24,6 @@
import java.util.concurrent.SynchronousQueue;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
-import javax.annotation.Nullable;
-import javax.security.auth.Subject;
/**
* Expose Prometheus metrics using a plain Java HttpServer.
@@ -66,11 +67,21 @@
this.server = httpServer;
this.executorService = executorService;
String metricsPath = getMetricsPath(metricsHandlerPath);
+ try {
+ server.removeContext("/");
+ } catch (IllegalArgumentException e) {
+ // context "/" not registered yet, ignore
+ }
registerHandler(
"/",
defaultHandler == null ? new DefaultHandler(metricsPath) : defaultHandler,
authenticator,
authenticatedSubjectAttributeName);
+ try {
+ server.removeContext(metricsPath);
+ } catch (IllegalArgumentException e) {
+ // context metricsPath not registered yet, ignore
+ }
registerHandler(
metricsPath,
new MetricsHandler(config, registry),
|
Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
2377a3f to
b55455f
Compare
|
To my knowledge contexts/handlers are registered per HttpServer instance, not globally. So how could At least I'm not able to come up with a test that already has a context registered on I think both approaches will effectively do the same, so I pushed your suggestion. |
|
thanks @Donnerbart - I'll merge once #1781 is merged |
The behavior of
HttpServer.createContext()was changed between Java 11 and 21. In the current JDKs the server doesn't allow to register multiple contexts with the same path. So if you configureyou get the following exception:
This PR fixes the issue by skipping the default handler registration in case the metrics are configured for the root path.
I also improved the unit test so we can properly assert
expectedStatusCodeandexpectedBody. With this improvement we can be sure that an endpoint actually returns the expected handler content, not just a matching status code.