Merge pull request #16221 from Johnny Lim

* gh-16221:
  Polish "Prevent double update of metrics when CompositeMeterRegistry exists"
  Prevent double update of metrics when CompositeMeterRegistry exists

Closes gh-16221

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 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<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) {
@@ -59,7 +63,10 @@ class MeterRegistryConfigurer {
 		// 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);
 		}

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<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 @@ 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;
 	}

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<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

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.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 +74,47 @@ public class MeterRegistryConfigurerIntegrationTests {
 				});
 	}
 
+	@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 {
 

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);
+				false, false);
 		CompositeMeterRegistry composite = new CompositeMeterRegistry();
 		configurer.configure(composite);
 		verify(this.mockCustomizer).customize(composite);
@@ -92,7 +93,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 +104,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,11 +115,34 @@ 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);
 	}
 
+	@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 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 +164,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 +180,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);