Cleanup MetricsFilter from old configuration

Signed-off-by: Patrik Ivarsson <patrik.ivarsson@avanza.se>
micrometer-metrics · Jan 20, 2025 · 4361d51 · 4361d51
1 parent f2005b6
commit 4361d51
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 25 deletions.
diff --git a/...ometer-core/src/main/java/io/micrometer/core/instrument/binder/logging/Log4j2Metrics.java b/...ometer-core/src/main/java/io/micrometer/core/instrument/binder/logging/Log4j2Metrics.java
@@ -83,24 +83,28 @@ public Log4j2Metrics(Iterable<Tag> tags, LoggerContext loggerContext) {
 
     @Override
     public void bindTo(MeterRegistry registry) {
+        Configuration configuration = loggerContext.getConfiguration();
+        addMetricsFilterToConfiguration(configuration, registry);
+        loggerContext.updateLoggers(configuration);
+
         PropertyChangeListener changeListener = listener -> {
-            if (listener.getNewValue() instanceof Configuration) {
-                Configuration configuration = (Configuration) listener.getNewValue();
-                addMetricsFilterToConfiguration(configuration, registry);
+            if (listener.getNewValue() instanceof Configuration && listener.getOldValue() != listener.getNewValue()) {
+                Configuration newConfiguration = (Configuration) listener.getNewValue();
+                addMetricsFilterToConfiguration(newConfiguration, registry);
+                if (listener.getOldValue() instanceof Configuration) {
+                    removeMetricsFilter((Configuration) listener.getOldValue());
+                }
             }
         };
 
         loggerContext.addPropertyChangeListener(changeListener);
         changeListeners.add(changeListener);
-
-        // This will trigger the changeListener
-        loggerContext.updateLoggers();
     }
 
     private void addMetricsFilterToConfiguration(Configuration configuration, MeterRegistry registry) {
         LoggerConfig rootLoggerConfig = configuration.getRootLogger();
 
-        if (!isMetricsFilter(rootLoggerConfig.getFilter())) {
+        if (!containsMetricsFilter(rootLoggerConfig.getFilter())) {
             rootLoggerConfig.addFilter(metricsFilters.computeIfAbsent(rootLoggerConfig.getName(),
                     name -> createMetricsFilterAndStart(registry)));
         }
@@ -113,16 +117,16 @@ private void addMetricsFilterToConfiguration(Configuration configuration, MeterR
                 if (loggerConfig == rootLoggerConfig) {
                     return;
                 }
-                if (!isMetricsFilter(loggerConfig.getFilter())) {
+                if (!containsMetricsFilter(loggerConfig.getFilter())) {
                     loggerConfig.addFilter(metricsFilters.computeIfAbsent(loggerConfig.getName(),
                             name -> createMetricsFilterAndStart(registry)));
                 }
             });
     }
 
-    private boolean isMetricsFilter(Filter logFilter) {
+    private boolean containsMetricsFilter(Filter logFilter) {
         return logFilter instanceof MetricsFilter || (logFilter instanceof CompositeFilter
-                && Arrays.stream(((CompositeFilter) logFilter).getFiltersArray()).anyMatch(this::isMetricsFilter));
+                && Arrays.stream(((CompositeFilter) logFilter).getFiltersArray()).anyMatch(this::containsMetricsFilter));
     }
 
     private MetricsFilter createMetricsFilterAndStart(MeterRegistry registry) {
@@ -134,27 +138,32 @@ private MetricsFilter createMetricsFilterAndStart(MeterRegistry registry) {
     @Override
     public void close() {
         if (!metricsFilters.isEmpty()) {
-            Configuration configuration = loggerContext.getConfiguration();
-            LoggerConfig rootLoggerConfig = configuration.getRootLogger();
-            rootLoggerConfig.removeFilter(metricsFilters.get(rootLoggerConfig.getName()));
-
-            loggerContext.getConfiguration()
-                .getLoggers()
-                .values()
-                .stream()
-                .filter(loggerConfig -> !loggerConfig.isAdditive())
-                .forEach(loggerConfig -> {
-                    if (loggerConfig != rootLoggerConfig) {
-                        loggerConfig.removeFilter(metricsFilters.get(loggerConfig.getName()));
-                    }
-                });
-
             changeListeners.forEach(loggerContext::removePropertyChangeListener);
+
+            Configuration configuration = loggerContext.getConfiguration();
+            removeMetricsFilter(configuration);
             loggerContext.updateLoggers(configuration);
+
             metricsFilters.values().forEach(MetricsFilter::stop);
         }
     }
 
+    private void removeMetricsFilter(Configuration configuration) {
+        LoggerConfig rootLoggerConfig = configuration.getRootLogger();
+        rootLoggerConfig.removeFilter(metricsFilters.get(rootLoggerConfig.getName()));
+
+        loggerContext.getConfiguration()
+            .getLoggers()
+            .values()
+            .stream()
+            .filter(loggerConfig -> !loggerConfig.isAdditive())
+            .forEach(loggerConfig -> {
+                if (loggerConfig != rootLoggerConfig) {
+                    loggerConfig.removeFilter(metricsFilters.get(loggerConfig.getName()));
+                }
+            });
+    }
+
     @NonNullApi
     @NonNullFields
     static class MetricsFilter extends AbstractFilter {

diff --git a/...er-core/src/test/java/io/micrometer/core/instrument/binder/logging/Log4j2MetricsTest.java b/...er-core/src/test/java/io/micrometer/core/instrument/binder/logging/Log4j2MetricsTest.java
@@ -28,6 +28,7 @@
 import org.apache.logging.log4j.core.config.ConfigurationSource;
 import org.apache.logging.log4j.core.config.Configurator;
 import org.apache.logging.log4j.core.config.LoggerConfig;
+import org.awaitility.Awaitility;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -189,18 +190,28 @@ void asyncLogShouldNotBeDuplicated() throws IOException {
     void rebindsMetricsWhenConfigurationIsReloaded() {
         LoggerContext context = new LoggerContext("test");
         Logger logger = context.getLogger("com.test");
+        Configuration oldConfiguration = context.getConfiguration();
 
         try (Log4j2Metrics metrics = new Log4j2Metrics(emptyList(), context)) {
             metrics.bindTo(registry);
 
             logger.error("first");
 
+            // Should have added filter to configuration
+            assertThat(oldConfiguration.getRootLogger().getFilter()).isInstanceOf(Log4j2Metrics.MetricsFilter.class);
+
             // This will reload the configuration to default
             context.reconfigure();
 
+            Configuration newConfiguration = context.getConfiguration();
+
             // For this event to be counted, the metrics must be rebound
             logger.error("second");
 
+            // Should have removed filter from old configuration, adding it to new configuration
+            assertThat(oldConfiguration.getRootLogger().getFilter()).isNull();
+            assertThat(newConfiguration.getRootLogger().getFilter()).isInstanceOf(Log4j2Metrics.MetricsFilter.class);
+
             assertThat(registry.get("log4j2.events").tags("level", "error").counter().count()).isEqualTo(2);
         }
     }