Merge pull request #32528 from marcingrzejszczak

* gh-32528: Polish "Break cycles between Zipkin senders and HTTP client observation" Break cycles between Zipkin senders and HTTP client observation Closes gh-32528
spring-projects · Sep 28, 2022 · 05d2f3c · 05d2f3c
2 parents 13c638b + 574242b
commit 05d2f3c
Show file tree

Hide file tree

Showing 5 changed files with 172 additions and 20 deletions.
diff --git a/...a/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java b/...a/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java
@@ -25,6 +25,7 @@
 import zipkin2.reporter.brave.ZipkinSpanHandler;
 import zipkin2.reporter.urlconnection.URLConnectionSender;
 
+import org.springframework.beans.factory.ObjectProvider;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnBean;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
@@ -73,12 +74,12 @@ static class RestTemplateSenderConfiguration {
 
 		@Bean
 		@ConditionalOnMissingBean(Sender.class)
-		@ConditionalOnBean(RestTemplateBuilder.class)
 		ZipkinRestTemplateSender restTemplateSender(ZipkinProperties properties,
-				RestTemplateBuilder restTemplateBuilder) {
-			RestTemplate restTemplate = restTemplateBuilder.setConnectTimeout(properties.getConnectTimeout())
-					.setReadTimeout(properties.getReadTimeout()).build();
-			return new ZipkinRestTemplateSender(properties.getEndpoint(), restTemplate);
+				ObjectProvider<ZipkinRestTemplateBuilderCustomizer> customizers) {
+			RestTemplateBuilder restTemplateBuilder = new RestTemplateBuilder()
+					.setConnectTimeout(properties.getConnectTimeout()).setReadTimeout(properties.getReadTimeout());
+			customizers.orderedStream().forEach((c) -> c.customize(restTemplateBuilder));
+			return new ZipkinRestTemplateSender(properties.getEndpoint(), restTemplateBuilder.build());
 		}
 
 	}
