From b41ed44b60822dd02e40e37b9a9f2f691e22146e Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 28 Sep 2022 15:54:42 +0200 Subject: [PATCH 1/2] Break cycles between Zipkin senders and HTTP client observation Previously, RestTemplateBuilder and WebClient.Builder beans were used to create the HTTP client for sending out spans. Those same beans are also instrumented for observability which results in a cycle. This commit breaks the cycle by not using the application-web builders to create the RestTemplate and WebClient's used by the Zipkin senders. Instead, builders are created inline, with new callbacks being introduced to allow the user to customize these Zipkin-specific builders. See gh-32528 --- .../tracing/zipkin/ZipkinConfigurations.java | 21 +++++++++------- ...onfigurationsSenderConfigurationTests.java | 25 +++++++++++-------- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java index be3c1292f8..85d90bd945 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java @@ -16,6 +16,8 @@ package org.springframework.boot.actuate.autoconfigure.tracing.zipkin; +import java.util.List; + import io.opentelemetry.exporter.zipkin.ZipkinSpanExporter; import zipkin2.Span; import zipkin2.codec.BytesEncoder; @@ -73,12 +75,12 @@ class ZipkinConfigurations { @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); + List customizers) { + RestTemplateBuilder restTemplateBuilder = new RestTemplateBuilder() + .setConnectTimeout(properties.getConnectTimeout()).setReadTimeout(properties.getReadTimeout()); + customizers.forEach((c) -> c.customize(restTemplateBuilder)); + return new ZipkinRestTemplateSender(properties.getEndpoint(), restTemplateBuilder.build()); } } @@ -90,10 +92,11 @@ class ZipkinConfigurations { @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, + List customizers) { + WebClient.Builder builder = WebClient.builder(); + customizers.forEach((c) -> c.customize(builder)); + return new ZipkinWebClientSender(properties.getEndpoint(), builder.build()); } } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurationsSenderConfigurationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurationsSenderConfigurationTests.java index 727a6b854d..892e3e0175 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurationsSenderConfigurationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/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.FilteredClassLoader; 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 @@ class ZipkinConfigurationsSenderConfigurationTests { 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,9 +93,9 @@ class ZipkinConfigurationsSenderConfigurationTests { } @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); @@ -100,9 +103,9 @@ class ZipkinConfigurationsSenderConfigurationTests { } @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); @@ -110,9 +113,9 @@ class ZipkinConfigurationsSenderConfigurationTests { } @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 @@ class ZipkinConfigurationsSenderConfigurationTests { private static class RestTemplateConfiguration { @Bean - RestTemplateBuilder restTemplateBuilder() { - return new RestTemplateBuilder(); + ZipkinRestTemplateBuilderCustomizer restTemplateBuilder() { + return mock(ZipkinRestTemplateBuilderCustomizer.class); } } @@ -150,8 +153,8 @@ class ZipkinConfigurationsSenderConfigurationTests { private static class WebClientConfiguration { @Bean - WebClient.Builder webClientBuilder() { - return WebClient.builder(); + ZipkinWebClientBuilderCustomizer webClientBuilder() { + return mock(ZipkinWebClientBuilderCustomizer.class); } } From 574242b44f681b00bbb7303886415d0a8b8a83fd Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 28 Sep 2022 20:15:02 +0100 Subject: [PATCH 2/2] Polish "Break cycles between Zipkin senders and HTTP client observation" See gh-32528 --- .../tracing/zipkin/ZipkinConfigurations.java | 11 ++- .../ZipkinRestTemplateBuilderCustomizer.java | 37 ++++++++++ .../ZipkinWebClientBuilderCustomizer.java | 38 ++++++++++ ...pkinAutoConfigurationIntegrationTests.java | 72 +++++++++++++++++++ 4 files changed, 152 insertions(+), 6 deletions(-) create mode 100644 spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinRestTemplateBuilderCustomizer.java create mode 100644 spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinWebClientBuilderCustomizer.java create mode 100644 spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinAutoConfigurationIntegrationTests.java diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java index 85d90bd945..72c196ceb5 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinConfigurations.java @@ -16,8 +16,6 @@ package org.springframework.boot.actuate.autoconfigure.tracing.zipkin; -import java.util.List; - import io.opentelemetry.exporter.zipkin.ZipkinSpanExporter; import zipkin2.Span; import zipkin2.codec.BytesEncoder; @@ -27,6 +25,7 @@ import zipkin2.reporter.Sender; 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; @@ -76,10 +75,10 @@ class ZipkinConfigurations { @Bean @ConditionalOnMissingBean(Sender.class) ZipkinRestTemplateSender restTemplateSender(ZipkinProperties properties, - List customizers) { + ObjectProvider customizers) { RestTemplateBuilder restTemplateBuilder = new RestTemplateBuilder() .setConnectTimeout(properties.getConnectTimeout()).setReadTimeout(properties.getReadTimeout()); - customizers.forEach((c) -> c.customize(restTemplateBuilder)); + customizers.orderedStream().forEach((c) -> c.customize(restTemplateBuilder)); return new ZipkinRestTemplateSender(properties.getEndpoint(), restTemplateBuilder.build()); } @@ -93,9 +92,9 @@ class ZipkinConfigurations { @Bean @ConditionalOnMissingBean(Sender.class) ZipkinWebClientSender webClientSender(ZipkinProperties properties, - List customizers) { + ObjectProvider customizers) { WebClient.Builder builder = WebClient.builder(); - customizers.forEach((c) -> c.customize(builder)); + customizers.orderedStream().forEach((c) -> c.customize(builder)); return new ZipkinWebClientSender(properties.getEndpoint(), builder.build()); } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinRestTemplateBuilderCustomizer.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinRestTemplateBuilderCustomizer.java new file mode 100644 index 0000000000..a1ee937d3e --- /dev/null +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/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/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinWebClientBuilderCustomizer.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinWebClientBuilderCustomizer.java new file mode 100644 index 0000000000..bd6be6e933 --- /dev/null +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/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/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinAutoConfigurationIntegrationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/ZipkinAutoConfigurationIntegrationTests.java new file mode 100644 index 0000000000..c6aeaf336a --- /dev/null +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/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()); + } + + , C extends ConfigurableApplicationContext, A extends ApplicationContextAssertProvider> AbstractApplicationContextRunner configure( + AbstractApplicationContextRunner 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)); + } + +}