From cc285d92dd07f2f03c5bb8935124740322b1cb9e Mon Sep 17 00:00:00 2001 From: Johnny Lim Date: Thu, 14 Mar 2019 01:36:56 +0900 Subject: [PATCH 1/2] Prevent double update of metrics when CompositeMeterRegistry exists See gh-16221 --- .../metrics/MeterRegistryConfigurer.java | 15 +++++- .../metrics/MeterRegistryPostProcessor.java | 14 +++++- .../metrics/MetricsAutoConfiguration.java | 6 ++- ...terRegistryConfigurerIntegrationTests.java | 47 +++++++++++++++++++ .../metrics/MeterRegistryConfigurerTests.java | 14 +++--- 5 files changed, 83 insertions(+), 13 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurer.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurer.java index 8dc29143d6..89c932b20d 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurer.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurer.java @@ -22,6 +22,7 @@ import java.util.stream.Collectors; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.binder.MeterBinder; +import io.micrometer.core.instrument.composite.CompositeMeterRegistry; import io.micrometer.core.instrument.config.MeterFilter; import org.springframework.beans.factory.ObjectProvider; @@ -45,13 +46,16 @@ class MeterRegistryConfigurer { private final boolean addToGlobalRegistry; + private final boolean hasCompositeMeterRegistry; + MeterRegistryConfigurer(ObjectProvider> customizers, ObjectProvider filters, ObjectProvider binders, - boolean addToGlobalRegistry) { + boolean addToGlobalRegistry, boolean hasCompositeMeterRegistry) { this.customizers = customizers; this.filters = filters; this.binders = binders; this.addToGlobalRegistry = addToGlobalRegistry; + this.hasCompositeMeterRegistry = hasCompositeMeterRegistry; } void configure(MeterRegistry registry) { @@ -59,7 +63,14 @@ class MeterRegistryConfigurer { // tags or alter timer or summary configuration. customize(registry); addFilters(registry); - addBinders(registry); + if (this.hasCompositeMeterRegistry) { + if (registry instanceof CompositeMeterRegistry) { + addBinders(registry); + } + } + else { + addBinders(registry); + } if (this.addToGlobalRegistry && registry != Metrics.globalRegistry) { Metrics.addRegistry(registry); } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryPostProcessor.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryPostProcessor.java index d1087cef73..f40aa92667 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryPostProcessor.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryPostProcessor.java @@ -18,11 +18,13 @@ package org.springframework.boot.actuate.autoconfigure.metrics; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.binder.MeterBinder; +import io.micrometer.core.instrument.composite.CompositeMeterRegistry; import io.micrometer.core.instrument.config.MeterFilter; import org.springframework.beans.BeansException; import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.config.BeanPostProcessor; +import org.springframework.context.ApplicationContext; /** * {@link BeanPostProcessor} that delegates to a lazily created @@ -44,14 +46,18 @@ class MeterRegistryPostProcessor implements BeanPostProcessor { private volatile MeterRegistryConfigurer configurer; + private final ApplicationContext applicationContext; + MeterRegistryPostProcessor(ObjectProvider meterBinders, ObjectProvider meterFilters, ObjectProvider> meterRegistryCustomizers, - ObjectProvider metricsProperties) { + ObjectProvider metricsProperties, + ApplicationContext applicationContext) { this.meterBinders = meterBinders; this.meterFilters = meterFilters; this.meterRegistryCustomizers = meterRegistryCustomizers; this.metricsProperties = metricsProperties; + this.applicationContext = applicationContext; } @Override @@ -65,9 +71,13 @@ class MeterRegistryPostProcessor implements BeanPostProcessor { private MeterRegistryConfigurer getConfigurer() { if (this.configurer == null) { + boolean hasCompositeMeterRegistry = this.applicationContext + .getBeanNamesForType(CompositeMeterRegistry.class, false, + false).length != 0; this.configurer = new MeterRegistryConfigurer(this.meterRegistryCustomizers, this.meterFilters, this.meterBinders, - this.metricsProperties.getObject().isUseGlobalRegistry()); + this.metricsProperties.getObject().isUseGlobalRegistry(), + hasCompositeMeterRegistry); } return this.configurer; } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsAutoConfiguration.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsAutoConfiguration.java index 7da1d6559b..c82f00d27d 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsAutoConfiguration.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsAutoConfiguration.java @@ -27,6 +27,7 @@ import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.context.ApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.annotation.Order; @@ -55,9 +56,10 @@ public class MetricsAutoConfiguration { ObjectProvider meterBinders, ObjectProvider meterFilters, ObjectProvider> meterRegistryCustomizers, - ObjectProvider metricsProperties) { + ObjectProvider metricsProperties, + ApplicationContext applicationContext) { return new MeterRegistryPostProcessor(meterBinders, meterFilters, - meterRegistryCustomizers, metricsProperties); + meterRegistryCustomizers, metricsProperties, applicationContext); } @Bean diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerIntegrationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerIntegrationTests.java index e77cea6919..52b19b2dac 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerIntegrationTests.java @@ -16,14 +16,19 @@ package org.springframework.boot.actuate.autoconfigure.metrics; +import java.util.Map; + import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.binder.MeterBinder; import io.micrometer.core.instrument.composite.CompositeMeterRegistry; import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.BeansException; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.boot.actuate.autoconfigure.metrics.export.atlas.AtlasMetricsExportAutoConfiguration; +import org.springframework.boot.actuate.autoconfigure.metrics.export.jmx.JmxMetricsExportAutoConfiguration; import org.springframework.boot.actuate.autoconfigure.metrics.export.prometheus.PrometheusMetricsExportAutoConfiguration; import org.springframework.boot.actuate.autoconfigure.metrics.export.simple.SimpleMetricsExportAutoConfiguration; import org.springframework.boot.actuate.autoconfigure.metrics.test.MetricsRun; @@ -33,6 +38,8 @@ import org.springframework.context.ApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import static org.assertj.core.api.Assertions.assertThat; + /** * Integration tests for {@link MeterRegistryConfigurer}. * @@ -66,6 +73,46 @@ public class MeterRegistryConfigurerIntegrationTests { }); } + @Test + public void counterWhenCompositeMeterRegistryDoesNotExistShouldWork() { + new ApplicationContextRunner() + .with(MetricsRun.limitedTo(JmxMetricsExportAutoConfiguration.class)) + .withConfiguration( + AutoConfigurations.of(LogbackMetricsAutoConfiguration.class)) + .run((context) -> { + Logger logger = LoggerFactory.getLogger("test-logger"); + logger.error("Error."); + + Map registriesByName = context + .getBeansOfType(MeterRegistry.class); + assertThat(registriesByName).hasSize(1); + MeterRegistry registry = registriesByName.values().iterator().next(); + assertThat(registry.get("logback.events").tag("level", "error") + .counter().count()).isEqualTo(1); + }); + } + + @Test + public void counterWhenCompositeMeterRegistryExistsShouldWork() { + new ApplicationContextRunner() + .with(MetricsRun.limitedTo(JmxMetricsExportAutoConfiguration.class, + PrometheusMetricsExportAutoConfiguration.class)) + .withConfiguration( + AutoConfigurations.of(LogbackMetricsAutoConfiguration.class)) + .run((context) -> { + Logger logger = LoggerFactory.getLogger("test-logger"); + logger.error("Error."); + + Map registriesByName = context + .getBeansOfType(MeterRegistry.class); + assertThat(registriesByName).hasSize(3); + registriesByName.forEach((name, + registry) -> assertThat(registry.get("logback.events") + .tag("level", "error").counter().count()) + .isEqualTo(1)); + }); + } + @Configuration static class TestConfiguration { diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerTests.java index 659da97fe9..3e28b70487 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerTests.java @@ -80,7 +80,7 @@ public class MeterRegistryConfigurerTests { MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( createObjectProvider(this.customizers), createObjectProvider(this.filters), createObjectProvider(this.binders), - false); + false, true); CompositeMeterRegistry composite = new CompositeMeterRegistry(); configurer.configure(composite); verify(this.mockCustomizer).customize(composite); @@ -92,7 +92,7 @@ public class MeterRegistryConfigurerTests { MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( createObjectProvider(this.customizers), createObjectProvider(this.filters), createObjectProvider(this.binders), - false); + false, false); configurer.configure(this.mockRegistry); verify(this.mockCustomizer).customize(this.mockRegistry); } @@ -103,7 +103,7 @@ public class MeterRegistryConfigurerTests { MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( createObjectProvider(this.customizers), createObjectProvider(this.filters), createObjectProvider(this.binders), - false); + false, false); configurer.configure(this.mockRegistry); verify(this.mockConfig).meterFilter(this.mockFilter); } @@ -114,7 +114,7 @@ public class MeterRegistryConfigurerTests { MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( createObjectProvider(this.customizers), createObjectProvider(this.filters), createObjectProvider(this.binders), - false); + false, false); configurer.configure(this.mockRegistry); verify(this.mockBinder).bindTo(this.mockRegistry); } @@ -127,7 +127,7 @@ public class MeterRegistryConfigurerTests { MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( createObjectProvider(this.customizers), createObjectProvider(this.filters), createObjectProvider(this.binders), - false); + false, false); configurer.configure(this.mockRegistry); InOrder ordered = inOrder(this.mockBinder, this.mockConfig, this.mockCustomizer); ordered.verify(this.mockCustomizer).customize(this.mockRegistry); @@ -140,7 +140,7 @@ public class MeterRegistryConfigurerTests { MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( createObjectProvider(this.customizers), createObjectProvider(this.filters), createObjectProvider(this.binders), - true); + true, false); try { configurer.configure(this.mockRegistry); assertThat(Metrics.globalRegistry.getRegistries()) @@ -156,7 +156,7 @@ public class MeterRegistryConfigurerTests { MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( createObjectProvider(this.customizers), createObjectProvider(this.filters), createObjectProvider(this.binders), - false); + false, false); configurer.configure(this.mockRegistry); assertThat(Metrics.globalRegistry.getRegistries()) .doesNotContain(this.mockRegistry); From 6b20d13b3d0201b5c2d07909aa808a27c094c73c Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 24 Apr 2019 14:50:30 +0100 Subject: [PATCH 2/2] Polish "Prevent double update of metrics when CompositeMeterRegistry exists" See gh-16221 --- .../metrics/MeterRegistryConfigurer.java | 10 ++----- ...terRegistryConfigurerIntegrationTests.java | 16 ++++++----- .../metrics/MeterRegistryConfigurerTests.java | 28 +++++++++++++++++-- 3 files changed, 38 insertions(+), 16 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurer.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurer.java index 89c932b20d..96752e02eb 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurer.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurer.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2018 the original author or authors. + * Copyright 2012-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -63,12 +63,8 @@ class MeterRegistryConfigurer { // tags or alter timer or summary configuration. customize(registry); addFilters(registry); - if (this.hasCompositeMeterRegistry) { - if (registry instanceof CompositeMeterRegistry) { - addBinders(registry); - } - } - else { + if (!this.hasCompositeMeterRegistry + || registry instanceof CompositeMeterRegistry) { addBinders(registry); } if (this.addToGlobalRegistry && registry != Metrics.globalRegistry) { diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerIntegrationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerIntegrationTests.java index 52b19b2dac..53f2b754e4 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerIntegrationTests.java @@ -18,12 +18,13 @@ package org.springframework.boot.actuate.autoconfigure.metrics; import java.util.Map; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.LoggerContext; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.binder.MeterBinder; import io.micrometer.core.instrument.composite.CompositeMeterRegistry; import org.junit.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.slf4j.impl.StaticLoggerBinder; import org.springframework.beans.BeansException; import org.springframework.beans.factory.config.BeanPostProcessor; @@ -74,13 +75,14 @@ public class MeterRegistryConfigurerIntegrationTests { } @Test - public void counterWhenCompositeMeterRegistryDoesNotExistShouldWork() { + public void counterIsIncrementedOncePerEventWithoutCompositeMeterRegistry() { new ApplicationContextRunner() .with(MetricsRun.limitedTo(JmxMetricsExportAutoConfiguration.class)) .withConfiguration( AutoConfigurations.of(LogbackMetricsAutoConfiguration.class)) .run((context) -> { - Logger logger = LoggerFactory.getLogger("test-logger"); + Logger logger = ((LoggerContext) StaticLoggerBinder.getSingleton() + .getLoggerFactory()).getLogger("test-logger"); logger.error("Error."); Map registriesByName = context @@ -93,16 +95,16 @@ public class MeterRegistryConfigurerIntegrationTests { } @Test - public void counterWhenCompositeMeterRegistryExistsShouldWork() { + public void counterIsIncrementedOncePerEventWithCompositeMeterRegistry() { new ApplicationContextRunner() .with(MetricsRun.limitedTo(JmxMetricsExportAutoConfiguration.class, PrometheusMetricsExportAutoConfiguration.class)) .withConfiguration( AutoConfigurations.of(LogbackMetricsAutoConfiguration.class)) .run((context) -> { - Logger logger = LoggerFactory.getLogger("test-logger"); + Logger logger = ((LoggerContext) StaticLoggerBinder.getSingleton() + .getLoggerFactory()).getLogger("test-logger"); logger.error("Error."); - Map registriesByName = context .getBeansOfType(MeterRegistry.class); assertThat(registriesByName).hasSize(3); diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerTests.java index 3e28b70487..746b71bd02 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/MeterRegistryConfigurerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2018 the original author or authors. + * Copyright 2012-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -38,6 +38,7 @@ import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; /** * Tests for {@link MeterRegistryConfigurer}. @@ -80,7 +81,7 @@ public class MeterRegistryConfigurerTests { MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( createObjectProvider(this.customizers), createObjectProvider(this.filters), createObjectProvider(this.binders), - false, true); + false, false); CompositeMeterRegistry composite = new CompositeMeterRegistry(); configurer.configure(composite); verify(this.mockCustomizer).customize(composite); @@ -119,6 +120,29 @@ public class MeterRegistryConfigurerTests { verify(this.mockBinder).bindTo(this.mockRegistry); } + @Test + public void configureShouldApplyBinderToComposite() { + this.binders.add(this.mockBinder); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + false, true); + CompositeMeterRegistry composite = new CompositeMeterRegistry(); + configurer.configure(composite); + verify(this.mockBinder).bindTo(composite); + } + + @Test + public void configureShouldNotApplyBinderWhenCompositeExists() { + this.binders.add(this.mockBinder); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + false, true); + configurer.configure(this.mockRegistry); + verifyZeroInteractions(this.mockBinder); + } + @Test public void configureShouldBeCalledInOrderCustomizerFilterBinder() { this.customizers.add(this.mockCustomizer);