@@ -90,10 +91,11 @@ static class WebClientSenderConfiguration {
 
 		@Bean
 		@ConditionalOnMissingBean(Sender.class)
-		@ConditionalOnBean(WebClient.Builder.class)
-		ZipkinWebClientSender webClientSender(ZipkinProperties properties, WebClient.Builder webClientBuilder) {
-			WebClient webClient = webClientBuilder.build();
-			return new ZipkinWebClientSender(properties.getEndpoint(), webClient);
+		ZipkinWebClientSender webClientSender(ZipkinProperties properties,
+				ObjectProvider<ZipkinWebClientBuilderCustomizer> customizers) {
+			WebClient.Builder builder = WebClient.builder();
+			customizers.orderedStream().forEach((c) -> c.customize(builder));
+			return new ZipkinWebClientSender(properties.getEndpoint(), builder.build());
 		}
 
 	}

diff --git a/...mework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinRestTemplateBuilderCustomizer.java b/...mework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinRestTemplateBuilderCustomizer.java
@@ -0,0 +1,37 @@
+/*
+ * Copyright 2012-2022 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.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.boot.actuate.autoconfigure.tracing.zipkin;
+
+import org.springframework.boot.web.client.RestTemplateBuilder;
+
+/**
+ * Callback interface that can be implemented by beans wishing to customize the
+ * {@link RestTemplateBuilder} used to send spans to Zipkin.
+ *
+ * @author Marcin Grzejszczak
+ * @since 3.0.0
+ */
+@FunctionalInterface
+public interface ZipkinRestTemplateBuilderCustomizer {
+
+	/**
+	 * Customize the rest template builder.
+	 * @param restTemplateBuilder the {@code RestTemplateBuilder} to customize
+	 */
+	void customize(RestTemplateBuilder restTemplateBuilder);
+
+}
diff --git a/...framework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinWebClientBuilderCustomizer.java b/...framework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinWebClientBuilderCustomizer.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2012-2022 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.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.boot.actuate.autoconfigure.tracing.zipkin;
+
+import org.springframework.web.reactive.function.client.WebClient;
+import org.springframework.web.reactive.function.client.WebClient.Builder;
+
+/**
+ * Callback interface that can be implemented by beans wishing to customize the
+ * {@link Builder} used to send spans to Zipkin.
+ *
+ * @author Marcin Grzejszczak
+ * @since 3.0.0
+ */
+@FunctionalInterface
+public interface ZipkinWebClientBuilderCustomizer {
+
+	/**
+	 * Customize the web client builder.
+	 * @param webClientBuilder the {@code WebClient.Builder} to customize
+	 */
+	void customize(WebClient.Builder webClientBuilder);
+
+}
diff --git a/...rk/boot/actuate/autoconfigure/tracing/zipkin/ZipkinAutoConfigurationIntegrationTests.java b/...rk/boot/actuate/autoconfigure/tracing/zipkin/ZipkinAutoConfigurationIntegrationTests.java
@@ -0,0 +1,72 @@
+/*
+ * Copyright 2012-2022 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.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.boot.actuate.autoconfigure.tracing.zipkin;
+
+import org.junit.jupiter.api.Test;
+import zipkin2.reporter.urlconnection.URLConnectionSender;
+
+import org.springframework.boot.actuate.autoconfigure.metrics.MetricsAutoConfiguration;
+import org.springframework.boot.actuate.autoconfigure.metrics.export.simple.SimpleMetricsExportAutoConfiguration;
+import org.springframework.boot.actuate.autoconfigure.metrics.web.client.HttpClientMetricsAutoConfiguration;
+import org.springframework.boot.actuate.autoconfigure.observation.ObservationAutoConfiguration;
+import org.springframework.boot.actuate.autoconfigure.tracing.BraveAutoConfiguration;
+import org.springframework.boot.actuate.autoconfigure.tracing.MicrometerTracingAutoConfiguration;
+import org.springframework.boot.autoconfigure.AutoConfigurations;
+import org.springframework.boot.autoconfigure.web.client.RestTemplateAutoConfiguration;
+import org.springframework.boot.autoconfigure.web.reactive.function.client.WebClientAutoConfiguration;
+import org.springframework.boot.test.context.FilteredClassLoader;
+import org.springframework.boot.test.context.assertj.ApplicationContextAssertProvider;
+import org.springframework.boot.test.context.runner.AbstractApplicationContextRunner;
+import org.springframework.boot.test.context.runner.ReactiveWebApplicationContextRunner;
+import org.springframework.boot.test.context.runner.WebApplicationContextRunner;
+import org.springframework.context.ConfigurableApplicationContext;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Integration tests for {@link ZipkinAutoConfiguration} and other related
+ * auto-configurations.
+ *
+ * @author Andy Wilkinson
+ */
+class ZipkinAutoConfigurationIntegrationTests {
+
+	@Test
+	void zipkinsUseOfRestTemplateDoesNotCauseACycle() {
+		configure(new WebApplicationContextRunner())
+				.withConfiguration(AutoConfigurations.of(RestTemplateAutoConfiguration.class))
+				.run((context) -> assertThat(context).hasNotFailed());
+	}
+
+	@Test
+	void zipkinsUseOfWebClientDoesNotCauseACycle() {
+		configure(new ReactiveWebApplicationContextRunner())
+				.withConfiguration(AutoConfigurations.of(WebClientAutoConfiguration.class))
+				.run((context) -> assertThat(context).hasNotFailed());
+	}
+
+	<SELF extends AbstractApplicationContextRunner<SELF, C, A>, C extends ConfigurableApplicationContext, A extends ApplicationContextAssertProvider<C>> AbstractApplicationContextRunner<SELF, C, A> configure(
+			AbstractApplicationContextRunner<SELF, ?, ?> runner) {
+		return runner
+				.withConfiguration(AutoConfigurations.of(MicrometerTracingAutoConfiguration.class,
+						ObservationAutoConfiguration.class, BraveAutoConfiguration.class, ZipkinAutoConfiguration.class,
+						HttpClientMetricsAutoConfiguration.class, MetricsAutoConfiguration.class,
+						SimpleMetricsExportAutoConfiguration.class))
+				.withClassLoader(new FilteredClassLoader(URLConnectionSender.class));
+	}
+
+}
diff --git a/...ot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurationsSenderConfigurationTests.java b/...ot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurationsSenderConfigurationTests.java
@@ -17,6 +17,7 @@
 package org.springframework.boot.actuate.autoconfigure.tracing.zipkin;
 
 import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentMatchers;
 import zipkin2.reporter.Sender;
 import zipkin2.reporter.urlconnection.URLConnectionSender;
 
@@ -26,12 +27,12 @@
 import org.springframework.boot.test.context.runner.ApplicationContextRunner;
 import org.springframework.boot.test.context.runner.ReactiveWebApplicationContextRunner;
 import org.springframework.boot.test.context.runner.WebApplicationContextRunner;
-import org.springframework.boot.web.client.RestTemplateBuilder;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.web.reactive.function.client.WebClient;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.BDDMockito.then;
 import static org.mockito.Mockito.mock;
 
 /**
@@ -66,6 +67,8 @@ void shouldPreferWebClientSenderIfWebApplicationIsReactiveAndUrlSenderIsNotAvail
 					assertThat(context).doesNotHaveBean(URLConnectionSender.class);
 					assertThat(context).hasSingleBean(Sender.class);
 					assertThat(context).hasSingleBean(ZipkinWebClientSender.class);
+					then(context.getBean(ZipkinWebClientBuilderCustomizer.class)).should()
+							.customize(ArgumentMatchers.any());
 				});
 	}
 
@@ -90,29 +93,29 @@ void shouldPreferWebClientInNonWebApplicationAndUrlConnectionSenderIsNotAvailabl
 	}
 
 	@Test
-	void willUseRestTemplateInNonWebApplicationIfUrlConnectionSenderIsNotAvailable() {
+	void willUseRestTemplateInNonWebApplicationIfUrlConnectionSenderAndWebclientAreNotAvailable() {
 		this.contextRunner.withUserConfiguration(RestTemplateConfiguration.class)
-				.withClassLoader(new FilteredClassLoader("zipkin2.reporter.urlconnection")).run((context) -> {
+				.withClassLoader(new FilteredClassLoader(URLConnectionSender.class, WebClient.class)).run((context) -> {
 					assertThat(context).doesNotHaveBean(URLConnectionSender.class);
 					assertThat(context).hasSingleBean(Sender.class);
 					assertThat(context).hasSingleBean(ZipkinRestTemplateSender.class);
 				});
 	}
 
 	@Test
-	void willUseRestTemplateInServletWebApplicationIfUrlConnectionSenderIsNotAvailable() {
+	void willUseRestTemplateInServletWebApplicationIfUrlConnectionSenderAndWebClientNotAvailable() {
 		this.servletContextRunner.withUserConfiguration(RestTemplateConfiguration.class)
-				.withClassLoader(new FilteredClassLoader("zipkin2.reporter.urlconnection")).run((context) -> {
+				.withClassLoader(new FilteredClassLoader(URLConnectionSender.class, WebClient.class)).run((context) -> {
 					assertThat(context).doesNotHaveBean(URLConnectionSender.class);
 					assertThat(context).hasSingleBean(Sender.class);
 					assertThat(context).hasSingleBean(ZipkinRestTemplateSender.class);
 				});
 	}
 
 	@Test
-	void willUseRestTemplateInReactiveWebApplicationIfUrlConnectionSenderIsNotAvailable() {
+	void willUseRestTemplateInReactiveWebApplicationIfUrlConnectionSenderAndWebClientAreNotAvailable() {
 		this.reactiveContextRunner.withUserConfiguration(RestTemplateConfiguration.class)
-				.withClassLoader(new FilteredClassLoader("zipkin2.reporter.urlconnection")).run((context) -> {
+				.withClassLoader(new FilteredClassLoader(URLConnectionSender.class, WebClient.class)).run((context) -> {
 					assertThat(context).doesNotHaveBean(URLConnectionSender.class);
 					assertThat(context).hasSingleBean(Sender.class);
 					assertThat(context).hasSingleBean(ZipkinRestTemplateSender.class);
@@ -140,8 +143,8 @@ void shouldBackOffOnCustomBeans() {
 	private static class RestTemplateConfiguration {
 
 		@Bean
-		RestTemplateBuilder restTemplateBuilder() {
-			return new RestTemplateBuilder();
+		ZipkinRestTemplateBuilderCustomizer restTemplateBuilder() {
+			return mock(ZipkinRestTemplateBuilderCustomizer.class);
 		}
 
 	}
@@ -150,8 +153,8 @@ RestTemplateBuilder restTemplateBuilder() {
 	private static class WebClientConfiguration {
 
 		@Bean
-		WebClient.Builder webClientBuilder() {
-			return WebClient.builder();
+		ZipkinWebClientBuilderCustomizer webClientBuilder() {
+			return mock(ZipkinWebClientBuilderCustomizer.class);
 		}
 
 	}