From 33fb1fa9a3152d0101fb3feb50da4af5ca38d03c Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 19 Dec 2018 16:47:30 +0000 Subject: [PATCH] Avoid creating meter binders before registry has been customized Previously, MeterRegistryPostProcessor would trigger the creation of all meter binders and meter registry customizers before applying the customizers and calling the binders. In some situations with complex dependency graphs where the creation of a binder and the injection of its dependencies inadvertently triggered some meter binding, this could result in meters being bound before the registry had been customized. This commit reworks MeterRegistryPostProcessor and MeterRegistryConfigurer to defer the retrieval of registry customizers and meter binders until just before they are needed. As a result, customizers are now retrieved and applied before the binders are retrieved. Closes gh-15483 --- .../metrics/MeterRegistryConfigurer.java | 34 ++++---- .../metrics/MeterRegistryPostProcessor.java | 12 +-- ...terRegistryConfigurerIntegrationTests.java | 77 +++++++++++++++++++ .../metrics/MeterRegistryConfigurerTests.java | 52 +++++++++---- 4 files changed, 137 insertions(+), 38 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 0fc251b007..846e402b2d 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 @@ -16,14 +16,15 @@ package org.springframework.boot.actuate.autoconfigure.metrics; -import java.util.Collection; -import java.util.Collections; +import java.util.List; +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.config.MeterFilter; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.boot.util.LambdaSafe; /** @@ -36,21 +37,20 @@ import org.springframework.boot.util.LambdaSafe; */ class MeterRegistryConfigurer { - private final Collection> customizers; + private final ObjectProvider> customizers; - private final Collection filters; + private final ObjectProvider filters; - private final Collection binders; + private final ObjectProvider binders; private final boolean addToGlobalRegistry; - MeterRegistryConfigurer(Collection binders, - Collection filters, - Collection> customizers, + MeterRegistryConfigurer(ObjectProvider> customizers, + ObjectProvider filters, ObjectProvider binders, boolean addToGlobalRegistry) { - this.binders = (binders != null) ? binders : Collections.emptyList(); - this.filters = (filters != null) ? filters : Collections.emptyList(); - this.customizers = (customizers != null) ? customizers : Collections.emptyList(); + this.customizers = customizers; + this.filters = filters; + this.binders = binders; this.addToGlobalRegistry = addToGlobalRegistry; } @@ -67,17 +67,23 @@ class MeterRegistryConfigurer { @SuppressWarnings("unchecked") private void customize(MeterRegistry registry) { - LambdaSafe.callbacks(MeterRegistryCustomizer.class, this.customizers, registry) + LambdaSafe + .callbacks(MeterRegistryCustomizer.class, asOrderedList(this.customizers), + registry) .withLogger(MeterRegistryConfigurer.class) .invoke((customizer) -> customizer.customize(registry)); } private void addFilters(MeterRegistry registry) { - this.filters.forEach(registry.config()::meterFilter); + this.filters.orderedStream().forEach(registry.config()::meterFilter); } private void addBinders(MeterRegistry registry) { - this.binders.forEach((binder) -> binder.bindTo(registry)); + this.binders.orderedStream().forEach((binder) -> binder.bindTo(registry)); + } + + private List asOrderedList(ObjectProvider provider) { + return provider.orderedStream().collect(Collectors.toList()); } } 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 1c96c9c298..342140476c 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 @@ -16,9 +16,6 @@ package org.springframework.boot.actuate.autoconfigure.metrics; -import java.util.List; -import java.util.stream.Collectors; - import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.binder.MeterBinder; import io.micrometer.core.instrument.config.MeterFilter; @@ -68,16 +65,11 @@ class MeterRegistryPostProcessor implements BeanPostProcessor { private MeterRegistryConfigurer getConfigurer() { if (this.configurer == null) { - this.configurer = new MeterRegistryConfigurer( - asOrderedList(this.meterBinders), asOrderedList(this.meterFilters), - asOrderedList(this.meterRegistryCustomizers), + this.configurer = new MeterRegistryConfigurer(this.meterRegistryCustomizers, + this.meterFilters, this.meterBinders, this.metricsProperties.getObject().isUseGlobalRegistry()); } return this.configurer; } - private List asOrderedList(ObjectProvider provider) { - return provider.orderedStream().collect(Collectors.toList()); - } - } 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 cc97942fe4..4a1f2c89b7 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 @@ -17,14 +17,21 @@ 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 org.junit.Test; +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.prometheus.PrometheusMetricsExportAutoConfiguration; +import org.springframework.boot.actuate.autoconfigure.metrics.export.simple.SimpleMetricsExportAutoConfiguration; import org.springframework.boot.actuate.autoconfigure.metrics.test.MetricsRun; import org.springframework.boot.autoconfigure.AutoConfigurations; import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.context.ApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; /** * Integration tests for {@link MeterRegistryConfigurer}. @@ -49,4 +56,74 @@ public class MeterRegistryConfigurerIntegrationTests { }); } + @Test + public void customizersAreAppliedBeforeBindersAreCreated() { + new ApplicationContextRunner() + .withConfiguration(AutoConfigurations.of(MetricsAutoConfiguration.class, + SimpleMetricsExportAutoConfiguration.class)) + .withUserConfiguration(TestConfiguration.class).run((context) -> { + + }); + } + + @Configuration + static class TestConfiguration { + + @Bean + MeterBinder testBinder(Alpha thing) { + return (registry) -> { + }; + } + + @Bean + MeterRegistryCustomizer testCustomizer() { + return (registry) -> { + registry.config().commonTags("testTag", "testValue"); + }; + } + + @Bean + Alpha alpha() { + return new Alpha(); + } + + @Bean + Bravo bravo(Alpha alpha) { + return new Bravo(alpha); + } + + @Bean + static BeanPostProcessor testPostProcessor(ApplicationContext context) { + return new BeanPostProcessor() { + + @Override + public Object postProcessAfterInitialization(Object bean, String beanName) + throws BeansException { + if (bean instanceof Bravo) { + MeterRegistry meterRegistry = context + .getBean(MeterRegistry.class); + meterRegistry.gauge("test", 1); + System.out.println( + meterRegistry.find("test").gauge().getId().getTags()); + } + return bean; + } + + }; + } + + } + + static class Alpha { + + } + + static class Bravo { + + Bravo(Alpha alpha) { + + } + + } + } 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 9bd41e76c6..7f0c8f4c52 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 @@ -31,9 +31,12 @@ import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import org.springframework.beans.factory.ObjectProvider; + import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; /** @@ -74,8 +77,10 @@ public class MeterRegistryConfigurerTests { @Test public void configureWhenCompositeShouldApplyCustomizer() { this.customizers.add(this.mockCustomizer); - MeterRegistryConfigurer configurer = new MeterRegistryConfigurer(this.binders, - this.filters, this.customizers, false); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + false); CompositeMeterRegistry composite = new CompositeMeterRegistry(); configurer.configure(composite); verify(this.mockCustomizer).customize(composite); @@ -84,8 +89,10 @@ public class MeterRegistryConfigurerTests { @Test public void configureShouldApplyCustomizer() { this.customizers.add(this.mockCustomizer); - MeterRegistryConfigurer configurer = new MeterRegistryConfigurer(this.binders, - this.filters, this.customizers, false); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + false); configurer.configure(this.mockRegistry); verify(this.mockCustomizer).customize(this.mockRegistry); } @@ -93,8 +100,10 @@ public class MeterRegistryConfigurerTests { @Test public void configureShouldApplyFilter() { this.filters.add(this.mockFilter); - MeterRegistryConfigurer configurer = new MeterRegistryConfigurer(this.binders, - this.filters, this.customizers, false); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + false); configurer.configure(this.mockRegistry); verify(this.mockConfig).meterFilter(this.mockFilter); } @@ -102,8 +111,10 @@ public class MeterRegistryConfigurerTests { @Test public void configureShouldApplyBinder() { this.binders.add(this.mockBinder); - MeterRegistryConfigurer configurer = new MeterRegistryConfigurer(this.binders, - this.filters, this.customizers, false); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + false); configurer.configure(this.mockRegistry); verify(this.mockBinder).bindTo(this.mockRegistry); } @@ -113,8 +124,10 @@ public class MeterRegistryConfigurerTests { this.customizers.add(this.mockCustomizer); this.filters.add(this.mockFilter); this.binders.add(this.mockBinder); - MeterRegistryConfigurer configurer = new MeterRegistryConfigurer(this.binders, - this.filters, this.customizers, false); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + false); configurer.configure(this.mockRegistry); InOrder ordered = inOrder(this.mockBinder, this.mockConfig, this.mockCustomizer); ordered.verify(this.mockCustomizer).customize(this.mockRegistry); @@ -124,8 +137,10 @@ public class MeterRegistryConfigurerTests { @Test public void configureWhenAddToGlobalRegistryShouldAddToGlobalRegistry() { - MeterRegistryConfigurer configurer = new MeterRegistryConfigurer(this.binders, - this.filters, this.customizers, true); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + true); try { configurer.configure(this.mockRegistry); assertThat(Metrics.globalRegistry.getRegistries()) @@ -138,11 +153,20 @@ public class MeterRegistryConfigurerTests { @Test public void configureWhenNotAddToGlobalRegistryShouldAddToGlobalRegistry() { - MeterRegistryConfigurer configurer = new MeterRegistryConfigurer(this.binders, - this.filters, this.customizers, false); + MeterRegistryConfigurer configurer = new MeterRegistryConfigurer( + createObjectProvider(this.customizers), + createObjectProvider(this.filters), createObjectProvider(this.binders), + false); configurer.configure(this.mockRegistry); assertThat(Metrics.globalRegistry.getRegistries()) .doesNotContain(this.mockRegistry); } + @SuppressWarnings("unchecked") + private ObjectProvider createObjectProvider(List objects) { + ObjectProvider objectProvider = mock(ObjectProvider.class); + given(objectProvider.orderedStream()).willReturn(objects.stream()); + return objectProvider; + } + }