Keep track of HTTPServer instances #40
Conversation
Signed-off-by: Mickael Maison <mickael.maison@gmail.com>
| @@ -57,7 +56,7 @@ public void configure(Map<String, ?> map) { | |||
| config = new PrometheusMetricsReporterConfig(map, registry); | |||
| collector = new KafkaMetricsCollector(); | |||
| // Add JVM metrics | |||
| JvmMetrics.builder().register(registry); | |||
| JvmMetrics.builder().register(); | |||
There was a problem hiding this comment.
The method without an argument is idempotent and registers the JVM metrics onto the default metrics registry, which is what we use at runtime. This allow multiple components to run this code without causing exceptions as the metrics are only added once: http://prometheus.github.io/client_java/api/io/prometheus/metrics/instrumentation/jvm/JvmMetrics.Builder.html#register()
It would be nice to pass a registry and still have the idempotent behavior if it's the default registry. I may open a PR on https://github.com/prometheus/client_java to do that. In the meantime, we have to use to method without arguments.
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| /** | ||
| * Static class to keep track of all the HTTP servers started by all the Kafka components in a JVM. |
There was a problem hiding this comment.
Right, "static" is not the right term. I meant it as a utility. But I guess we can just say "Class to keep track ..."
| * @return A ServerCounter instance | ||
| * @throws IOException if the HTTP server does not exist and cannot be started | ||
| */ | ||
| public synchronized static ServerCounter get(Listener listener, PrometheusRegistry registry) throws IOException { |
There was a problem hiding this comment.
Should it be named something like getOrCreate if it is not a simple getter?
There was a problem hiding this comment.
Yes I've renamed it to getOrCreate()
Signed-off-by: Mickael Maison <mickael.maison@gmail.com>
| LOG.info("HTTP server already started"); | ||
| return Optional.empty(); | ||
| HttpServers.ServerCounter server = HttpServers.getOrCreate(listener, registry); | ||
| LOG.info("HTTP server started on listener http://{}:{}", listener.host, server.port()); |
There was a problem hiding this comment.
The log line is misleading as the server isn't necessarily being started here.
| .port(listener.port) | ||
| .registry(registry) | ||
| .buildAndStart(); | ||
| this.listener = listener; |
There was a problem hiding this comment.
I'd add a log statement for starting the server.
| private synchronized boolean close() { | ||
| int remaining = count.decrementAndGet(); | ||
| if (remaining == 0) { | ||
| server.close(); |
There was a problem hiding this comment.
Done. I checked that the port is returned (and valid) after close() is called.
| * @return A ServerCounter instance | ||
| * @throws IOException if the HTTP server does not exist and cannot be started | ||
| */ | ||
| public synchronized static ServerCounter getOrCreate(Listener listener, PrometheusRegistry registry) throws IOException { |
There was a problem hiding this comment.
The API is a bit weak. It has you believe that you could attach different registries, but it actually ignore it.
I see that both Yammer and Kafka paths pass use the same registry (the default one) so given our use of the class, there's no issue.
I don't really have an alternative suggestion here.
There was a problem hiding this comment.
Yeah the registry used to be hard coded to the default registry as it's what all components have to use. But because there isn't a mechanism to clear a registry, we allowed passing it in for tests.
Signed-off-by: Mickael Maison <mickael.maison@gmail.com>
An attempt at addressing #34 by keeping track of all HTTPServer instances started so they can be properly closed whenever possible. You can have multiple HTTPServer instances when several Kafka components are started in the same JVM, for example in Kafka Connect or Kafka Streams.
If multiple components attempt to create servers on the same listener, the same server will be reused. When all using components are shutdown, the server can also be closed.