diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java index 98e7ca8eb..19301464a 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java @@ -16,7 +16,6 @@ package org.springframework.cloud.sleuth.instrument.web; -import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Optional; @@ -26,7 +25,7 @@ import java.util.stream.Collectors; import brave.Tracing; -import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.BeanCurrentlyInCreationException; import org.springframework.boot.actuate.autoconfigure.endpoint.web.WebEndpointProperties; import org.springframework.boot.actuate.autoconfigure.web.server.ConditionalOnManagementPort; import org.springframework.boot.actuate.autoconfigure.web.server.ManagementPortType; @@ -43,6 +42,7 @@ import org.springframework.boot.context.properties.EnableConfigurationProperties import org.springframework.cloud.sleuth.autoconfig.TraceAutoConfiguration; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.lang.Nullable; import org.springframework.util.StringUtils; /** @@ -64,32 +64,56 @@ import org.springframework.util.StringUtils; @EnableConfigurationProperties(SleuthWebProperties.class) public class TraceWebAutoConfiguration { - @Autowired(required = false) - List patterns = new ArrayList<>(); - @Bean @ConditionalOnMissingBean - SkipPatternProvider sleuthSkipPatternProvider() { - if (this.patterns == null) { + SkipPatternProvider sleuthSkipPatternProvider( + @Nullable List patterns) { + if (patterns == null || patterns.isEmpty()) { return null; } - List presentPatterns = this.patterns.stream() + + // Actuator endpoints are queried to make the default skip pattern. There's an + // edge case where actuator endpoints indirectly reference the still constructing + // HttpTracing bean. Ex: an instrumented client could cause a cyclic dep. + // + // Below optimizes for the opposite: that custom actuator endpoints are not in + // use. This allows configuration to be eagerly parsed, allowing any errors to + // surface earlier. In the case there is a cyclic dep, this parsing becomes lazy, + // deferring any errors creating the skip pattern. + // + // See #1679 + try { + Pattern result = consolidateSkipPatterns(patterns); + if (result == null) { + return null; + } + return () -> result; + } + catch (BeanCurrentlyInCreationException e) { + // Most likely, there is an actuator endpoint that indirectly references an + // instrumented HTTP client. + return () -> consolidateSkipPatterns(patterns); + } + } + + @Nullable + static Pattern consolidateSkipPatterns(List patterns) { + List presentPatterns = patterns.stream() .map(SingleSkipPattern::skipPattern).filter(Optional::isPresent) .map(Optional::get).collect(Collectors.toList()); if (presentPatterns.isEmpty()) { return null; } if (presentPatterns.size() == 1) { - Pattern pattern = presentPatterns.get(0); - return () -> pattern; + return presentPatterns.get(0); } + StringJoiner joiner = new StringJoiner("|"); for (Pattern pattern : presentPatterns) { String s = pattern.pattern(); joiner.add(s); } - Pattern pattern = Pattern.compile(joiner.toString()); - return () -> pattern; + return Pattern.compile(joiner.toString()); } @Configuration(proxyBeanMethods = false) diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/EndpointWithCyclicDependenciesTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/EndpointWithCyclicDependenciesTests.java new file mode 100644 index 000000000..a998461e6 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/EndpointWithCyclicDependenciesTests.java @@ -0,0 +1,74 @@ +/* + * Copyright 2013-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. + * 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.cloud.sleuth.instrument.web; + +import brave.http.HttpTracing; +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.actuate.endpoint.web.annotation.RestControllerEndpoint; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.stereotype.Service; + +/** + * This tests that actuator components can have instrumented HTTP clients inside of them. + * + * @author Marcin Grzejszczak + */ +@SpringBootTest(classes = { EndpointWithCyclicDependenciesTests.ClientConfig.class }) +public class EndpointWithCyclicDependenciesTests { + + @Test + void should_load_context() { + } + + static class Client { + + } + + @EnableAutoConfiguration + @Configuration + static class ClientConfig { + + @Bean + public Client client(HttpTracing httpTracing) { + // imagine this instruments the client. + return new Client(); + } + + } + + @Service + static class MyService { + + @Autowired + Client client; + + } + + @RestControllerEndpoint(id = "admin-endpoint") + static class MyRestEndpoint { + + @Autowired + MyService myService; + + } + +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java index 9ff60d736..302b49672 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java @@ -19,6 +19,7 @@ package org.springframework.cloud.sleuth.instrument.web; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.List; import java.util.Optional; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -42,6 +43,7 @@ import org.springframework.boot.context.properties.EnableConfigurationProperties import org.springframework.boot.test.context.runner.WebApplicationContextRunner; import org.springframework.cloud.sleuth.autoconfig.TraceAutoConfiguration; import org.springframework.context.ApplicationContext; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import static org.assertj.core.api.BDDAssertions.then; @@ -113,10 +115,9 @@ public class SkipPatternProviderConfigTest { @Test public void should_return_empty_when_no_endpoints() { - EndpointsSupplier endpointsSupplier = Collections::emptyList; Optional pattern = new TraceWebAutoConfiguration.ActuatorSkipPatternProviderConfig() .skipPatternForActuatorEndpointsSamePort(new ServerProperties(), - new WebEndpointProperties(), endpointsSupplier) + new WebEndpointProperties(), Collections::emptyList) .skipPattern(); then(pattern).isEmpty(); @@ -237,9 +238,9 @@ public class SkipPatternProviderConfigTest { @Test public void should_combine_skip_patterns_from_list() throws Exception { TraceWebAutoConfiguration configuration = new TraceWebAutoConfiguration(); - configuration.patterns.addAll(Arrays.asList(foo(), bar())); + List patterns = Arrays.asList(foo(), bar()); - Pattern pattern = configuration.sleuthSkipPatternProvider().skipPattern(); + Pattern pattern = configuration.sleuthSkipPatternProvider(patterns).skipPattern(); then(pattern.pattern()).isEqualTo("foo|bar"); } @@ -276,4 +277,14 @@ public class SkipPatternProviderConfigTest { } + @Configuration + static class EmptyEndpoints { + + @Bean + EndpointsSupplier endpointsSupplier() { + return Collections::emptyList; + } + + } + } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/internal/LazyBeanTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/internal/LazyBeanTests.java index db03fb410..b985c848c 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/internal/LazyBeanTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/internal/LazyBeanTests.java @@ -17,28 +17,53 @@ package org.springframework.cloud.sleuth.internal; import brave.propagation.CurrentTraceContext; +import org.junit.After; import org.junit.Test; -import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; import static org.assertj.core.api.BDDAssertions.then; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; public class LazyBeanTests { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(); + + @After + public void close() { + context.close(); + } + @Test - public void should_return_null_when_exception_thrown_upon_bean_retrieval() { - ConfigurableApplicationContext springContext = mock( - ConfigurableApplicationContext.class); + public void should_work_with_basic_type() { + context.register(BasicConfig.class); + context.refresh(); - when(springContext.getBean(CurrentTraceContext.class)) - .thenThrow(new IllegalStateException()); + LazyBean provider = LazyBean.create(context, + CurrentTraceContext.class); - LazyBean provider = new LazyBean<>(springContext, + then(provider.get()).isNotNull(); + } + + @Test + public void should_return_null_when_no_basic_type() { + context.refresh(); + + LazyBean provider = LazyBean.create(context, CurrentTraceContext.class); then(provider.get()).isNull(); } + @Configuration + static class BasicConfig { + + @Bean + CurrentTraceContext currentTraceContext() { + return CurrentTraceContext.Default.create(); + } + + } + }