Create one MetricsFilter per registry in Log4j2Metrics (#5818)

Before, we were needlessly making multiple instances of `MetricsFilter` that were functionally equivalent. This reduces the instances to one per registry to which Log4j2Metrics is bound. This will reduce allocations and memory usage slightly.
micrometer-metrics · Jan 23, 2025 · d645839 · d645839
1 parent 9111b44
commit d645839
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 14 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
@@ -31,9 +31,9 @@
 import org.apache.logging.log4j.core.filter.AbstractFilter;
 import org.apache.logging.log4j.core.filter.CompositeFilter;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
+import java.util.*;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 
 import static java.util.Collections.emptyList;
 
@@ -61,7 +61,7 @@ public class Log4j2Metrics implements MeterBinder, AutoCloseable {
 
     private final LoggerContext loggerContext;
 
-    private final List<MetricsFilter> metricsFilters = new ArrayList<>();
+    private final ConcurrentMap<MeterRegistry, MetricsFilter> metricsFilters = new ConcurrentHashMap<>();
 
     public Log4j2Metrics() {
         this(emptyList());
@@ -80,7 +80,7 @@ public Log4j2Metrics(Iterable<Tag> tags, LoggerContext loggerContext) {
     public void bindTo(MeterRegistry registry) {
         Configuration configuration = loggerContext.getConfiguration();
         LoggerConfig rootLoggerConfig = configuration.getRootLogger();
-        rootLoggerConfig.addFilter(createMetricsFilterAndStart(registry));
+        rootLoggerConfig.addFilter(getOrCreateMetricsFilterAndStart(registry));
 
         loggerContext.getConfiguration()
             .getLoggers()
@@ -102,25 +102,26 @@ public void bindTo(MeterRegistry registry) {
                 if (logFilter instanceof MetricsFilter) {
                     return;
                 }
-                loggerConfig.addFilter(createMetricsFilterAndStart(registry));
+                loggerConfig.addFilter(getOrCreateMetricsFilterAndStart(registry));
             });
 
         loggerContext.updateLoggers(configuration);
     }
 
-    private MetricsFilter createMetricsFilterAndStart(MeterRegistry registry) {
-        MetricsFilter metricsFilter = new MetricsFilter(registry, tags);
-        metricsFilter.start();
-        metricsFilters.add(metricsFilter);
-        return metricsFilter;
+    private MetricsFilter getOrCreateMetricsFilterAndStart(MeterRegistry registry) {
+        return metricsFilters.computeIfAbsent(registry, r -> {
+            MetricsFilter metricsFilter = new MetricsFilter(r, tags);
+            metricsFilter.start();
+            return metricsFilter;
+        });
     }
 
     @Override
     public void close() {
         if (!metricsFilters.isEmpty()) {
             Configuration configuration = loggerContext.getConfiguration();
             LoggerConfig rootLoggerConfig = configuration.getRootLogger();
-            metricsFilters.forEach(rootLoggerConfig::removeFilter);
+            metricsFilters.values().forEach(rootLoggerConfig::removeFilter);
 
             loggerContext.getConfiguration()
                 .getLoggers()
@@ -129,12 +130,13 @@ public void close() {
                 .filter(loggerConfig -> !loggerConfig.isAdditive())
                 .forEach(loggerConfig -> {
                     if (loggerConfig != rootLoggerConfig) {
-                        metricsFilters.forEach(loggerConfig::removeFilter);
+                        metricsFilters.values().forEach(loggerConfig::removeFilter);
                     }
                 });
 
             loggerContext.updateLoggers(configuration);
-            metricsFilters.forEach(MetricsFilter::stop);
+            metricsFilters.values().forEach(MetricsFilter::stop);
+            metricsFilters.clear();
         }
     }
 

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
@@ -23,6 +23,7 @@
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.core.Filter;
 import org.apache.logging.log4j.core.LoggerContext;
 import org.apache.logging.log4j.core.config.Configuration;
 import org.apache.logging.log4j.core.config.ConfigurationSource;
@@ -34,6 +35,7 @@
 
 import java.io.IOException;
 import java.time.Duration;
+import java.util.Iterator;
 
 import static java.util.Collections.emptyList;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -184,4 +186,47 @@ void asyncLogShouldNotBeDuplicated() throws IOException {
             .until(() -> registry.get("log4j2.events").tags("level", "info").counter().count() == 3);
     }
 
+    @Test
+    void metricsFilterIsReused() {
+        LoggerContext loggerContext = new LoggerContext("test");
+
+        LoggerConfig loggerConfig = new LoggerConfig("com.test", Level.INFO, false);
+        Configuration configuration = loggerContext.getConfiguration();
+        configuration.addLogger("com.test", loggerConfig);
+        loggerContext.setConfiguration(configuration);
+        loggerContext.updateLoggers();
+
+        Logger logger1 = loggerContext.getLogger("com.test.log1");
+        loggerContext.getLogger("com.test.log2");
+
+        new Log4j2Metrics(emptyList(), loggerContext).bindTo(registry);
+        Iterator<Filter> rootFilters = loggerContext.getRootLogger().getFilters();
+        Log4j2Metrics.MetricsFilter rootFilter = (Log4j2Metrics.MetricsFilter) rootFilters.next();
+        assertThat(rootFilters.hasNext()).isFalse();
+
+        Log4j2Metrics.MetricsFilter logger1Filter = (Log4j2Metrics.MetricsFilter) loggerContext.getConfiguration()
+            .getLoggerConfig(logger1.getName())
+            .getFilter();
+        assertThat(logger1Filter).isEqualTo(rootFilter);
+    }
+
+    @Test
+    void multipleRegistriesCanBeBound() {
+        MeterRegistry registry2 = new SimpleMeterRegistry();
+
+        Log4j2Metrics log4j2Metrics = new Log4j2Metrics(emptyList());
+        log4j2Metrics.bindTo(registry);
+
+        Logger logger = LogManager.getLogger(Log4j2MetricsTest.class);
+        Configurator.setLevel(logger, Level.INFO);
+        logger.info("Hello, world!");
+        assertThat(registry.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(1);
+
+        log4j2Metrics.bindTo(registry2);
+        logger.info("Hello, world!");
+        assertThat(registry.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(2);
+        assertThat(registry2.get("log4j2.events").tags("level", "info").counter().count()).isEqualTo(1);
+
+    }
+
 }