Merge pull request #16221 from Johnny Lim

wilkinsona · wilkinsona · commit 4fc813b246e3 · 2019-04-24T15:08:09.000+01:00
* gh-16221: Polish "Prevent double update of metrics when CompositeMeterRegistry exists" Prevent double update of metrics when CompositeMeterRegistry exists Closes gh-16221
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
@@ -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.
@@ -22,6 +22,7 @@
 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,21 +46,27 @@ class MeterRegistryConfigurer {
 
 	private final boolean addToGlobalRegistry;
 
+	private final boolean hasCompositeMeterRegistry;
+
 	MeterRegistryConfigurer(ObjectProvider<MeterRegistryCustomizer<?>> customizers,
 			ObjectProvider<MeterFilter> filters, ObjectProvider<MeterBinder> 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) {
 		// Customizers must be applied before binders, as they may add custom
 		// tags or alter timer or summary configuration.
 		customize(registry);
 		addFilters(registry);
-		addBinders(registry);
+		if (!this.hasCompositeMeterRegistry
+				|| registry instanceof CompositeMeterRegistry) {
+			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
@@ -18,11 +18,13 @@
 
 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<MeterBinder> meterBinders,
 			ObjectProvider<MeterFilter> meterFilters,
 			ObjectProvider<MeterRegistryCustomizer<?>> meterRegistryCustomizers,
-			ObjectProvider<MetricsProperties> metricsProperties) {
+			ObjectProvider<MetricsProperties> metricsProperties,
+			ApplicationContext applicationContext) {
 		this.meterBinders = meterBinders;
 		this.meterFilters = meterFilters;
 		this.meterRegistryCustomizers = meterRegistryCustomizers;
 		this.metricsProperties = metricsProperties;
+		this.applicationContext = applicationContext;
 	}
 
 	@Override
@@ -65,9 +71,13 @@ public Object postProcessAfterInitialization(Object bean, String beanName)
 
 	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
@@ -27,6 +27,7 @@
 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 static MeterRegistryPostProcessor meterRegistryPostProcessor(
 			ObjectProvider<MeterBinder> meterBinders,
 			ObjectProvider<MeterFilter> meterFilters,
 			ObjectProvider<MeterRegistryCustomizer<?>> meterRegistryCustomizers,
-			ObjectProvider<MetricsProperties> metricsProperties) {
+			ObjectProvider<MetricsProperties> 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
@@ -16,14 +16,20 @@
 
 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.impl.StaticLoggerBinder;
 
 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 +39,8 @@
 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 +74,47 @@ public void customizersAreAppliedBeforeBindersAreCreated() {
 				});
 	}
 
+	@Test
+	public void counterIsIncrementedOncePerEventWithoutCompositeMeterRegistry() {
+		new ApplicationContextRunner()
+				.with(MetricsRun.limitedTo(JmxMetricsExportAutoConfiguration.class))
+				.withConfiguration(
+						AutoConfigurations.of(LogbackMetricsAutoConfiguration.class))
+				.run((context) -> {
+					Logger logger = ((LoggerContext) StaticLoggerBinder.getSingleton()
+							.getLoggerFactory()).getLogger("test-logger");
+					logger.error("Error.");
+
+					Map<String, MeterRegistry> 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 counterIsIncrementedOncePerEventWithCompositeMeterRegistry() {
+		new ApplicationContextRunner()
+				.with(MetricsRun.limitedTo(JmxMetricsExportAutoConfiguration.class,
+						PrometheusMetricsExportAutoConfiguration.class))
+				.withConfiguration(
+						AutoConfigurations.of(LogbackMetricsAutoConfiguration.class))
+				.run((context) -> {
+					Logger logger = ((LoggerContext) StaticLoggerBinder.getSingleton()
+							.getLoggerFactory()).getLogger("test-logger");
+					logger.error("Error.");
+					Map<String, MeterRegistry> 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
@@ -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.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 void configureWhenCompositeShouldApplyCustomizer() {
 		MeterRegistryConfigurer configurer = new MeterRegistryConfigurer(
 				createObjectProvider(this.customizers),
 				createObjectProvider(this.filters), createObjectProvider(this.binders),
-				false);
+				false, false);
 		CompositeMeterRegistry composite = new CompositeMeterRegistry();
 		configurer.configure(composite);
 		verify(this.mockCustomizer).customize(composite);
@@ -92,7 +93,7 @@ public void configureShouldApplyCustomizer() {
 		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 +104,7 @@ public void configureShouldApplyFilter() {
 		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,11 +115,34 @@ public void configureShouldApplyBinder() {
 		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);
 	}
 
+	@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);
@@ -127,7 +151,7 @@ public void configureShouldBeCalledInOrderCustomizerFilterBinder() {
 		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 +164,7 @@ public void configureWhenAddToGlobalRegistryShouldAddToGlobalRegistry() {
 		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 +180,7 @@ public void configureWhenNotAddToGlobalRegistryShouldAddToGlobalRegistry() {
 		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);
