Repository: kroxylicious/kroxylicious (java)

Issue:
"Attempted to register a meter with id" errors during integration tests
_Please use this to only for bug reports_. For questions or when you need help, you can use the [GitHub Discussions](https://github.com/kroxylicious/kroxylicious/discussions) or use the [community Slack](https://kroxylicious.slack.com) chat.

**Describe the bug**

Running the integration tests on main, I notice ERRORs like this being spewed out.  The messages don't seem to cause harm and the tests still pass. The noise is unpleasant. 

```
2023-07-18 13:04:10 ERROR io.kroxylicious.proxy.internal.MeterRegistries:62 - Attempted to register a meter with id MeterId{name='jvm.gc.pause', tags=[tag(a=b),tag(action=end of minor GC),tag(cause=G1 Humongous Allocation),tag(gc=G1 Young Generation)]} which is already registered but with a different set of tag names
java.lang.IllegalArgumentException: tags for id MeterId{name='jvm.gc.pause', tags=[tag(a=b),tag(action=end of minor GC),tag(cause=G1 Humongous Allocation),tag(gc=G1 Young Generation)]} differ from existing meters registered
	at io.kroxylicious.proxy.internal.MeterRegistries$1.accept(MeterRegistries.java:63)
	at io.micrometer.core.instrument.MeterRegistry.accept(MeterRegistry.java:646)
	at io.micrometer.core.instrument.MeterRegistry.getOrCreateMeter(MeterRegistry.java:612)
	at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:577)
	at io.micrometer.core.instrument.MeterRegistry.timer(MeterRegistry.java:323)
	at io.micrometer.core.instrument.Timer$Builder.register(Timer.java:444)
	at io.micrometer.core.instrument.binder.jvm.JvmGcMetrics$GcMetricsNotificationListener.handleNotification(JvmGcMetrics.java:211)
	at java.management/sun.management.NotificationEmitterSupport.sendNotification(NotificationEmitterSupport.java:155)
	at jdk.management/com.sun.management.internal.GarbageCollectorExtImpl.createGCNotification(GarbageCollectorExtImpl.java:115)
```
A clear and concise description of what the bug is.

**To Reproduce**
Steps to reproduce the behavior:
1. `mvn  verify -pl integrationtests`

**Expected behavior**
No MeterRegistry errors logged to console.

**YAML files and logs**

Attach or copy paste your kroxylicious configuration YAML (with sensitive details
like broker address redacted).
Attach or copy and paste relevant logs.

**Additional context**
Add any other context about the problem here.


Pull request:
Fix #463 - deregister micrometer hooks, meters and the registry on shutdown

### Type of change

_Select the type of your PR_

- Bugfix

### Description

Trying to prevent the stack trace originating from the JMX notifications during integration test runs.
The issue is not causing test failures, but the noise is polluting the logs.

### Additional Context

Build hygiene.

### Checklist

_Please go through this checklist and make sure all applicable tasks have been done_

- [x] Write tests
- [x] Make sure all tests pass
- [ ] Review performance test results. Ensure that any degradations to performance numbers are understood and justified.
- [x] Make sure all Sonar-Lint warnings are addressed or are justifiably ignored.
- [ ] Update documentation
- [x] Reference relevant issue(s) and close them after merging
- [x] For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 58fb4029..fb92de2b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ Please enumerate **all user-facing** changes using format `<githib issue/pr numb
 
 ## 0.3.0
 
+* [#463](https://github.com/kroxylicious/kroxylicious/issues/463): deregister micrometer hooks, meters and the registry on shutdown
 * [#443](https://github.com/kroxylicious/kroxylicious/pull/443): Obtain upstream ApiVersions when proxy is not SASL offloading
 * [#412](https://github.com/kroxylicious/kroxylicious/issues/412): Remove $(portNumber) pattern from brokerAddressPattern for SniRouting and PortPerBroker schemes
 * [#414](https://github.com/kroxylicious/kroxylicious/pull/414): Add kubernetes sample illustrating SNI based routing, downstream/upstream TLS and the use of certificates from cert-manager.
diff --git a/kroxylicious/src/main/java/io/kroxylicious/proxy/KafkaProxy.java b/kroxylicious/src/main/java/io/kroxylicious/proxy/KafkaProxy.java
index 6bd126e6..92b27235 100644
--- a/kroxylicious/src/main/java/io/kroxylicious/proxy/KafkaProxy.java
+++ b/kroxylicious/src/main/java/io/kroxylicious/proxy/KafkaProxy.java
@@ -56,6 +56,7 @@ public final class KafkaProxy implements AutoCloseable {
 
     private final NetworkBindingOperationProcessor bindingOperationProcessor = new DefaultNetworkBindingOperationProcessor();
     private final EndpointRegistry endpointRegistry = new EndpointRegistry(bindingOperationProcessor);
+    private MeterRegistries meterRegistries;
 
     private record EventGroupConfig(String name, EventLoopGroup bossGroup, EventLoopGroup workerGroup, Class<? extends ServerChannel> clazz) {
 
@@ -99,7 +100,7 @@ public final class KafkaProxy implements AutoCloseable {
         portConflictDefector.validate(virtualClusterMap, adminHttpHostPort);
 
         var availableCores = Runtime.getRuntime().availableProcessors();
-        var meterRegistries = new MeterRegistries(micrometerConfig);
+        meterRegistries = new MeterRegistries(micrometerConfig);
 
         this.adminEventGroup = buildNettyEventGroups("admin", availableCores, config.isUseIoUring());
         this.serverEventGroup = buildNettyEventGroups("server", availableCores, config.isUseIoUring());
@@ -225,11 +226,15 @@ public final class KafkaProxy implements AutoCloseable {
                 }
                 return null;
             }).toCompletableFuture().join();
+            if (meterRegistries != null) {
+                meterRegistries.close();
+            }
         }
         finally {
             adminEventGroup = null;
             serverEventGroup = null;
             metricsChannel = null;
+            meterRegistries = null;
             shutdown.complete(null);
             LOGGER.info("Shut down completed.");
 
diff --git a/kroxylicious/src/main/java/io/kroxylicious/proxy/internal/MeterRegistries.java b/kroxylicious/src/main/java/io/kroxylicious/proxy/internal/MeterRegistries.java
index b6f29f04..c922bb72 100644
--- a/kroxylicious/src/main/java/io/kroxylicious/proxy/internal/MeterRegistries.java
+++ b/kroxylicious/src/main/java/io/kroxylicious/proxy/internal/MeterRegistries.java
@@ -22,27 +22,28 @@ import io.micrometer.prometheus.PrometheusConfig;
 import io.micrometer.prometheus.PrometheusMeterRegistry;
 
 import io.kroxylicious.proxy.config.MicrometerDefinition;
+import io.kroxylicious.proxy.micrometer.MicrometerConfigurationHook;
 import io.kroxylicious.proxy.micrometer.MicrometerConfigurationHookContributorManager;
 
-public class MeterRegistries {
+public class MeterRegistries implements AutoCloseable {
     private final PrometheusMeterRegistry prometheusMeterRegistry;
 
     private static final Logger logger = LoggerFactory.getLogger(MeterRegistries.class);
+    private final List<MicrometerConfigurationHook> hooks;
 
     public MeterRegistries(List<MicrometerDefinition> micrometerConfig) {
-        configureMicrometer(micrometerConfig);
+        this.hooks = registerHooks(micrometerConfig);
         this.prometheusMeterRegistry = new PrometheusMeterRegistry(PrometheusConfig.DEFAULT);
         Metrics.addRegistry(prometheusMeterRegistry);
     }
 
-    private void configureMicrometer(List<MicrometerDefinition> micrometerConfig) {
+    private List<MicrometerConfigurationHook> registerHooks(List<MicrometerDefinition> micrometerConfig) {
         CompositeMeterRegistry globalRegistry = Metrics.globalRegistry;
         preventDifferentTagNameRegistration(globalRegistry);
         MicrometerConfigurationHookContributorManager manager = MicrometerConfigurationHookContributorManager.getInstance();
-        micrometerConfig
-                .stream()
-                .map(f -> manager.getHook(f.type(), f.config()))
-                .forEach(micrometerConfigurationHook -> micrometerConfigurationHook.configure(globalRegistry));
+        var hooks = micrometerConfig.stream().map(f -> manager.getHook(f.type(), f.config())).toList();
+        hooks.forEach(micrometerConfigurationHook -> micrometerConfigurationHook.configure(globalRegistry));
+        return hooks;
     }
 
     /**
@@ -80,4 +81,13 @@ public class MeterRegistries {
     public Optional<PrometheusMeterRegistry> maybePrometheusMeterRegistry() {
         return Optional.ofNullable(prometheusMeterRegistry);
     }
+
+    @Override
+    public void close() {
+        hooks.forEach(MicrometerConfigurationHook::close);
+        // remove the meters we contributed to the global registry.
+        var copy = List.copyOf(prometheusMeterRegistry.getMeters());
+        copy.forEach(Metrics.globalRegistry::remove);
+        Metrics.removeRegistry(prometheusMeterRegistry);
+    }
 }
diff --git a/kroxylicious/src/main/java/io/kroxylicious/proxy/micrometer/MicrometerConfigurationHook.java b/kroxylicious/src/main/java/io/kroxylicious/proxy/micrometer/MicrometerConfigurationHook.java
index 5c74f386..922786e6 100644
--- a/kroxylicious/src/main/java/io/kroxylicious/proxy/micrometer/MicrometerConfigurationHook.java
+++ b/kroxylicious/src/main/java/io/kroxylicious/proxy/micrometer/MicrometerConfigurationHook.java
@@ -7,7 +7,19 @@ package io.kroxylicious.proxy.micrometer;
 
 import io.micrometer.core.instrument.MeterRegistry;
 
-public interface MicrometerConfigurationHook {
+public interface MicrometerConfigurationHook extends AutoCloseable {
+    /**
+     * Configures this hook into the given registry.
+     *
+     * @param targetRegistry meter registry.
+     */
     void configure(MeterRegistry targetRegistry);
 
+    /**
+     * Releases resources that may have been registered by this hook.  Implementations
+     * of this method must not throw unchecked exceptions.
+     */
+    default void close() {
+    }
+
 }
diff --git a/kroxylicious/src/main/java/io/kroxylicious/proxy/micrometer/StandardBindersHook.java b/kroxylicious/src/main/java/io/kroxylicious/proxy/micrometer/StandardBindersHook.java
index 9ebe7eb3..03e9eb70 100644
--- a/kroxylicious/src/main/java/io/kroxylicious/proxy/micrometer/StandardBindersHook.java
+++ b/kroxylicious/src/main/java/io/kroxylicious/proxy/micrometer/StandardBindersHook.java
@@ -6,6 +6,7 @@
 package io.kroxylicious.proxy.micrometer;
 
 import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -30,6 +31,7 @@ import io.kroxylicious.proxy.config.BaseConfig;
 public class StandardBindersHook implements MicrometerConfigurationHook {
     private static final Logger log = LoggerFactory.getLogger(StandardBindersHook.class);
     private final StandardBindersHookConfig config;
+    private final List<AutoCloseable> closeableBinders = new CopyOnWriteArrayList<>();
 
     public static class StandardBindersHookConfig extends BaseConfig {
         private final List<String> binderNames;
@@ -52,35 +54,39 @@ public class StandardBindersHook implements MicrometerConfigurationHook {
         for (String binderName : this.config.binderNames) {
             MeterBinder binder = getBinder(binderName);
             binder.bindTo(targetRegistry);
-            log.info("bound " + binderName + " to micrometer registry");
+            if (binder instanceof AutoCloseable closeable) {
+                this.closeableBinders.add(closeable);
+            }
+            log.info("bound {} to micrometer registry", binderName);
         }
 
     }
 
-    private MeterBinder getBinder(String binderName) {
-        switch (binderName) {
-            case "UptimeMetrics":
-                return new UptimeMetrics();
-            case "ProcessorMetrics":
-                return new ProcessorMetrics();
-            case "FileDescriptorMetrics":
-                return new FileDescriptorMetrics();
-            case "ClassLoaderMetrics":
-                return new ClassLoaderMetrics();
-            case "JvmCompilationMetrics":
-                return new JvmCompilationMetrics();
-            case "JvmGcMetrics":
-                return new JvmGcMetrics();
-            case "JvmHeapPressureMetrics":
-                return new JvmHeapPressureMetrics();
-            case "JvmInfoMetrics":
-                return new JvmInfoMetrics();
-            case "JvmMemoryMetrics":
-                return new JvmMemoryMetrics();
-            case "JvmThreadMetrics":
-                return new JvmThreadMetrics();
-            default:
-                throw new IllegalArgumentException("no binder available for " + binderName);
-        }
+    @Override
+    public void close() {
+        closeableBinders.forEach(closeable -> {
+            try {
+                closeable.close();
+            }
+            catch (Exception e) {
+                log.warn("Ignoring exception whilst closing standard binder {}", closeable.getClass(), e);
+            }
+        });
+    }
+
+    /* testing */ protected MeterBinder getBinder(String binderName) {
+        return switch (binderName) {
+            case "UptimeMetrics" -> new UptimeMetrics();
+            case "ProcessorMetrics" -> new ProcessorMetrics();
+            case "FileDescriptorMetrics" -> new FileDescriptorMetrics();
+            case "ClassLoaderMetrics" -> new ClassLoaderMetrics();
+            case "JvmCompilationMetrics" -> new JvmCompilationMetrics();
+            case "JvmGcMetrics" -> new JvmGcMetrics();
+            case "JvmHeapPressureMetrics" -> new JvmHeapPressureMetrics();
+            case "JvmInfoMetrics" -> new JvmInfoMetrics();
+            case "JvmMemoryMetrics" -> new JvmMemoryMetrics();
+            case "JvmThreadMetrics" -> new JvmThreadMetrics();
+            default -> throw new IllegalArgumentException("no binder available for " + binderName);
+        };
     }
 }
diff --git a/scripts/common.sh b/scripts/common.sh
index c40858b0..e140e62c 100644
--- a/scripts/common.sh
+++ b/scripts/common.sh
@@ -21,11 +21,8 @@ resolveCommand () {
   fi
 }
 
-if [[ -z $CONTAINER_ENGINE ]]; then
-  echo "Setting CONTAINER_ENGINE to default: docker"
-  CONTAINER_ENGINE=$(resolveCommand docker)
-  export CONTAINER_ENGINE
-fi
+CONTAINER_ENGINE=$(resolveCommand "${CONTAINER_ENGINE:-docker}")
+export CONTAINER_ENGINE
 
 KUBECTL=$(resolveCommand kubectl)
 export KUBECTL
diff --git a/scripts/run-with-strimzi.sh b/scripts/run-with-strimzi.sh
index b12bf16b..da19291e 100755
--- a/scripts/run-with-strimzi.sh
+++ b/scripts/run-with-strimzi.sh
@@ -5,8 +5,8 @@
 # Licensed under the Apache Software License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
 #
 
-set -eo pipefail
-DEFAULT_QUAY_ORG='kroxylicious-developer'
+set -euo pipefail
+DEFAULT_QUAY_ORG='kroxylicious'
 
 SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
 . "${SCRIPT_DIR}/common.sh"
@@ -28,7 +28,7 @@ function cleanTmpDir {
 }
 trap cleanTmpDir EXIT
 
-if [[ -z "${QUAY_ORG}" ]]; then
+if [[ -z "${QUAY_ORG:-}" ]]; then
   echo "Please set QUAY_ORG, exiting"
   exit 1
 fi


Test patch:
diff --git a/integrationtests/src/test/java/io/kroxylicious/proxy/MetricsIT.java b/integrationtests/src/test/java/io/kroxylicious/proxy/MetricsIT.java
index 97fc4ca1..71092738 100644
--- a/integrationtests/src/test/java/io/kroxylicious/proxy/MetricsIT.java
+++ b/integrationtests/src/test/java/io/kroxylicious/proxy/MetricsIT.java
@@ -12,7 +12,6 @@ import java.util.Map;
 import java.util.Random;
 import java.util.regex.Pattern;
 
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 
@@ -32,11 +31,6 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
 @ExtendWith(KafkaClusterExtension.class)
 public class MetricsIT {
 
-    @BeforeEach
-    public void beforeEach() {
-        Metrics.globalRegistry.clear();
-    }
-
     @Test
     public void shouldOfferPrometheusMetricsScrapeEndpoint(KafkaCluster cluster) {
         var config = proxy(cluster)
diff --git a/integrationtests/src/test/resources/log4j2-test.properties b/integrationtests/src/test/resources/log4j2-test.properties
index 2abaaa2b..d5739202 100644
--- a/integrationtests/src/test/resources/log4j2-test.properties
+++ b/integrationtests/src/test/resources/log4j2-test.properties
@@ -43,13 +43,13 @@ rootLogger.additivity = false
 #logger.netty.additivity = false
 #logger.netty.appenderRef.console.ref = STDOUT
 #
-logger.broker.name = kafka
-logger.broker.level = DEBUG
-logger.broker.additivity = false
-logger.broker.appenderRef.console.ref = STDOUT
-
-logger.net.name = org.apache.kafka.clients.NetworkClient
-logger.net.level = debug
+#logger.broker.name = kafka
+#logger.broker.level = DEBUG
+#logger.broker.additivity = false
+#logger.broker.appenderRef.console.ref = STDOUT
+#
+#logger.net.name = org.apache.kafka.clients.NetworkClient
+#logger.net.level = debug
 
 #
 #logger.scl.name = state.change.logger
diff --git a/kroxylicious/src/test/java/io/kroxylicious/proxy/micrometer/StandardBindersHookTest.java b/kroxylicious/src/test/java/io/kroxylicious/proxy/micrometer/StandardBindersHookTest.java
index 0927a5ae..441f4f91 100644
--- a/kroxylicious/src/test/java/io/kroxylicious/proxy/micrometer/StandardBindersHookTest.java
+++ b/kroxylicious/src/test/java/io/kroxylicious/proxy/micrometer/StandardBindersHookTest.java
@@ -8,16 +8,26 @@ package io.kroxylicious.proxy.micrometer;
 import java.util.List;
 
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 
 import io.micrometer.core.instrument.Meter;
 import io.micrometer.core.instrument.MeterRegistry;
+import io.micrometer.core.instrument.binder.MeterBinder;
 import io.micrometer.core.instrument.composite.CompositeMeterRegistry;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.verify;
 
+@ExtendWith(MockitoExtension.class)
 class StandardBindersHookTest {
 
+    private static final String AUTO_CLOSEABLE_BINDER = "AutoCloseableBinder";
+    @Mock(extraInterfaces = AutoCloseable.class)
+    MeterBinder closeableBinder;
+
     @Test
     public void testNullHookConfigThrows() {
         assertThatThrownBy(() -> new StandardBindersHook(null)).isInstanceOf(IllegalArgumentException.class);
@@ -25,29 +35,51 @@ class StandardBindersHookTest {
 
     @Test
     public void testNullBinderNames() {
-        StandardBindersHook hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(null));
-        MeterRegistry registry = whenRegistryConfiguredWith(hook);
-        thenNoMetersRegistered(registry);
+        try (StandardBindersHook hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(null))) {
+            MeterRegistry registry = whenRegistryConfiguredWith(hook);
+            thenNoMetersRegistered(registry);
+        }
     }
 
     @Test
     public void testEmptyBinderNames() {
-        StandardBindersHook hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(List.of()));
-        MeterRegistry registry = whenRegistryConfiguredWith(hook);
-        thenNoMetersRegistered(registry);
+        try (StandardBindersHook hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(List.of()))) {
+            MeterRegistry registry = whenRegistryConfiguredWith(hook);
+            thenNoMetersRegistered(registry);
+        }
     }
 
     @Test
     public void testKnownBinder() {
-        StandardBindersHook hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(List.of("UptimeMetrics")));
-        MeterRegistry registry = whenRegistryConfiguredWith(hook);
-        thenUptimeMeterRegistered(registry);
+        try (StandardBindersHook hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(List.of("UptimeMetrics")))) {
+            MeterRegistry registry = whenRegistryConfiguredWith(hook);
+            thenUptimeMeterRegistered(registry);
+        }
     }
 
     @Test
     public void testUnknownBinder() {
-        StandardBindersHook hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(List.of("SadClown")));
-        assertThatThrownBy(() -> whenRegistryConfiguredWith(hook)).isInstanceOf(IllegalArgumentException.class);
+        try (StandardBindersHook hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(List.of("SadClown")))) {
+            assertThatThrownBy(() -> whenRegistryConfiguredWith(hook)).isInstanceOf(IllegalArgumentException.class);
+        }
+    }
+
+    @Test
+    public void testAutoCloseableBindingClosed() throws Exception {
+        var hook = new StandardBindersHook(new StandardBindersHook.StandardBindersHookConfig(List.of(AUTO_CLOSEABLE_BINDER))) {
+            @Override
+            protected MeterBinder getBinder(String binderName) {
+                if (binderName.equals(AUTO_CLOSEABLE_BINDER)) {
+                    return closeableBinder;
+                }
+                throw new IllegalArgumentException();
+            }
+        };
+        var registry = whenRegistryConfiguredWith(hook);
+        verify(closeableBinder).bindTo(registry);
+
+        hook.close();
+        verify(((AutoCloseable) closeableBinder)).close();
     }
 
     private static void thenUptimeMeterRegistered(MeterRegistry registry) {