Permits Actuator endpoints to depend on HttpTracing (#1680)

* Permits Actuator endpoints to depend on HttpTracing

It is unusual to inject HttpTracing into an actuator endpoint vs an end-user api
such as `Tracer` or `SpanCustomizer`. However, we decided to allow this and so
need a test to prove this continues to work.

Fixes #1679

Author: adriancole

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 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.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 @@
 @EnableConfigurationProperties(SleuthWebProperties.class)
 public class TraceWebAutoConfiguration {
 
-	@Autowired(required = false)
-	List<SingleSkipPattern> patterns = new ArrayList<>();
-
 	@Bean
 	@ConditionalOnMissingBean
-	SkipPatternProvider sleuthSkipPatternProvider() {
-		if (this.patterns == null) {
+	SkipPatternProvider sleuthSkipPatternProvider(
+			@Nullable List<SingleSkipPattern> patterns) {
+		if (patterns == null || patterns.isEmpty()) {
 			return null;
 		}
-		List<Pattern> 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<SingleSkipPattern> patterns) {
+		List<Pattern> 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)

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

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java

@@ -19,6 +19,7 @@
 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.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 void should_return_management_context_with_context_path() throws Exceptio
 
 	@Test
 	public void should_return_empty_when_no_endpoints() {
-		EndpointsSupplier<ExposableWebEndpoint> endpointsSupplier = Collections::emptyList;
 		Optional<Pattern> pattern = new TraceWebAutoConfiguration.ActuatorSkipPatternProviderConfig()
 				.skipPatternForActuatorEndpointsSamePort(new ServerProperties(),
-						new WebEndpointProperties(), endpointsSupplier)
+						new WebEndpointProperties(), Collections::emptyList)
 				.skipPattern();
 
 		then(pattern).isEmpty();
@@ -237,9 +238,9 @@ public void should_return_endpoints_with_context_path_different_port() {
 	@Test
 	public void should_combine_skip_patterns_from_list() throws Exception {
 		TraceWebAutoConfiguration configuration = new TraceWebAutoConfiguration();
-		configuration.patterns.addAll(Arrays.asList(foo(), bar()));
+		List<SingleSkipPattern> 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 @@ static class ServerPropertiesConfig {
 
 	}
 
+	@Configuration
+	static class EmptyEndpoints {
+
+		@Bean
+		EndpointsSupplier<ExposableWebEndpoint> endpointsSupplier() {
+			return Collections::emptyList;
+		}
+
+	}
+
 }

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();
+
+		LazyBean<CurrentTraceContext> provider = LazyBean.create(context,
+				CurrentTraceContext.class);
+
+		then(provider.get()).isNotNull();
+	}
 
-		when(springContext.getBean(CurrentTraceContext.class))
-				.thenThrow(new IllegalStateException());
+	@Test
+	public void should_return_null_when_no_basic_type() {
+		context.refresh();
 
-		LazyBean<CurrentTraceContext> provider = new LazyBean<>(springContext,
+		LazyBean<CurrentTraceContext> provider = LazyBean.create(context,
 				CurrentTraceContext.class);
 
 		then(provider.get()).isNull();
 	}
 
+	@Configuration
+	static class BasicConfig {
+
+		@Bean
+		CurrentTraceContext currentTraceContext() {
+			return CurrentTraceContext.Default.create();
+		}
+
+	}
+
 }