From e4c9658b75dfa0ecf2a0da3fe5dfaf7f9e1b6fd9 Mon Sep 17 00:00:00 2001 From: Patrik Ivarsson <83230533+pativa@users.noreply.github.com> Date: Tue, 18 Feb 2025 05:38:22 +0100 Subject: [PATCH] Handle programatically added log4j2 LoggerConfig (#5902) The goal of this is to be able to bind to a newly added `LoggerConfig` even if the `Configuration` is the same. This means we always need to check if the filter was already added to avoid adding it twice. We need to keep track of if anything was changed to avoid an endless loop when we call `updateLoggers()` Signed-off-by: Patrik Ivarsson --- .../binder/logging/Log4j2Metrics.java | 49 +++++++++---------- .../binder/logging/Log4j2MetricsTest.java | 30 ++++++++++++ 2 files changed, 54 insertions(+), 25 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 0fa8aaa88c..fab6bbb54a 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 @@ -96,11 +96,13 @@ public void bindTo(MeterRegistry registry) { loggerContext.updateLoggers(configuration); PropertyChangeListener changeListener = listener -> { - if (listener.getNewValue() instanceof Configuration && listener.getOldValue() != listener.getNewValue()) { + if (listener.getNewValue() instanceof Configuration) { Configuration newConfiguration = (Configuration) listener.getNewValue(); - registerMetricsFilter(newConfiguration, registry); - loggerContext.updateLoggers(newConfiguration); - if (listener.getOldValue() instanceof Configuration) { + if (registerMetricsFilter(newConfiguration, registry)) { + loggerContext.updateLoggers(newConfiguration); + } + if (listener.getOldValue() != listener.getNewValue() + && listener.getOldValue() instanceof Configuration) { removeMetricsFilter((Configuration) listener.getOldValue()); } } @@ -110,32 +112,29 @@ public void bindTo(MeterRegistry registry) { loggerContext.addPropertyChangeListener(changeListener); } - private void registerMetricsFilter(Configuration configuration, MeterRegistry registry) { - LoggerConfig rootLoggerConfig = configuration.getRootLogger(); + private boolean registerMetricsFilter(Configuration configuration, MeterRegistry registry) { MetricsFilter metricsFilter = getOrCreateMetricsFilterAndStart(registry); - rootLoggerConfig.addFilter(metricsFilter); + LoggerConfig rootLoggerConfig = configuration.getRootLogger(); + boolean changed = addFilterIfAbsent(rootLoggerConfig, metricsFilter); - configuration.getLoggers() - .values() - .stream() - .filter(loggerConfig -> !loggerConfig.isAdditive()) - .forEach(loggerConfig -> { - if (loggerConfig == rootLoggerConfig) { - return; - } - Filter logFilter = loggerConfig.getFilter(); + for (LoggerConfig loggerConfig : configuration.getLoggers().values()) { + if (loggerConfig != rootLoggerConfig && !loggerConfig.isAdditive()) { + changed |= addFilterIfAbsent(loggerConfig, metricsFilter); + } + } - if (metricsFilter.equals(logFilter)) { - return; - } + return changed; + } - if (logFilter instanceof CompositeFilter - && Arrays.asList(((CompositeFilter) logFilter).getFiltersArray()).contains(metricsFilter)) { - return; - } + private boolean addFilterIfAbsent(LoggerConfig loggerConfig, MetricsFilter metricsFilter) { + Filter existingFilter = loggerConfig.getFilter(); + if (existingFilter == metricsFilter || (existingFilter instanceof CompositeFilter + && Arrays.asList(((CompositeFilter) existingFilter).getFiltersArray()).contains(metricsFilter))) { + return false; + } - loggerConfig.addFilter(metricsFilter); - }); + loggerConfig.addFilter(metricsFilter); + return true; } private MetricsFilter getOrCreateMetricsFilterAndStart(MeterRegistry registry) { 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 e4987fabbb..f5e2bafb19 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 @@ -368,4 +368,34 @@ void bindingTwiceToSameRegistry_doesNotDoubleCount() { assertThat(registry.get("log4j2.events").tags("level", "error").counter().count()).isEqualTo(2); } + @Issue("#5901") + @Test + void programmaticallyAddedLoggerConfigShouldBeCounted() { + LoggerContext context = new LoggerContext("test"); + + Log4j2Metrics metrics = new Log4j2Metrics(emptyList(), context); + metrics.bindTo(registry); + + Configuration configuration = context.getConfiguration(); + + // here we add a logger programmatically, metrics for this will not be counted + // until updateLoggers is called + LoggerConfig addedLoggerConfig = new LoggerConfig("com.test", Level.DEBUG, false); + configuration.addLogger("com.test", addedLoggerConfig); + Logger logger = context.getLogger("com.test"); + + assertThat(addedLoggerConfig.getFilter()).isNull(); + + context.updateLoggers(configuration); + + assertThat(addedLoggerConfig.getFilter()).isInstanceOf(Log4j2Metrics.MetricsFilter.class); + + logger.debug("test"); + assertThat(registry.get("log4j2.events").tags("level", "debug").counter().count()).isEqualTo(1); + + metrics.close(); + + assertThat(addedLoggerConfig.getFilter()).isNull(); + } + }