From 466e1324ce166e6ac2b49d37331b95c23a28ba38 Mon Sep 17 00:00:00 2001 From: Patrik Ivarsson Date: Thu, 23 Jan 2025 21:26:04 +0100 Subject: [PATCH] Rebind log4j2 metrics if configuration is changed * Use `PropertyChangeListener` to rebind the `MetricsFilter` to log4j2 if configuration is reloaded * Keep track of the listener to deregister it when we close the binder Signed-off-by: Patrik Ivarsson --- .../binder/logging/Log4j2Metrics.java | 62 +++++++++++++------ .../binder/logging/Log4j2MetricsTest.java | 52 ++++++++++++++++ 2 files changed, 95 insertions(+), 19 deletions(-) diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/logging/Log4j2Metrics.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/logging/Log4j2Metrics.java index 98e9b772f5..1a6ea316e1 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/logging/Log4j2Metrics.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/logging/Log4j2Metrics.java @@ -31,9 +31,12 @@ import org.apache.logging.log4j.core.filter.AbstractFilter; import org.apache.logging.log4j.core.filter.CompositeFilter; -import java.util.*; +import java.beans.PropertyChangeListener; +import java.util.Arrays; +import java.util.List; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.CopyOnWriteArrayList; import static java.util.Collections.emptyList; @@ -62,6 +65,7 @@ public class Log4j2Metrics implements MeterBinder, AutoCloseable { private final LoggerContext loggerContext; private final ConcurrentMap metricsFilters = new ConcurrentHashMap<>(); + private final List changeListeners = new CopyOnWriteArrayList<>(); public Log4j2Metrics() { this(emptyList()); @@ -79,10 +83,27 @@ public Log4j2Metrics(Iterable tags, LoggerContext loggerContext) { @Override public void bindTo(MeterRegistry registry) { Configuration configuration = loggerContext.getConfiguration(); + registerMetricsFilter(configuration, registry); + loggerContext.updateLoggers(configuration); + + PropertyChangeListener changeListener = listener -> { + if (listener.getNewValue() instanceof Configuration && listener.getOldValue() != listener.getNewValue()) { + registerMetricsFilter((Configuration) listener.getNewValue(), registry); + if (listener.getOldValue() instanceof Configuration) { + removeMetricsFilter((Configuration) listener.getOldValue()); + } + } + }; + + changeListeners.add(changeListener); + loggerContext.addPropertyChangeListener(changeListener); + } + + private void registerMetricsFilter(Configuration configuration, MeterRegistry registry) { LoggerConfig rootLoggerConfig = configuration.getRootLogger(); rootLoggerConfig.addFilter(getOrCreateMetricsFilterAndStart(registry)); - loggerContext.getConfiguration() + configuration .getLoggers() .values() .stream() @@ -104,8 +125,6 @@ public void bindTo(MeterRegistry registry) { } loggerConfig.addFilter(getOrCreateMetricsFilterAndStart(registry)); }); - - loggerContext.updateLoggers(configuration); } private MetricsFilter getOrCreateMetricsFilterAndStart(MeterRegistry registry) { @@ -118,28 +137,33 @@ private MetricsFilter getOrCreateMetricsFilterAndStart(MeterRegistry registry) { @Override public void close() { + changeListeners.forEach(loggerContext::removePropertyChangeListener); + if (!metricsFilters.isEmpty()) { Configuration configuration = loggerContext.getConfiguration(); - LoggerConfig rootLoggerConfig = configuration.getRootLogger(); - metricsFilters.values().forEach(rootLoggerConfig::removeFilter); - - loggerContext.getConfiguration() - .getLoggers() - .values() - .stream() - .filter(loggerConfig -> !loggerConfig.isAdditive()) - .forEach(loggerConfig -> { - if (loggerConfig != rootLoggerConfig) { - metricsFilters.values().forEach(loggerConfig::removeFilter); - } - }); - + removeMetricsFilter(configuration); loggerContext.updateLoggers(configuration); + metricsFilters.values().forEach(MetricsFilter::stop); - metricsFilters.clear(); } } + private void removeMetricsFilter(Configuration configuration) { + LoggerConfig rootLoggerConfig = configuration.getRootLogger(); + metricsFilters.values().forEach(rootLoggerConfig::removeFilter); + + configuration + .getLoggers() + .values() + .stream() + .filter(loggerConfig -> !loggerConfig.isAdditive()) + .forEach(loggerConfig -> { + if (loggerConfig != rootLoggerConfig) { + metricsFilters.values().forEach(rootLoggerConfig::removeFilter); + } + }); + } + @NonNullApi @NonNullFields static class MetricsFilter extends AbstractFilter { diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/logging/Log4j2MetricsTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/logging/Log4j2MetricsTest.java index a2dcdb18da..824f3bad28 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/logging/Log4j2MetricsTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/logging/Log4j2MetricsTest.java @@ -229,4 +229,56 @@ void multipleRegistriesCanBeBound() { } + @Issue("#5756") + @Test + 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"); + assertThat(registry.get("log4j2.events").tags("level", "error").counter().count()).isEqualTo(1); + + // Should have added filter to configuration + Filter oldFilter = oldConfiguration.getRootLogger().getFilter(); + assertThat(oldFilter).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"); + assertThat(registry.get("log4j2.events").tags("level", "error").counter().count()).isEqualTo(2); + + // Should have removed filter from old configuration, adding it to the new + assertThat(oldConfiguration.getRootLogger().getFilter()).isNull(); + Filter newFilter = newConfiguration.getRootLogger().getFilter(); + assertThat(newFilter).isInstanceOf(Log4j2Metrics.MetricsFilter.class); + } + } + + @Test + void shouldNotRebindMetricsIfBinderIsClosed() { + LoggerContext context = new LoggerContext("test"); + Logger logger = context.getLogger("com.test"); + + try (Log4j2Metrics metrics = new Log4j2Metrics(emptyList(), context)) { + metrics.bindTo(registry); + logger.error("first"); + } + + // This will reload the configuration to default + context.reconfigure(); + + // This event should not be counted as the metrics binder is already closed + logger.error("second"); + + assertThat(registry.get("log4j2.events").tags("level", "error").counter().count()).isEqualTo(1); + } + }